Re: [PATCH] xfs: fix spurious len double-counting in rmap three-way merge check

[Carlos Maiolino <cem@kernel.org>]

On Wed, Nov 04, 2026 at 11:27:54AM +0800, Jianyun Gao wrote:
> Hi Carlos,
> 
> Thanks for the review. To answer your questions:
> 
> How I found it: I found this while reviewing the merge logic in
> xfs_rmap_map. After tracing the left-edge merge path, I noticed that
> ltrec.rm_blockcount is already updated at line 1119
> (ltrec.rm_blockcount += len), so adding len again in the overflow check
> is redundant. When the real merged length is still within
> XFS_RMAP_LEN_MAX but the inflated value exceeds it, the three-way merge
> is incorrectly rejected, leaving the right record unmerged.
> 
> Testing: I ran the rmap-specific regression tests with this patch
> applied, using kvm-xfstests on a 4k-block XFS configuration with
> rmapbt=1 and reflink=1:
> 
>   kvm-xfstests -c xfs/4k xfs/236 xfs/578
> 
> Results:
>   xfs/236      Pass     6s
>   xfs/578      Pass     34s
> Totals: 2 tests, 0 skipped, 0 failures, 0 errors
> 
> xfs/236 directly exercises the rmap btree grow/shrink paths affected by
> this change. I did not observe any regressions or warnings.
> If there are specific test cases you'd like me to run, please let me know.

Ok, I'd expect the tests to be a bit more thorough than two test cases,
ie. I'd expect this to be ran against at least the auto group too. But
nevertheless I'm glad you at did at least some of the homework.

The patch also seems fine to me, so

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> Thanks,
> Jianyun
> 
> On Tue, Nov 3, 2026 at 11:33 PM Carlos Maiolino <cem@kernel.org> wrote:
> >
> > On Tue, Nov 03, 2026 at 10:41:44PM +0800, jianyungao89@gmail.com wrote:
> > > From: Jianyun Gao <jianyungao89@gmail.com>
> > >
> > > In xfs_rmap_map, the left-edge merge path already executes
> > > ltrec.rm_blockcount += len at line 1119 before checking whether the
> > > right record is also contiguous and can be merged.  The subsequent
> > > overflow guard therefore must compare the already-incremented
> > > ltrec.rm_blockcount plus gtrec.rm_blockcount against XFS_RMAP_LEN_MAX,
> > > but it incorrectly adds len a second time:
> > >
> > > (unsigned long)ltrec.rm_blockcount + len +
> > >    gtrec.rm_blockcount <= XFS_RMAP_LEN_MAX
> > >
> > > The extra len makes the check too strict: when the real merged length
> > > does not overflow but the inflated value exceeds XFS_RMAP_LEN_MAX, the
> > > three-way merge is rejected and the code falls back to a two-way merge,
> > > leaving the right record unmerged.  This produces rmap btree
> > > fragmentation but does not corrupt on-disk metadata.
> > >
> > > Remove the spurious len from the overflow check.
> > >
> > > Fixes: 0a1b0b3855cf7 ("xfs: add an extent to the rmap btree")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jianyun Gao <jianyungao89@gmail.com>
> >
> > Hi, even though this looks reasonably correct, how did you find this
> > issue?
> > Did you test this at all against any regressions (i.e. running it
> > against xfstests)? This is touching critical logic on the rmap trees
> > which can potentially end really bad.
> >
> > Cheers.
> >
> > > ---
> > >  fs/xfs/libxfs/xfs_rmap.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > > index 14aef87837a9..c2b2f8e7502d 100644
> > > --- a/fs/xfs/libxfs/xfs_rmap.c
> > > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > > @@ -1120,7 +1120,7 @@ xfs_rmap_map(
> > >               if (have_gt &&
> > >                   bno + len == gtrec.rm_startblock &&
> > >                   (ignore_off || offset + len == gtrec.rm_offset) &&
> > > -                 (unsigned long)ltrec.rm_blockcount + len +
> > > +                 (unsigned long)ltrec.rm_blockcount +
> > >                               gtrec.rm_blockcount <= XFS_RMAP_LEN_MAX) {
> > >                       /*
> > >                        * right edge also contiguous, delete right record
> > > --
> > > 2.34.1
> > >
> > >
>