[PATCHSET 0/3] xfs: fix various bugs in fsmap

[PATCHSET 0/3] xfs: fix various bugs in fsmap

[Darrick J. Wong]

Hi all,

While performing some code audits of the fsmap code, I noticed some off
by one errors in the realtime bitmap query code that provides the rt
fsmap implementation.

The first problem I found is that while the rtbitmap range query
function will constrain the starting and ending rtextent parameters to
match the actual rt volume, it does so by changing the struct passed in
by the caller.  This is a no-no, since query functions themselves are
not supposed to change the global state.

The second problem is an off-by-one error in the rtbitmap fsmap function
itself.  The fsmap record emitter function has the neat property that it
can detect gaps between the fsmap record we're about to emit and the
last one it emitted.  When this happens, it first emits an fsmap record
to fill the gap and then emits the one it was called about.

When the last block of the queried range is in use, we synthesize a
fsmap record just outside the query range, which has the effect of
emitting an "unknown owner" fsmap record for the inuse space.
Unfortunately, we don't range check the last block value, with the
result that the "unknown owner" fsmap can claim to extend beyond the end
of the rt volume!  So range check that.

The third problem is similar to the first: each fsmap backend is passed
the keys of the range to query and some scratch space.  The backend can
change anything it wants in the scratch space, but it's not supposed to
change the keys.  Unfortunately, range checking in the backend functions
/did/ change the keys, which causes subsequent backends to be called
with incorrect keys.  Fix this.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsmap-fixes-5.15
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   14 ++++++-----
 fs/xfs/xfs_fsmap.c           |   52 ++++++++++++++++++++++++------------------
 fs/xfs/xfs_rtalloc.h         |    7 ++----
 3 files changed, 40 insertions(+), 33 deletions(-)

[PATCH 1/3] xfs: make xfs_rtalloc_query_range input parameters const

[Darrick J. Wong]

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

In commit 8ad560d2565e, we changed xfs_rtalloc_query_range to constrain
the range of bits in the realtime bitmap file that would actually be
searched.  In commit a3a374bf1889, we changed the range again
(incorrectly), leading to the fix in commit d88850bd5516, which finally
corrected the range check code.  Unfortunately, the author never noticed
that the function modifies its input parameters, which is a totaly no-no
since none of the other range query functions change their input
parameters.

So, fix this function yet again to stash the upper end of the query
range (i.e. the high key) in a local variable and hope this is the last
time I have to fix my own function.  While we're at it, mark the key
inputs const so nobody makes this mistake again. :(

Fixes: 8ad560d2565e ("xfs: strengthen rtalloc query range checks")
Not-fixed-by: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
Not-fixed-by: d88850bd5516 ("xfs: fix high key handling in the rt allocator's query_range function")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   14 +++++++-------
 fs/xfs/xfs_rtalloc.h         |    7 +++----
 2 files changed, 10 insertions(+), 11 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 483375c6a735..5740ba664867 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1009,8 +1009,8 @@ xfs_rtfree_extent(
 int
 xfs_rtalloc_query_range(
 	struct xfs_trans		*tp,
-	struct xfs_rtalloc_rec		*low_rec,
-	struct xfs_rtalloc_rec		*high_rec,
+	const struct xfs_rtalloc_rec	*low_rec,
+	const struct xfs_rtalloc_rec	*high_rec,
 	xfs_rtalloc_query_range_fn	fn,
 	void				*priv)
 {
@@ -1018,6 +1018,7 @@ xfs_rtalloc_query_range(
 	struct xfs_mount		*mp = tp->t_mountp;
 	xfs_rtblock_t			rtstart;
 	xfs_rtblock_t			rtend;
+	xfs_rtblock_t			high_key;
 	int				is_free;
 	int				error = 0;
 
@@ -1026,12 +1027,12 @@ xfs_rtalloc_query_range(
 	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
 	    low_rec->ar_startext == high_rec->ar_startext)
 		return 0;
-	high_rec->ar_startext = min(high_rec->ar_startext,
-			mp->m_sb.sb_rextents - 1);
+
+	high_key = min(high_rec->ar_startext, mp->m_sb.sb_rextents - 1);
 
 	/* Iterate the bitmap, looking for discrepancies. */
 	rtstart = low_rec->ar_startext;
-	while (rtstart <= high_rec->ar_startext) {
+	while (rtstart <= high_key) {
 		/* Is the first block free? */
 		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
 				&is_free);
@@ -1039,8 +1040,7 @@ xfs_rtalloc_query_range(
 			break;
 
 		/* How long does the extent go for? */
-		error = xfs_rtfind_forw(mp, tp, rtstart,
-				high_rec->ar_startext, &rtend);
+		error = xfs_rtfind_forw(mp, tp, rtstart, high_key, &rtend);
 		if (error)
 			break;
 
diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
index ed885620589c..51097cb24311 100644
--- a/fs/xfs/xfs_rtalloc.h
+++ b/fs/xfs/xfs_rtalloc.h
@@ -124,10 +124,9 @@ int xfs_rtfree_range(struct xfs_mount *mp, struct xfs_trans *tp,
 		     xfs_rtblock_t start, xfs_extlen_t len,
 		     struct xfs_buf **rbpp, xfs_fsblock_t *rsb);
 int xfs_rtalloc_query_range(struct xfs_trans *tp,
-			    struct xfs_rtalloc_rec *low_rec,
-			    struct xfs_rtalloc_rec *high_rec,
-			    xfs_rtalloc_query_range_fn fn,
-			    void *priv);
+		const struct xfs_rtalloc_rec *low_rec,
+		const struct xfs_rtalloc_rec *high_rec,
+		xfs_rtalloc_query_range_fn fn, void *priv);
 int xfs_rtalloc_query_all(struct xfs_trans *tp,
 			  xfs_rtalloc_query_range_fn fn,
 			  void *priv);

Re: [PATCH 1/3] xfs: make xfs_rtalloc_query_range input parameters const

[Christoph Hellwig]

On Wed, Aug 11, 2021 at 05:58:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In commit 8ad560d2565e, we changed xfs_rtalloc_query_range to constrain
> the range of bits in the realtime bitmap file that would actually be
> searched.  In commit a3a374bf1889, we changed the range again
> (incorrectly), leading to the fix in commit d88850bd5516, which finally
> corrected the range check code.  Unfortunately, the author never noticed
> that the function modifies its input parameters, which is a totaly no-no
> since none of the other range query functions change their input
> parameters.
> 
> So, fix this function yet again to stash the upper end of the query
> range (i.e. the high key) in a local variable and hope this is the last
> time I have to fix my own function.  While we're at it, mark the key
> inputs const so nobody makes this mistake again. :(

Heh.

Looks good:

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

> Not-fixed-by: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
> Not-fixed-by: d88850bd5516 ("xfs: fix high key handling in the rt allocator's query_range function")

Are these tags a thing now or is this just a grumpy Darrick?

Re: [PATCH 1/3] xfs: make xfs_rtalloc_query_range input parameters const

[Darrick J. Wong]

On Thu, Aug 12, 2021 at 09:30:33AM +0100, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 05:58:42PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > In commit 8ad560d2565e, we changed xfs_rtalloc_query_range to constrain
> > the range of bits in the realtime bitmap file that would actually be
> > searched.  In commit a3a374bf1889, we changed the range again
> > (incorrectly), leading to the fix in commit d88850bd5516, which finally
> > corrected the range check code.  Unfortunately, the author never noticed
> > that the function modifies its input parameters, which is a totaly no-no
> > since none of the other range query functions change their input
> > parameters.
> > 
> > So, fix this function yet again to stash the upper end of the query
> > range (i.e. the high key) in a local variable and hope this is the last
> > time I have to fix my own function.  While we're at it, mark the key
> > inputs const so nobody makes this mistake again. :(
> 
> Heh.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> > Not-fixed-by: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
> > Not-fixed-by: d88850bd5516 ("xfs: fix high key handling in the rt allocator's query_range function")
> 
> Are these tags a thing now or is this just a grumpy Darrick?

Just grumpy me.  If you click on the commits in gitk you can see the
long stream of me trying to wrap his head around the weirdness of
rtbitmap. :/

Thanks for the review though. :)

--D

Re: [PATCH 1/3] xfs: make xfs_rtalloc_query_range input parameters const

[Chandan Babu R]

On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In commit 8ad560d2565e, we changed xfs_rtalloc_query_range to constrain
> the range of bits in the realtime bitmap file that would actually be
> searched.  In commit a3a374bf1889, we changed the range again
> (incorrectly), leading to the fix in commit d88850bd5516, which finally
> corrected the range check code.  Unfortunately, the author never noticed
> that the function modifies its input parameters, which is a totaly no-no
> since none of the other range query functions change their input
> parameters.
>
> So, fix this function yet again to stash the upper end of the query
> range (i.e. the high key) in a local variable and hope this is the last
> time I have to fix my own function.  While we're at it, mark the key
> inputs const so nobody makes this mistake again. :(
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Fixes: 8ad560d2565e ("xfs: strengthen rtalloc query range checks")
> Not-fixed-by: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
> Not-fixed-by: d88850bd5516 ("xfs: fix high key handling in the rt allocator's query_range function")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |   14 +++++++-------
>  fs/xfs/xfs_rtalloc.h         |    7 +++----
>  2 files changed, 10 insertions(+), 11 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 483375c6a735..5740ba664867 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1009,8 +1009,8 @@ xfs_rtfree_extent(
>  int
>  xfs_rtalloc_query_range(
>  	struct xfs_trans		*tp,
> -	struct xfs_rtalloc_rec		*low_rec,
> -	struct xfs_rtalloc_rec		*high_rec,
> +	const struct xfs_rtalloc_rec	*low_rec,
> +	const struct xfs_rtalloc_rec	*high_rec,
>  	xfs_rtalloc_query_range_fn	fn,
>  	void				*priv)
>  {
> @@ -1018,6 +1018,7 @@ xfs_rtalloc_query_range(
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	xfs_rtblock_t			rtstart;
>  	xfs_rtblock_t			rtend;
> +	xfs_rtblock_t			high_key;
>  	int				is_free;
>  	int				error = 0;
>  
> @@ -1026,12 +1027,12 @@ xfs_rtalloc_query_range(
>  	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
>  	    low_rec->ar_startext == high_rec->ar_startext)
>  		return 0;
> -	high_rec->ar_startext = min(high_rec->ar_startext,
> -			mp->m_sb.sb_rextents - 1);
> +
> +	high_key = min(high_rec->ar_startext, mp->m_sb.sb_rextents - 1);
>  
>  	/* Iterate the bitmap, looking for discrepancies. */
>  	rtstart = low_rec->ar_startext;
> -	while (rtstart <= high_rec->ar_startext) {
> +	while (rtstart <= high_key) {
>  		/* Is the first block free? */
>  		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
>  				&is_free);
> @@ -1039,8 +1040,7 @@ xfs_rtalloc_query_range(
>  			break;
>  
>  		/* How long does the extent go for? */
> -		error = xfs_rtfind_forw(mp, tp, rtstart,
> -				high_rec->ar_startext, &rtend);
> +		error = xfs_rtfind_forw(mp, tp, rtstart, high_key, &rtend);
>  		if (error)
>  			break;
>  
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index ed885620589c..51097cb24311 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -124,10 +124,9 @@ int xfs_rtfree_range(struct xfs_mount *mp, struct xfs_trans *tp,
>  		     xfs_rtblock_t start, xfs_extlen_t len,
>  		     struct xfs_buf **rbpp, xfs_fsblock_t *rsb);
>  int xfs_rtalloc_query_range(struct xfs_trans *tp,
> -			    struct xfs_rtalloc_rec *low_rec,
> -			    struct xfs_rtalloc_rec *high_rec,
> -			    xfs_rtalloc_query_range_fn fn,
> -			    void *priv);
> +		const struct xfs_rtalloc_rec *low_rec,
> +		const struct xfs_rtalloc_rec *high_rec,
> +		xfs_rtalloc_query_range_fn fn, void *priv);
>  int xfs_rtalloc_query_all(struct xfs_trans *tp,
>  			  xfs_rtalloc_query_range_fn fn,
>  			  void *priv);


-- 
chandan

[PATCH 2/3] xfs: fix off-by-one error when the last rt extent is in use

[Darrick J. Wong]

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

The fsmap implementation for realtime devices uses the gap between
info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range
to feed userspace fsmap records with an "unknown" owner.  We use this
trick to report to userspace when the last rtextent in the filesystem is
in use by synthesizing a null rmap record starting at the next block
after the query range.

Unfortunately, there's a minor accounting bug in the way that we
construct the null rmap record.  Originally, ahigh.ar_startext contains
the last rtextent for which the user wants records.  It's entirely
possible that number is beyond the end of the rt volume, so the location
synthesized rmap record /must/ be constrained to the minimum of the high
key and the number of extents in the rt volume.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_fsmap.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 7d0b09c1366e..a0e8ab58124b 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 {
 	struct xfs_rtalloc_rec		alow = { 0 };
 	struct xfs_rtalloc_rec		ahigh = { 0 };
+	struct xfs_mount		*mp = tp->t_mountp;
 	int				error;
 
-	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
+	xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED);
 
+	/*
+	 * Set up query parameters to return free extents covering the range we
+	 * want.
+	 */
 	alow.ar_startext = info->low.rm_startblock;
+	do_div(alow.ar_startext, mp->m_sb.sb_rextsize);
+
 	ahigh.ar_startext = info->high.rm_startblock;
-	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
-	if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize))
+	if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize))
 		ahigh.ar_startext++;
+
 	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
 			xfs_getfsmap_rtdev_rtbitmap_helper, info);
 	if (error)
 		goto err;
 
-	/* Report any gaps at the end of the rtbitmap */
+	/*
+	 * Report any gaps at the end of the rtbitmap by simulating a null
+	 * rmap starting at the block after the end of the query range.
+	 */
 	info->last = true;
+	ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext);
+
 	error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info);
 	if (error)
 		goto err;
 err:
-	xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
+	xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED);
 	return error;
 }
 

Re: [PATCH 2/3] xfs: fix off-by-one error when the last rt extent is in use

[Christoph Hellwig]

On Wed, Aug 11, 2021 at 05:58:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The fsmap implementation for realtime devices uses the gap between
> info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range
> to feed userspace fsmap records with an "unknown" owner.  We use this
> trick to report to userspace when the last rtextent in the filesystem is
> in use by synthesizing a null rmap record starting at the next block
> after the query range.
> 
> Unfortunately, there's a minor accounting bug in the way that we
> construct the null rmap record.  Originally, ahigh.ar_startext contains
> the last rtextent for which the user wants records.  It's entirely
> possible that number is beyond the end of the rt volume, so the location
> synthesized rmap record /must/ be constrained to the minimum of the high
> key and the number of extents in the rt volume.

Looks good, although the change is a little hard to follow due to the
big amount of cleanups vs the tiny actual bug fix.

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

Re: [PATCH 2/3] xfs: fix off-by-one error when the last rt extent is in use

[Darrick J. Wong]

On Thu, Aug 12, 2021 at 09:36:52AM +0100, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 05:58:47PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The fsmap implementation for realtime devices uses the gap between
> > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range
> > to feed userspace fsmap records with an "unknown" owner.  We use this
> > trick to report to userspace when the last rtextent in the filesystem is
> > in use by synthesizing a null rmap record starting at the next block
> > after the query range.
> > 
> > Unfortunately, there's a minor accounting bug in the way that we
> > construct the null rmap record.  Originally, ahigh.ar_startext contains
> > the last rtextent for which the user wants records.  It's entirely
> > possible that number is beyond the end of the rt volume, so the location
> > synthesized rmap record /must/ be constrained to the minimum of the high
> > key and the number of extents in the rt volume.
> 
> Looks good, although the change is a little hard to follow due to the
> big amount of cleanups vs the tiny actual bug fix.

I'll clean that up a bit before I commit this to the -merge branch.

--D

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

Re: [PATCH 2/3] xfs: fix off-by-one error when the last rt extent is in use

[Chandan Babu R]

On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The fsmap implementation for realtime devices uses the gap between
> info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range
> to feed userspace fsmap records with an "unknown" owner.  We use this
> trick to report to userspace when the last rtextent in the filesystem is
> in use by synthesizing a null rmap record starting at the next block
> after the query range.
>
> Unfortunately, there's a minor accounting bug in the way that we
> construct the null rmap record.  Originally, ahigh.ar_startext contains
> the last rtextent for which the user wants records.  It's entirely
> possible that number is beyond the end of the rt volume, so the location
> synthesized rmap record /must/ be constrained to the minimum of the high
> key and the number of extents in the rt volume.
>

When the number of blocks on the realtime device is not an integral multiple
of xfs_sb->sb_rextsize, ahigh.ar_startext can contain a value which is one
more than the highest valid rtextent. Hence, without this patch, the last
record reported to the userpace might contain an invalid upper bound. Assuming
that my understanding is indeed correct,

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsmap.c |   22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 7d0b09c1366e..a0e8ab58124b 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>  {
>  	struct xfs_rtalloc_rec		alow = { 0 };
>  	struct xfs_rtalloc_rec		ahigh = { 0 };
> +	struct xfs_mount		*mp = tp->t_mountp;
>  	int				error;
>  
> -	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
> +	xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED);
>  
> +	/*
> +	 * Set up query parameters to return free extents covering the range we
> +	 * want.
> +	 */
>  	alow.ar_startext = info->low.rm_startblock;
> +	do_div(alow.ar_startext, mp->m_sb.sb_rextsize);
> +
>  	ahigh.ar_startext = info->high.rm_startblock;
> -	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
> -	if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize))
> +	if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize))
>  		ahigh.ar_startext++;
> +
>  	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
>  			xfs_getfsmap_rtdev_rtbitmap_helper, info);
>  	if (error)
>  		goto err;
>  
> -	/* Report any gaps at the end of the rtbitmap */
> +	/*
> +	 * Report any gaps at the end of the rtbitmap by simulating a null
> +	 * rmap starting at the block after the end of the query range.
> +	 */
>  	info->last = true;
> +	ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext);
> +
>  	error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info);
>  	if (error)
>  		goto err;
>  err:
> -	xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
> +	xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED);
>  	return error;
>  }
>  


-- 
chandan

Re: [PATCH 2/3] xfs: fix off-by-one error when the last rt extent is in use

[Darrick J. Wong]

On Fri, Aug 13, 2021 at 04:20:34PM +0530, Chandan Babu R wrote:
> On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > The fsmap implementation for realtime devices uses the gap between
> > info->next_daddr and a free rtextent reported by xfs_rtalloc_query_range
> > to feed userspace fsmap records with an "unknown" owner.  We use this
> > trick to report to userspace when the last rtextent in the filesystem is
> > in use by synthesizing a null rmap record starting at the next block
> > after the query range.
> >
> > Unfortunately, there's a minor accounting bug in the way that we
> > construct the null rmap record.  Originally, ahigh.ar_startext contains
> > the last rtextent for which the user wants records.  It's entirely
> > possible that number is beyond the end of the rt volume, so the location
> > synthesized rmap record /must/ be constrained to the minimum of the high
> > key and the number of extents in the rt volume.
> >
> 
> When the number of blocks on the realtime device is not an integral multiple
> of xfs_sb->sb_rextsize, ahigh.ar_startext can contain a value which is one
> more than the highest valid rtextent. Hence, without this patch, the last
> record reported to the userpace might contain an invalid upper bound. Assuming
> that my understanding is indeed correct,

Correct.  Thanks for reviewing this!

--D

> 
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
> 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_fsmap.c |   22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 7d0b09c1366e..a0e8ab58124b 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -523,27 +523,39 @@ xfs_getfsmap_rtdev_rtbitmap_query(
> >  {
> >  	struct xfs_rtalloc_rec		alow = { 0 };
> >  	struct xfs_rtalloc_rec		ahigh = { 0 };
> > +	struct xfs_mount		*mp = tp->t_mountp;
> >  	int				error;
> >  
> > -	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
> > +	xfs_ilock(mp->m_rbmip, XFS_ILOCK_SHARED);
> >  
> > +	/*
> > +	 * Set up query parameters to return free extents covering the range we
> > +	 * want.
> > +	 */
> >  	alow.ar_startext = info->low.rm_startblock;
> > +	do_div(alow.ar_startext, mp->m_sb.sb_rextsize);
> > +
> >  	ahigh.ar_startext = info->high.rm_startblock;
> > -	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
> > -	if (do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize))
> > +	if (do_div(ahigh.ar_startext, mp->m_sb.sb_rextsize))
> >  		ahigh.ar_startext++;
> > +
> >  	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
> >  			xfs_getfsmap_rtdev_rtbitmap_helper, info);
> >  	if (error)
> >  		goto err;
> >  
> > -	/* Report any gaps at the end of the rtbitmap */
> > +	/*
> > +	 * Report any gaps at the end of the rtbitmap by simulating a null
> > +	 * rmap starting at the block after the end of the query range.
> > +	 */
> >  	info->last = true;
> > +	ahigh.ar_startext = min(mp->m_sb.sb_rextents, ahigh.ar_startext);
> > +
> >  	error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info);
> >  	if (error)
> >  		goto err;
> >  err:
> > -	xfs_iunlock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
> > +	xfs_iunlock(mp->m_rbmip, XFS_ILOCK_SHARED);
> >  	return error;
> >  }
> >  
> 
> 
> -- 
> chandan

[PATCH 3/3] xfs: make fsmap backend function key parameters const

[Darrick J. Wong]

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

There are several GETFSMAP backend functions for XFS to cover the three
devices and various feature support.  Each of these functions are passed
pointers to the low and high keys for the dataset that userspace
requested, and a pointer to scratchpad variables that are used to
control the iteration and fill out records.  The scratchpad data can be
changed arbitrarily, but the keys are supposed to remain unchanged (and
under the control of the outermost loop in xfs_getfsmap).

Unfortunately, the data and rt backends modify the keys that are passed
in from the main control loop, which causes subsequent calls to return
incorrect query results.  Specifically, each of those two functions set
the block number in the high key to the size of their respective device.
Since fsmap results are sorted in device number order, if the lower
numbered device is smaller than the higher numbered device, the first
function will set the high key to the small size, and the key remains
unchanged as it is passed into the function for the higher numbered
device.  The second function will then fail to return all of the results
for the dataset that userspace is asking for because the keyspace is
incorrectly constrained.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_fsmap.c |   30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)


diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index a0e8ab58124b..7bcc2ab68b8d 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -61,7 +61,7 @@ xfs_fsmap_to_internal(
 static int
 xfs_fsmap_owner_to_rmap(
 	struct xfs_rmap_irec	*dest,
-	struct xfs_fsmap	*src)
+	const struct xfs_fsmap	*src)
 {
 	if (!(src->fmr_flags & FMR_OF_SPECIAL_OWNER)) {
 		dest->rm_owner = src->fmr_owner;
@@ -171,7 +171,7 @@ struct xfs_getfsmap_info {
 struct xfs_getfsmap_dev {
 	u32			dev;
 	int			(*fn)(struct xfs_trans *tp,
-				      struct xfs_fsmap *keys,
+				      const struct xfs_fsmap *keys,
 				      struct xfs_getfsmap_info *info);
 };
 
@@ -389,7 +389,7 @@ xfs_getfsmap_datadev_bnobt_helper(
 static void
 xfs_getfsmap_set_irec_flags(
 	struct xfs_rmap_irec	*irec,
-	struct xfs_fsmap	*fmr)
+	const struct xfs_fsmap	*fmr)
 {
 	irec->rm_flags = 0;
 	if (fmr->fmr_flags & FMR_OF_ATTR_FORK)
@@ -404,7 +404,7 @@ xfs_getfsmap_set_irec_flags(
 STATIC int
 xfs_getfsmap_logdev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
@@ -473,7 +473,7 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
 STATIC int
 __xfs_getfsmap_rtdev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	int				(*query_fn)(struct xfs_trans *,
 						    struct xfs_getfsmap_info *),
 	struct xfs_getfsmap_info	*info)
@@ -481,16 +481,14 @@ __xfs_getfsmap_rtdev(
 	struct xfs_mount		*mp = tp->t_mountp;
 	xfs_fsblock_t			start_fsb;
 	xfs_fsblock_t			end_fsb;
-	xfs_daddr_t			eofs;
+	uint64_t			eofs;
 	int				error = 0;
 
 	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
 	if (keys[0].fmr_physical >= eofs)
 		return 0;
-	if (keys[1].fmr_physical >= eofs)
-		keys[1].fmr_physical = eofs - 1;
 	start_fsb = XFS_BB_TO_FSBT(mp, keys[0].fmr_physical);
-	end_fsb = XFS_BB_TO_FSB(mp, keys[1].fmr_physical);
+	end_fsb = XFS_BB_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
 
 	/* Set up search keys */
 	info->low.rm_startblock = start_fsb;
@@ -563,7 +561,7 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 STATIC int
 xfs_getfsmap_rtdev_rtbitmap(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	info->missing_owner = XFS_FMR_OWN_UNKNOWN;
@@ -576,7 +574,7 @@ xfs_getfsmap_rtdev_rtbitmap(
 STATIC int
 __xfs_getfsmap_datadev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info,
 	int				(*query_fn)(struct xfs_trans *,
 						    struct xfs_getfsmap_info *,
@@ -591,16 +589,14 @@ __xfs_getfsmap_datadev(
 	xfs_fsblock_t			end_fsb;
 	xfs_agnumber_t			start_ag;
 	xfs_agnumber_t			end_ag;
-	xfs_daddr_t			eofs;
+	uint64_t			eofs;
 	int				error = 0;
 
 	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
 	if (keys[0].fmr_physical >= eofs)
 		return 0;
-	if (keys[1].fmr_physical >= eofs)
-		keys[1].fmr_physical = eofs - 1;
 	start_fsb = XFS_DADDR_TO_FSB(mp, keys[0].fmr_physical);
-	end_fsb = XFS_DADDR_TO_FSB(mp, keys[1].fmr_physical);
+	end_fsb = XFS_DADDR_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
 
 	/*
 	 * Convert the fsmap low/high keys to AG based keys.  Initialize
@@ -728,7 +724,7 @@ xfs_getfsmap_datadev_rmapbt_query(
 STATIC int
 xfs_getfsmap_datadev_rmapbt(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	info->missing_owner = XFS_FMR_OWN_FREE;
@@ -763,7 +759,7 @@ xfs_getfsmap_datadev_bnobt_query(
 STATIC int
 xfs_getfsmap_datadev_bnobt(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	struct xfs_alloc_rec_incore	akeys[2];

Re: [PATCH 3/3] xfs: make fsmap backend function key parameters const

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 3/3] xfs: make fsmap backend function key parameters const

[Chandan Babu R]

On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> There are several GETFSMAP backend functions for XFS to cover the three
> devices and various feature support.  Each of these functions are passed
> pointers to the low and high keys for the dataset that userspace
> requested, and a pointer to scratchpad variables that are used to
> control the iteration and fill out records.  The scratchpad data can be
> changed arbitrarily, but the keys are supposed to remain unchanged (and
> under the control of the outermost loop in xfs_getfsmap).
>
> Unfortunately, the data and rt backends modify the keys that are passed
> in from the main control loop, which causes subsequent calls to return
> incorrect query results.  Specifically, each of those two functions set
> the block number in the high key to the size of their respective device.
> Since fsmap results are sorted in device number order, if the lower
> numbered device is smaller than the higher numbered device, the first
> function will set the high key to the small size, and the key remains
> unchanged as it is passed into the function for the higher numbered
> device.  The second function will then fail to return all of the results
> for the dataset that userspace is asking for because the keyspace is
> incorrectly constrained.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsmap.c |   30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index a0e8ab58124b..7bcc2ab68b8d 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -61,7 +61,7 @@ xfs_fsmap_to_internal(
>  static int
>  xfs_fsmap_owner_to_rmap(
>  	struct xfs_rmap_irec	*dest,
> -	struct xfs_fsmap	*src)
> +	const struct xfs_fsmap	*src)
>  {
>  	if (!(src->fmr_flags & FMR_OF_SPECIAL_OWNER)) {
>  		dest->rm_owner = src->fmr_owner;
> @@ -171,7 +171,7 @@ struct xfs_getfsmap_info {
>  struct xfs_getfsmap_dev {
>  	u32			dev;
>  	int			(*fn)(struct xfs_trans *tp,
> -				      struct xfs_fsmap *keys,
> +				      const struct xfs_fsmap *keys,
>  				      struct xfs_getfsmap_info *info);
>  };
>  
> @@ -389,7 +389,7 @@ xfs_getfsmap_datadev_bnobt_helper(
>  static void
>  xfs_getfsmap_set_irec_flags(
>  	struct xfs_rmap_irec	*irec,
> -	struct xfs_fsmap	*fmr)
> +	const struct xfs_fsmap	*fmr)
>  {
>  	irec->rm_flags = 0;
>  	if (fmr->fmr_flags & FMR_OF_ATTR_FORK)
> @@ -404,7 +404,7 @@ xfs_getfsmap_set_irec_flags(
>  STATIC int
>  xfs_getfsmap_logdev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
> @@ -473,7 +473,7 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
>  STATIC int
>  __xfs_getfsmap_rtdev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	int				(*query_fn)(struct xfs_trans *,
>  						    struct xfs_getfsmap_info *),
>  	struct xfs_getfsmap_info	*info)
> @@ -481,16 +481,14 @@ __xfs_getfsmap_rtdev(
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	xfs_fsblock_t			start_fsb;
>  	xfs_fsblock_t			end_fsb;
> -	xfs_daddr_t			eofs;
> +	uint64_t			eofs;
>  	int				error = 0;
>  
>  	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
>  	if (keys[0].fmr_physical >= eofs)
>  		return 0;
> -	if (keys[1].fmr_physical >= eofs)
> -		keys[1].fmr_physical = eofs - 1;
>  	start_fsb = XFS_BB_TO_FSBT(mp, keys[0].fmr_physical);
> -	end_fsb = XFS_BB_TO_FSB(mp, keys[1].fmr_physical);
> +	end_fsb = XFS_BB_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
>  
>  	/* Set up search keys */
>  	info->low.rm_startblock = start_fsb;
> @@ -563,7 +561,7 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>  STATIC int
>  xfs_getfsmap_rtdev_rtbitmap(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	info->missing_owner = XFS_FMR_OWN_UNKNOWN;
> @@ -576,7 +574,7 @@ xfs_getfsmap_rtdev_rtbitmap(
>  STATIC int
>  __xfs_getfsmap_datadev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info,
>  	int				(*query_fn)(struct xfs_trans *,
>  						    struct xfs_getfsmap_info *,
> @@ -591,16 +589,14 @@ __xfs_getfsmap_datadev(
>  	xfs_fsblock_t			end_fsb;
>  	xfs_agnumber_t			start_ag;
>  	xfs_agnumber_t			end_ag;
> -	xfs_daddr_t			eofs;
> +	uint64_t			eofs;
>  	int				error = 0;
>  
>  	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
>  	if (keys[0].fmr_physical >= eofs)
>  		return 0;
> -	if (keys[1].fmr_physical >= eofs)
> -		keys[1].fmr_physical = eofs - 1;
>  	start_fsb = XFS_DADDR_TO_FSB(mp, keys[0].fmr_physical);
> -	end_fsb = XFS_DADDR_TO_FSB(mp, keys[1].fmr_physical);
> +	end_fsb = XFS_DADDR_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
>  
>  	/*
>  	 * Convert the fsmap low/high keys to AG based keys.  Initialize
> @@ -728,7 +724,7 @@ xfs_getfsmap_datadev_rmapbt_query(
>  STATIC int
>  xfs_getfsmap_datadev_rmapbt(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	info->missing_owner = XFS_FMR_OWN_FREE;
> @@ -763,7 +759,7 @@ xfs_getfsmap_datadev_bnobt_query(
>  STATIC int
>  xfs_getfsmap_datadev_bnobt(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	struct xfs_alloc_rec_incore	akeys[2];


-- 
chandan