On Monday 9 November 2020 11:47:39 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Fix some serious WTF in the reference count scrubber's rmap fragment
> processing. The code comment says that this loop is supposed to move
> all fragment records starting at or before bno onto the worklist, but
> there's no obvious reason why nr (the number of items added) should
> increment starting from 1, and breaking the loop when we've added the
> target number seems dubious since we could have more rmap fragments that
> should have been added to the worklist.
>
> This seems to manifest in xfs/411 when adding one to the refcount field.
>
> Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/refcount.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> index beaeb6fa3119..dd672e6bbc75 100644
> --- a/fs/xfs/scrub/refcount.c
> +++ b/fs/xfs/scrub/refcount.c
> @@ -170,7 +170,6 @@ xchk_refcountbt_process_rmap_fragments(
> */
> INIT_LIST_HEAD(&worklist);
> rbno = NULLAGBLOCK;
> - nr = 1;
>
> /* Make sure the fragments actually /are/ in agbno order. */
> bno = 0;
> @@ -184,15 +183,14 @@ xchk_refcountbt_process_rmap_fragments(
> * Find all the rmaps that start at or before the refc extent,
> * and put them on the worklist.
> */
> + nr = 0;
> list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> - if (frag->rm.rm_startblock > refchk->bno)
> - goto done;
> + if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> + break;
In the case of fuzzed refcnt value of 1, The condition "nr > target_nr" causes
"nr != target_nr" condition (appearing after the loop) to evaluate to true
(since atleast two rmap entries would be present for the refcount extent)
which in turn causes xchk_refcountbt_xref_rmap() to flag the data structure as
corrupt. Please let me know if my understanding of the code flow is correct?
> bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> if (bno < rbno)
> rbno = bno;
> list_move_tail(&frag->list, &worklist);
> - if (nr == target_nr)
> - break;
> nr++;
> }
>
>
>
--
chandan
