On Tuesday 13 October 2020 10:28:53 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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
