[PATCH] xfs: fix off-by-one error in fsmap's end_daddr usage

[PATCH] xfs: fix off-by-one error in fsmap's end_daddr usage

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In commit ca6448aed4f10a, we created an "end_daddr" variable to fix
fsmap reporting when the end of the range requested falls in the middle
of an unknown (aka free on the rmapbt) region.  Unfortunately, I didn't
notice that the the code sets end_daddr to the last sector of the device
but then uses that quantity to compute the length of the synthesized
mapping.

Zizhi Wo later observed that when end_daddr isn't set, we still don't
report the last fsblock on a device because in that case (aka when
info->last is true), the info->high mapping that we pass to
xfs_getfsmap_group_helper has a startblock that points to the last
fsblock.  This is also wrong because the code uses startblock to
compute the length of the synthesized mapping.

Fix the second problem by setting end_daddr unconditionally, and fix the
first problem by setting start_daddr to one past the end of the range to
query.

Cc: <stable@vger.kernel.org> # v6.11
Fixes: ca6448aed4f10a ("xfs: Fix missing interval for missing_owner in xfs fsmap")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reported-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/xfs/xfs_fsmap.c |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index f6d20da34aba1d..0202b705dd5eb2 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -165,7 +165,8 @@ struct xfs_getfsmap_info {
 	xfs_daddr_t		next_daddr;	/* next daddr we expect */
 	/* daddr of low fsmap key when we're using the rtbitmap */
 	xfs_daddr_t		low_daddr;
-	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
+	/* daddr of high fsmap key, or the last daddr on the device */
+	xfs_daddr_t		end_daddr;
 	u64			missing_owner;	/* owner of holes */
 	u32			dev;		/* device id */
 	/*
@@ -388,8 +389,8 @@ xfs_getfsmap_group_helper(
 	 * we calculated from userspace's high key to synthesize the record.
 	 * Note that if the btree query found a mapping, there won't be a gap.
 	 */
-	if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL)
-		frec->start_daddr = info->end_daddr;
+	if (info->last)
+		frec->start_daddr = info->end_daddr + 1;
 	else
 		frec->start_daddr = xfs_gbno_to_daddr(xg, startblock);
 
@@ -737,8 +738,8 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
 	 * we calculated from userspace's high key to synthesize the record.
 	 * Note that if the btree query found a mapping, there won't be a gap.
 	 */
-	if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) {
-		frec.start_daddr = info->end_daddr;
+	if (info->last) {
+		frec.start_daddr = info->end_daddr + 1;
 	} else {
 		frec.start_daddr = xfs_rtb_to_daddr(mp, start_rtb);
 	}
@@ -1108,7 +1109,10 @@ xfs_getfsmap(
 	struct xfs_trans		*tp = NULL;
 	struct xfs_fsmap		dkeys[2];	/* per-dev keys */
 	struct xfs_getfsmap_dev		handlers[XFS_GETFSMAP_DEVS];
-	struct xfs_getfsmap_info	info = { NULL };
+	struct xfs_getfsmap_info	info = {
+		.fsmap_recs		= fsmap_recs,
+		.head			= head,
+	};
 	bool				use_rmap;
 	int				i;
 	int				error = 0;
@@ -1176,9 +1180,6 @@ xfs_getfsmap(
 
 	info.next_daddr = head->fmh_keys[0].fmr_physical +
 			  head->fmh_keys[0].fmr_length;
-	info.end_daddr = XFS_BUF_DADDR_NULL;
-	info.fsmap_recs = fsmap_recs;
-	info.head = head;
 
 	/* For each device we support... */
 	for (i = 0; i < XFS_GETFSMAP_DEVS; i++) {
@@ -1191,17 +1192,23 @@ xfs_getfsmap(
 			break;
 
 		/*
-		 * If this device number matches the high key, we have
-		 * to pass the high key to the handler to limit the
-		 * query results.  If the device number exceeds the
-		 * low key, zero out the low key so that we get
-		 * everything from the beginning.
+		 * If this device number matches the high key, we have to pass
+		 * the high key to the handler to limit the query results, and
+		 * set the end_daddr so that we can synthesize records at the
+		 * end of the query range or device.
 		 */
 		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
 			dkeys[1] = head->fmh_keys[1];
 			info.end_daddr = min(handlers[i].nr_sectors - 1,
 					     dkeys[1].fmr_physical);
+		} else {
+			info.end_daddr = handlers[i].nr_sectors - 1;
 		}
+
+		/*
+		 * If the device number exceeds the low key, zero out the low
+		 * key so that we get everything from the beginning.
+		 */
 		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
 			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
 

[PATCH] xfs/273: check thoroughness of the fsmappings

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Enhance this test to make sure that there are no gaps in the fsmap
records, and (especially) that they we report all the way to the end of
the device.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/273 |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tests/xfs/273 b/tests/xfs/273
index d7fb80c4033429..ecfe5e7760a092 100755
--- a/tests/xfs/273
+++ b/tests/xfs/273
@@ -24,6 +24,8 @@ _require_scratch
 _require_populate_commands
 _require_xfs_io_command "fsmap"
 
+_fixed_by_git_commit kernel XXXXXXXXXXXXXX "xfs: fix off-by-one error in fsmap's end_daddr usage"
+
 rm -f "$seqres.full"
 
 echo "Format and mount"
@@ -37,6 +39,51 @@ cat $TEST_DIR/a $TEST_DIR/b >> $seqres.full
 
 diff -uw $TEST_DIR/a $TEST_DIR/b
 
+# Do we have mappings for every sector on the device?
+ddev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.datablocks)
+rtdev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.rtblocks)
+fsblock_bytes=$(_xfs_statfs_field "$SCRATCH_MNT" geom.bsize)
+
+ddev_daddrs=$((ddev_fsblocks * fsblock_bytes / 512))
+rtdev_daddrs=$((rtdev_fsblocks * fsblock_bytes / 512))
+
+ddev_devno=$(stat -c '%t:%T' $SCRATCH_DEV)
+if [ "$USE_EXTERNAL" = "yes" ] && [ -n "$SCRATCH_RTDEV" ]; then
+	rtdev_devno=$(stat -c '%t:%T' $SCRATCH_RTDEV)
+fi
+
+$XFS_IO_PROG -c 'fsmap -m -n 65536' $SCRATCH_MNT | awk -F ',' \
+	-v data_devno=$ddev_devno \
+	-v rt_devno=$rtdev_devno \
+	-v data_daddrs=$ddev_daddrs \
+	-v rt_daddrs=$rtdev_daddrs \
+'BEGIN {
+	next_daddr[data_devno] = 0;
+	next_daddr[rt_devno] = 0;
+}
+{
+	if ($1 == "EXT")
+		next
+	devno = sprintf("%x:%x", $2, $3);
+	if (devno != data_devno && devno != rt_devno)
+		next
+
+	if (next_daddr[devno] < $4)
+		printf("%sh: expected daddr %d, saw \"%s\"\n", devno,
+				next_daddr[devno], $0);
+		next = $5 + 1;
+		if (next > next_daddr[devno])
+		       next_daddr[devno] = next;
+}
+END {
+	if (data_daddrs != next_daddr[data_devno])
+		printf("%sh: fsmap stops at %d, expected %d\n",
+				data_devno, next_daddr[data_devno], data_daddrs);
+	if (rt_devno != "" && rt_daddrs != next_daddr[rt_devno])
+		printf("%sh: fsmap stops at %d, expected %d\n",
+				rt_devno, next_daddr[rt_devno], rt_daddrs);
+}'
+
 # success, all done
 status=0
 exit

Re: [PATCH] xfs/273: check thoroughness of the fsmappings

[Zorro Lang]

On Fri, Nov 08, 2024 at 09:41:46AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Enhance this test to make sure that there are no gaps in the fsmap
> records, and (especially) that they we report all the way to the end of
> the device.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/273 |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/tests/xfs/273 b/tests/xfs/273
> index d7fb80c4033429..ecfe5e7760a092 100755
> --- a/tests/xfs/273
> +++ b/tests/xfs/273
> @@ -24,6 +24,8 @@ _require_scratch
>  _require_populate_commands
>  _require_xfs_io_command "fsmap"
>  
> +_fixed_by_git_commit kernel XXXXXXXXXXXXXX "xfs: fix off-by-one error in fsmap's end_daddr usage"

The _fixed_by_kernel_commit can replace the "_fixed_by_git_commit kernel".

> +
>  rm -f "$seqres.full"
>  
>  echo "Format and mount"
> @@ -37,6 +39,51 @@ cat $TEST_DIR/a $TEST_DIR/b >> $seqres.full
>  
>  diff -uw $TEST_DIR/a $TEST_DIR/b
>  
> +# Do we have mappings for every sector on the device?
> +ddev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.datablocks)
> +rtdev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.rtblocks)
> +fsblock_bytes=$(_xfs_statfs_field "$SCRATCH_MNT" geom.bsize)
> +
> +ddev_daddrs=$((ddev_fsblocks * fsblock_bytes / 512))
> +rtdev_daddrs=$((rtdev_fsblocks * fsblock_bytes / 512))
> +
> +ddev_devno=$(stat -c '%t:%T' $SCRATCH_DEV)
> +if [ "$USE_EXTERNAL" = "yes" ] && [ -n "$SCRATCH_RTDEV" ]; then
> +	rtdev_devno=$(stat -c '%t:%T' $SCRATCH_RTDEV)
> +fi
> +
> +$XFS_IO_PROG -c 'fsmap -m -n 65536' $SCRATCH_MNT | awk -F ',' \
> +	-v data_devno=$ddev_devno \
> +	-v rt_devno=$rtdev_devno \
> +	-v data_daddrs=$ddev_daddrs \
> +	-v rt_daddrs=$rtdev_daddrs \
> +'BEGIN {
> +	next_daddr[data_devno] = 0;
> +	next_daddr[rt_devno] = 0;
> +}
> +{
> +	if ($1 == "EXT")
> +		next
> +	devno = sprintf("%x:%x", $2, $3);
> +	if (devno != data_devno && devno != rt_devno)
> +		next
> +
> +	if (next_daddr[devno] < $4)
> +		printf("%sh: expected daddr %d, saw \"%s\"\n", devno,
> +				next_daddr[devno], $0);
> +		next = $5 + 1;

Ahaha, awk expert Darrick :) I tried this patch, but got below error when
I tried this patch:

  +awk: cmd. line:15:             next = $5 + 1;
  +awk: cmd. line:15:                  ^ syntax error

Thanks,
Zorro

> +		if (next > next_daddr[devno])
> +		       next_daddr[devno] = next;
> +}
> +END {
> +	if (data_daddrs != next_daddr[data_devno])
> +		printf("%sh: fsmap stops at %d, expected %d\n",
> +				data_devno, next_daddr[data_devno], data_daddrs);
> +	if (rt_devno != "" && rt_daddrs != next_daddr[rt_devno])
> +		printf("%sh: fsmap stops at %d, expected %d\n",
> +				rt_devno, next_daddr[rt_devno], rt_daddrs);
> +}'
> +
>  # success, all done
>  status=0
>  exit
> 

Re: [PATCH] xfs/273: check thoroughness of the fsmappings

[Darrick J. Wong]

On Sat, Nov 09, 2024 at 10:45:16PM +0800, Zorro Lang wrote:
> On Fri, Nov 08, 2024 at 09:41:46AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Enhance this test to make sure that there are no gaps in the fsmap
> > records, and (especially) that they we report all the way to the end of
> > the device.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/273 |   47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/tests/xfs/273 b/tests/xfs/273
> > index d7fb80c4033429..ecfe5e7760a092 100755
> > --- a/tests/xfs/273
> > +++ b/tests/xfs/273
> > @@ -24,6 +24,8 @@ _require_scratch
> >  _require_populate_commands
> >  _require_xfs_io_command "fsmap"
> >  
> > +_fixed_by_git_commit kernel XXXXXXXXXXXXXX "xfs: fix off-by-one error in fsmap's end_daddr usage"
> 
> The _fixed_by_kernel_commit can replace the "_fixed_by_git_commit kernel".

<nod>

> > +
> >  rm -f "$seqres.full"
> >  
> >  echo "Format and mount"
> > @@ -37,6 +39,51 @@ cat $TEST_DIR/a $TEST_DIR/b >> $seqres.full
> >  
> >  diff -uw $TEST_DIR/a $TEST_DIR/b
> >  
> > +# Do we have mappings for every sector on the device?
> > +ddev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.datablocks)
> > +rtdev_fsblocks=$(_xfs_statfs_field "$SCRATCH_MNT" geom.rtblocks)
> > +fsblock_bytes=$(_xfs_statfs_field "$SCRATCH_MNT" geom.bsize)
> > +
> > +ddev_daddrs=$((ddev_fsblocks * fsblock_bytes / 512))
> > +rtdev_daddrs=$((rtdev_fsblocks * fsblock_bytes / 512))
> > +
> > +ddev_devno=$(stat -c '%t:%T' $SCRATCH_DEV)
> > +if [ "$USE_EXTERNAL" = "yes" ] && [ -n "$SCRATCH_RTDEV" ]; then
> > +	rtdev_devno=$(stat -c '%t:%T' $SCRATCH_RTDEV)
> > +fi
> > +
> > +$XFS_IO_PROG -c 'fsmap -m -n 65536' $SCRATCH_MNT | awk -F ',' \
> > +	-v data_devno=$ddev_devno \
> > +	-v rt_devno=$rtdev_devno \
> > +	-v data_daddrs=$ddev_daddrs \
> > +	-v rt_daddrs=$rtdev_daddrs \
> > +'BEGIN {
> > +	next_daddr[data_devno] = 0;
> > +	next_daddr[rt_devno] = 0;
> > +}
> > +{
> > +	if ($1 == "EXT")
> > +		next
> > +	devno = sprintf("%x:%x", $2, $3);
> > +	if (devno != data_devno && devno != rt_devno)
> > +		next
> > +
> > +	if (next_daddr[devno] < $4)
> > +		printf("%sh: expected daddr %d, saw \"%s\"\n", devno,
> > +				next_daddr[devno], $0);
> > +		next = $5 + 1;
> 
> Ahaha, awk expert Darrick :) I tried this patch, but got below error when
> I tried this patch:
> 
>   +awk: cmd. line:15:             next = $5 + 1;
>   +awk: cmd. line:15:                  ^ syntax error

Aha, I forgot to commit the change renaming next to n before sending. :(

--D

> Thanks,
> Zorro
> 
> > +		if (next > next_daddr[devno])
> > +		       next_daddr[devno] = next;
> > +}
> > +END {
> > +	if (data_daddrs != next_daddr[data_devno])
> > +		printf("%sh: fsmap stops at %d, expected %d\n",
> > +				data_devno, next_daddr[data_devno], data_daddrs);
> > +	if (rt_devno != "" && rt_daddrs != next_daddr[rt_devno])
> > +		printf("%sh: fsmap stops at %d, expected %d\n",
> > +				rt_devno, next_daddr[rt_devno], rt_daddrs);
> > +}'
> > +
> >  # success, all done
> >  status=0
> >  exit
> > 
> 

Re: [PATCH] xfs: fix off-by-one error in fsmap's end_daddr usage

[Christoph Hellwig]

> @@ -737,8 +738,8 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
>  	 * we calculated from userspace's high key to synthesize the record.
>  	 * Note that if the btree query found a mapping, there won't be a gap.
>  	 */
> -	if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) {
> -		frec.start_daddr = info->end_daddr;
> +	if (info->last) {
> +		frec.start_daddr = info->end_daddr + 1;
>  	} else {
>  		frec.start_daddr = xfs_rtb_to_daddr(mp, start_rtb);
>  	}

Nit: no need for the braces here.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>