| Message ID | 162872992222.1220643.2988115020171417694.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: fix various bugs in fsmap | expand |
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?
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
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);
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);