ddt: fix refcount bypass and gang member leak for dedup gang blocks

include/sys/ddt.h

@@ -129,9 +129,12 @@ typedef struct {
  * characteristics of the stored block, such as its location on disk (DVAs),
  * birth txg and ref count.
  *
- * The "traditional" entry has an array of four, one for each number of DVAs
- * (copies= property) and another for additional "ditto" copies. Users of the
- * traditional struct will specify the variant (index) of the one they want.
+ * The "traditional" entry has an array of four, one for each value of the
+ * copies= property the block was written with and another for additional
+ * "ditto" copies. (The stored block's BP may carry more DVAs than copies=,
+ * eg a gang header is stored in more copies than the data it gangs, so the
+ * slot is not the BP's DVA count.) Users of the traditional struct will
+ * specify the variant (index) of the one they want.
  *
  * The newer "flat" entry has only a single form that is specified using the
  * DDT_PHYS_FLAT variant.

module/zfs/ddt.c

@@ -78,10 +78,13 @@
  *
  * Traditionally, each ddt_phys_t slot in the entry represents a separate dedup
  * block for the same content/checksum. The slot is selected based on the
- * zp_copies parameter the block is written with, that is, the number of DVAs
- * in the block. The "ditto" slot (DDT_PHYS_DITTO) used to be used for
- * now-removed "dedupditto" feature. These are no longer written, and will be
- * freed if encountered on old pools.
+ * zp_copies parameter the block is written with. Note that the block may
+ * carry more DVAs than zp_copies (a gang header is stored in more copies
+ * than the data it gangs), so the slot cannot be inferred from the BP's DVA
+ * count; a stored phys is matched to a BP by block identity (see
+ * ddt_phys_select()). The "ditto" slot (DDT_PHYS_DITTO) used to be used for
+ * the now-removed "dedupditto" feature. These are no longer written, and
+ * will be freed if encountered on old pools.
  *
  * If the "fast_dedup" feature is enabled, new dedup tables will be created
  * with the "flat phys" option. In this mode, there is only one ddt_phys_t
@@ -1187,44 +1190,31 @@ ddt_prefetch_all(spa_t *spa)
 static int ddt_configure(ddt_t *ddt, boolean_t new);
 
 /*
- * If the BP passed to ddt_lookup has valid DVAs, then we need to compare them
- * to the ones in the entry. If they're different, then the passed-in BP is
- * from a previous generation of this entry (ie was previously pruned) and we
+ * If the BP passed to ddt_lookup has valid DVAs, then we need to check that
+ * they match one of the phys in the entry. If not, then the passed-in BP is
+ * from a previous generation of this entry (eg was previously pruned) and we
  * have to act like the entry doesn't exist at all.
  *
- * This should only happen during a lookup to free the block (zio_ddt_free()).
+ * Callers that pass verify expect the entry they get back to hold a phys
+ * matching the BP in hand.
  *
- * XXX this is similar in spirit to ddt_phys_select(), maybe can combine
- *       -- robn, 2024-02-09
+ * The match is made on block identity (DVA[0] and physical birth) via
+ * ddt_phys_select(). The phys slot must not be inferred from the BP's DVA
+ * count: a gang header is stored in more copies than the data it gangs, so
+ * its BP carries more DVAs than the copies value the block was written with
+ * (eg a copies=1 dedup gang block has a two-DVA header BP but lives in the
+ * copies=1 slot). Slot-by-DVA-count would therefore check the wrong slot and
+ * misread a live entry as pruned, bypassing its refcount when the block is
+ * freed.
  */
 static boolean_t
 ddt_entry_lookup_is_valid(ddt_t *ddt, const blkptr_t *bp, ddt_entry_t *dde)
 {
 	/* If the BP has no DVAs, then this entry is good */
-	uint_t ndvas = BP_GET_NDVAS(bp);
-	if (ndvas == 0)
+	if (BP_GET_NDVAS(bp) == 0)
 		return (B_TRUE);
 
-	/*
-	 * Only checking the phys for the copies. For flat, there's only one;
-	 * for trad it'll be the one that has the matching set of DVAs.
-	 */
-	const dva_t *dvas = (ddt->ddt_flags & DDT_FLAG_FLAT) ?
-	    dde->dde_phys->ddp_flat.ddp_dva :
-	    dde->dde_phys->ddp_trad[ndvas].ddp_dva;
-
-	/*
-	 * Compare entry DVAs with the BP. They should all be there, but
-	 * there's not really anything we can do if its only partial anyway,
-	 * that's an error somewhere else, maybe long ago.
-	 */
-	uint_t d;
-	for (d = 0; d < ndvas; d++)
-		if (!DVA_EQUAL(&dvas[d], &bp->blk_dva[d]))
-			return (B_FALSE);
-	ASSERT3U(d, ==, ndvas);
-
-	return (B_TRUE);
+	return (ddt_phys_select(ddt, dde, bp) != DDT_PHYS_NONE);
 }
 
 ddt_entry_t *
@@ -2694,10 +2684,15 @@ ddt_addref(spa_t *spa, const blkptr_t *bp)
 		 * This entry was either synced to a store object (dde_type is
 		 * real) or was logged. It must be properly on disk at this
 		 * point, so we can just bump its refcount.
+		 *
+		 * The verified lookup above guarantees a matching phys
+		 * exists for any BP that carries DVAs, and clone BPs always
+		 * do; the VERIFY keeps DDT_PHYS_NONE from reaching
+		 * ddt_phys_addref(), which would index out of bounds on
+		 * release builds.
 		 */
-		int p = DDT_PHYS_FOR_COPIES(ddt, BP_GET_NDVAS(bp));
-		ddt_phys_variant_t v = DDT_PHYS_VARIANT(ddt, p);
-
+		ddt_phys_variant_t v = ddt_phys_select(ddt, dde, bp);
+		VERIFY3U(v, !=, DDT_PHYS_NONE);
 		ddt_phys_addref(dde->dde_phys, v);
 		result = B_TRUE;
 	} else {

module/zfs/zio.c

@@ -2132,7 +2132,12 @@ zio_free_bp_init(zio_t *zio)
 
 	if (zio->io_child_type == ZIO_CHILD_LOGICAL) {
 		if (BP_GET_DEDUP(bp))
-			zio->io_pipeline = ZIO_DDT_FREE_PIPELINE;
+			/*
+			 * Keep the gang stages zio_create() added: if
+			 * zio_ddt_free() falls back to a plain free, they
+			 * free the gang members along with the header.
+			 */
+			zio->io_pipeline |= ZIO_DDT_FREE_PIPELINE;
 	}
 
 	ASSERT3P(zio->io_bp, ==, &zio->io_bp_copy);
@@ -4174,13 +4179,12 @@ zio_ddt_free(zio_t *zio)
 			ddt_phys_decref(dde->dde_phys, v);
 		else
 			/*
-			 * If the entry was found but the phys was not, then
-			 * this block must have been pruned from the dedup
-			 * table, and the entry refers to a later version of
-			 * this data. Therefore, the caller is trying to delete
-			 * the only stored instance of this block, and so we
-			 * need to do a normal (not dedup) free. Clear dde so
-			 * we fall into the block below.
+			 * No phys matches this BP; ddt_lookup() returned a
+			 * fresh, empty entry because the key is not in the
+			 * table at all (eg the original entry was pruned).
+			 * There is no reference to release, so we need to do
+			 * a normal (not dedup) free. Clear dde so we fall
+			 * into the block below.
 			 */
 			dde = NULL;
 	}
@@ -4198,8 +4202,24 @@ zio_ddt_free(zio_t *zio)
 		 * table.  Clear the DEDUP bit so it is treated as a normal
 		 * block from here on.  BRT_FREE and DVA_FREE follow in the
 		 * pipeline and will handle any cloned references and the
-		 * actual block free respectively.
+		 * actual block free respectively, along with the gang stages
+		 * for a gang BP.
+		 *
+		 * Only flat (FDT) tables are ever pruned, so a miss against
+		 * a traditional table means the table and the BP disagree,
+		 * which should not be possible. The plain free below is
+		 * still the best we can do for this BP, but leave a trace.
 		 */
+		if (!(ddt->ddt_flags & DDT_FLAG_FLAT)) {
+			zfs_dbgmsg("%s: no matching traditional DDT phys for "
+			    "dedup BP DVA[0]=<%llu:%llx:%llx> phys_birth=%llu; "
+			    "freeing without a refcount decrement",
+			    spa_name(spa),
+			    (u_longlong_t)DVA_GET_VDEV(&bp->blk_dva[0]),
+			    (u_longlong_t)DVA_GET_OFFSET(&bp->blk_dva[0]),
+			    (u_longlong_t)DVA_GET_ASIZE(&bp->blk_dva[0]),
+			    (u_longlong_t)BP_GET_PHYSICAL_BIRTH(bp));
+		}
 		BP_SET_DEDUP(bp, 0);
 	}
 

tests/runfiles/linux.run

@@ -94,6 +94,10 @@ tags = ['functional', 'cli_root', 'zpool_split']
 tests = ['compress_004_pos']
 tags = ['functional', 'compression']
 
+[tests/functional/dedup:Linux]
+tests = ['dedup_legacy_gang']
+tags = ['functional', 'dedup']
+
 [tests/functional/devices:Linux]
 tests = ['devices_001_pos', 'devices_002_neg', 'devices_003_pos']
 tags = ['functional', 'devices']

tests/zfs-tests/tests/Makefile.am

@@ -1559,6 +1559,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/dedup/dedup_fdt_import.ksh \
 	functional/dedup/dedup_fdt_pacing.ksh \
 	functional/dedup/dedup_legacy_create.ksh \
+	functional/dedup/dedup_legacy_gang.ksh \
 	functional/dedup/dedup_legacy_import.ksh \
 	functional/dedup/dedup_legacy_fdt_upgrade.ksh \
 	functional/dedup/dedup_legacy_fdt_mixed.ksh \

tests/zfs-tests/tests/functional/dedup/dedup_legacy_gang.ksh

@@ -0,0 +1,177 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or https://opensource.org/licenses/CDDL-1.0.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+# DESCRIPTION:
+#	Verify that deduplicated gang blocks in a traditional (legacy) DDT
+#	are refcounted correctly.  A gang header is stored in more copies
+#	than the data it gangs, so its BP carries more DVAs than the
+#	copies= value the block was written with, and the free-time and
+#	clone-time DDT phys selection must match by block identity, not by
+#	the BP's DVA count.  Getting this wrong bypassed the refcount:
+#	each reference delete physically freed the shared gang header (a
+#	committed double-free from the second delete on), leaked the gang
+#	members and left a stale DDT entry behind.
+#
+# STRATEGY:
+#	1. Create a pool with feature@fast_dedup disabled (traditional DDT)
+#	   and a dedup dataset.
+#	2. Force ganging and write a file of unique blocks; verify every
+#	   block is a two-DVA gang BP stored in the copies=1 phys slot.
+#	3. Create a second reference via dedup (copy) and a third via
+#	   block cloning; verify each bumps the refcount of that same
+#	   phys slot, and that the clone took no BRT reference.
+#	4. Delete the references one at a time; verify the refcount drops
+#	   by exactly one each time and a survivor is still readable
+#	   after a fresh import.
+#	5. After the last delete, verify the DDT is empty and zdb reports
+#	   no leaked space or double allocations.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/gang_blocks/gang_blocks.kshlib
+
+verify_runnable "global"
+
+log_assert "Deduplicated gang blocks in a legacy DDT are refcounted correctly"
+
+log_must save_tunable METASLAB_FORCE_GANGING
+log_must save_tunable METASLAB_FORCE_GANGING_PCT
+log_must save_tunable BCLONE_ENABLED
+
+function cleanup
+{
+	if poolexists $TESTPOOL ; then
+		destroy_pool $TESTPOOL
+	fi
+	log_must restore_tunable METASLAB_FORCE_GANGING
+	log_must restore_tunable METASLAB_FORCE_GANGING_PCT
+	log_must restore_tunable BCLONE_ENABLED
+}
+
+log_onexit cleanup
+
+# Count DDT entries whose copies=1 phys slot holds a two-DVA gang BP with
+# the given refcount - the shape whose refcounting this test verifies.
+# A zdb failure yields a non-numeric result, failing the caller's test.
+function count_gang_phys1
+{
+	typeset refcnt=$1
+	typeset out
+
+	out=$(zdb -DDDDD $TESTPOOL) || { echo "zdb failed"; return; }
+	echo "$out" | grep -c "refcnt $refcnt phys 1 .*gang.*double"
+}
+
+# We disable compression so our writes create predictable results on disk,
+# and use xattr=sa to prevent selinux xattrs influencing our accounting.
+# ashift=9 keeps gang headers small.
+log_must zpool create -f \
+    -o ashift=9 \
+    -o feature@fast_dedup=disabled \
+    -o feature@block_cloning=enabled \
+    -O dedup=on \
+    -O compression=off \
+    -O xattr=sa \
+    $TESTPOOL $DISKS
+
+log_must test "$(get_pool_prop feature@fast_dedup $TESTPOOL)" = "disabled"
+
+# redundant_metadata=all (the default) stores gang headers in copies+1
+# copies; pin it so the copies=1 blocks below always get a two-DVA gang
+# header BP.
+log_must zfs create -o recordsize=128k -o redundant_metadata=all \
+    $TESTPOOL/$TESTFS
+typeset mountpoint=$(get_prop mountpoint $TESTPOOL/$TESTFS)
+
+# Ensure block cloning is enabled (it is on by default) so the
+# FICLONE call below succeeds instead of failing with EOPNOTSUPP.
+log_must set_tunable32 BCLONE_ENABLED 1
+
+# Write unique data with ganging forced: 4 blocks, each a dedup gang block
+# whose gang header BP carries more DVAs than copies=1. The allocation (and
+# so the ganging decision) happens at sync, so keep the tunable set until
+# the pool has synced.
+log_must set_tunable64 METASLAB_FORCE_GANGING 20000
+log_must set_tunable32 METASLAB_FORCE_GANGING_PCT 100
+log_must dd if=/dev/urandom of=$mountpoint/file1 bs=128k count=4
+sync_pool $TESTPOOL
+log_must set_tunable32 METASLAB_FORCE_GANGING_PCT 0
+
+typeset dva=$(get_first_block_dva $TESTPOOL/$TESTFS file1)
+check_is_gang_dva $dva
+
+# All four entries must have the hazardous shape: a two-DVA gang BP in
+# the copies=1 slot.
+log_must eval "zdb -D $TESTPOOL | grep -q 'DDT-sha256-zap-unique:.*entries=4'"
+log_must test "$(count_gang_phys1 1)" -eq 4
+
+typeset sum=$(xxh128digest $mountpoint/file1)
+
+# Second reference via dedup: same phys slot, refcount 2.
+log_must dd if=$mountpoint/file1 of=$mountpoint/file2 bs=128k
+sync_pool $TESTPOOL
+log_must eval "zdb -D $TESTPOOL | grep -q 'DDT-sha256-zap-duplicate:.*entries=4'"
+log_must test "$(count_gang_phys1 2)" -eq 4
+
+# Third reference via block cloning: ddt_addref() must bump the same
+# phys slot to 3; nothing may land in the BRT.  Use FICLONE (-c):
+# unlike copy_file_range it cannot silently fall back to a byte copy,
+# which would dedup to the same refcount without exercising the clone
+# path.
+log_must clonefile -c $mountpoint/file1 $mountpoint/file3
+sync_pool $TESTPOOL
+log_must test "$(get_pool_prop bcloneused $TESTPOOL)" -eq 0
+log_must test "$(count_gang_phys1 3)" -eq 4
+
+# Delete the references one at a time. Each delete must drop the
+# refcount by exactly one; the shared gang blocks stay allocated until
+# the last one.
+log_must rm $mountpoint/file1
+sync_pool $TESTPOOL
+log_must test "$(count_gang_phys1 2)" -eq 4
+
+# A survivor must still be readable from disk after a fresh import.
+log_must zpool export $TESTPOOL
+log_must zpool import $TESTPOOL
+log_must test "$(xxh128digest $mountpoint/file2)" = "$sum"
+
+log_must rm $mountpoint/file3
+sync_pool $TESTPOOL
+log_must test "$(count_gang_phys1 1)" -eq 4
+log_must test "$(xxh128digest $mountpoint/file2)" = "$sum"
+
+log_must rm $mountpoint/file2
+sync_pool $TESTPOOL
+
+# The last delete must have released the entries and freed the gang
+# members: empty DDT, no leaked space, no double frees.
+log_must eval "zdb -D $TESTPOOL | grep -q 'All DDTs are empty'"
+
+zdb_out=$(zdb -bcc $TESTPOOL 2>&1)
+typeset rc=$?
+echo "$zdb_out"
+if (( rc != 0 )) || \
+    echo "$zdb_out" | grep -Eq "leaked space|DOUBLE ALLOC|DOUBLE FREE"; then
+	log_fail "legacy dedup gang free leaked or double-freed space"
+fi
+
+log_pass "Deduplicated gang blocks in a legacy DDT are refcounted correctly"