Re: [PATCH] xfs: fix high key handling in the rt allocator's query_range function

[Chandan Babu R <chandanrlinux@gmail.com>]

On Tuesday 13 October 2020 10:28:53 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix some off-by-one errors in xfs_rtalloc_query_range.  The highest key
> in the realtime bitmap is always one less than the number of rt extents,
> which means that the key clamp at the start of the function is wrong.
> The 4th argument to xfs_rtfind_forw is the highest rt extent that we
> want to probe, which means that passing 1 less than the high key is
> wrong.  Finally, drop the rem variable that controls the loop because we
> can compare the iteration point (rtstart) against the high key directly.
> 
> The sordid history of this function is that the original commit (fb3c3)
> incorrectly passed (high_rec->ar_startblock - 1) as the 'limit' parameter
> to xfs_rtfind_forw.  This was wrong because the "high key" is supposed
> to be the largest key for which the caller wants result rows, not the
> key for the first row that could possibly be outside the range that the
> caller wants to see.
> 
> A subsequent attempt (8ad56) to strengthen the parameter checking added
> incorrect clamping of the parameters to the number of rt blocks in the
> system (despite the bitmap functions all taking units of rt extents) to
> avoid querying ranges past the end of rt bitmap file but failed to fix
> the incorrect _rtfind_forw parameter.  The original _rtfind_forw
> parameter error then survived the conversion of the startblock and
> blockcount fields to rt extents (a0e5c), and the most recent off-by-one
> fix (a3a37) thought it was patching a problem when the end of the rt
> volume is not in use, but none of these fixes actually solved the
> original problem that the author was confused about the "limit" argument
> to xfs_rtfind_forw.
> 
> Sadly, all four of these patches were written by this author and even
> his own usage of this function and rt testing were inadequate to get
> this fixed quickly.
>

The high key being the minimum of (number of rtextents - 1, high key passed by
userspace) is correct.

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

> Original-problem: fb3c3de2f65c ("xfs: add a couple of queries to iterate free extents in the rtbitmap")
> Not-fixed-by: 8ad560d2565e ("xfs: strengthen rtalloc query range checks")
> Not-fixed-by: a0e5c435babd ("xfs: fix xfs_rtalloc_rec units")
> Fixes: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 1d9fa8a300f1..6c1aba16113c 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1018,7 +1018,6 @@ xfs_rtalloc_query_range(
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	xfs_rtblock_t			rtstart;
>  	xfs_rtblock_t			rtend;
> -	xfs_rtblock_t			rem;
>  	int				is_free;
>  	int				error = 0;
>  
> @@ -1027,13 +1026,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;
> -	if (high_rec->ar_startext > mp->m_sb.sb_rextents)
> -		high_rec->ar_startext = mp->m_sb.sb_rextents;
> +	high_rec->ar_startext = min(high_rec->ar_startext,
> +			mp->m_sb.sb_rextents - 1);
>  
>  	/* Iterate the bitmap, looking for discrepancies. */
>  	rtstart = low_rec->ar_startext;
> -	rem = high_rec->ar_startext - rtstart;
> -	while (rem) {
> +	while (rtstart <= high_rec->ar_startext) {
>  		/* Is the first block free? */
>  		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
>  				&is_free);
> @@ -1042,7 +1040,7 @@ xfs_rtalloc_query_range(
>  
>  		/* How long does the extent go for? */
>  		error = xfs_rtfind_forw(mp, tp, rtstart,
> -				high_rec->ar_startext - 1, &rtend);
> +				high_rec->ar_startext, &rtend);
>  		if (error)
>  			break;
>  
> @@ -1055,7 +1053,6 @@ xfs_rtalloc_query_range(
>  				break;
>  		}
>  
> -		rem -= rtend - rtstart + 1;
>  		rtstart = rtend + 1;
>  	}
>  
> 


-- 
chandan