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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Nov 14, 2024 at 04:33:49PM +0100, Christoph Hellwig wrote:
> 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.

Er... I'm not sure why you're sending this patch to Hans and stable but
not linux-xfs?  Also it's already on the list, though nobody's responded
to it yet:

https://lore.kernel.org/linux-xfs/20241108173907.GB168069@frogsfrogsfrogs/

--D

> 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 8d5d4d172d15..59b7a8e50414 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;
> @@ -1185,9 +1189,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++) {
> @@ -1200,17 +1201,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));
>  
> -- 
> 2.45.2
>