Re: [PATCH] xfs: fix exchange-range-to-eof file size exchange

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Oct 06, 2026 at 06:25:17PM +1100, Dave Chinner wrote:
> On Mon, Oct 05, 2026 at 10:09:14PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Norbert Szetei posted a patch containing an incomplete description of a
> > bug in XFS_IOC_EXCHANGE_RANGE's TO_EOF flag.  The bug tricks
> > exchange-range into modifying a file so that it has shared blocks
> > starting beyond EOF, which is never allowed because files never have
> > written data beyond EOF, and you can only share written data blocks.
> > This is key to all the incorrect behavior that follows.
> ....
> > 
> > Having confused the file sizes, the PoC uses a second exchange-range
> > call on what looks like a non-reflinked single-block file.  Because the
> > file size is set incorrectly, exchange-range thinks it's exchanging the
> > full contents of two files and clears the reflink flag on the broken
> > file.
> 
> Hmmm, another "exchrange reflink flag coherency" bug.
> 
> Aside from fixing this specific issue, how can we prevent
> other/future logic bugs from failing to set the reflink flag
> appropriately and so never expose such a bug to userspace again?
> 
> I'm thinking that we should not try to swap the reflink flag along
> with the extents. Instead, if one of the files had the reflink flag
> set before the swap, then we scan scan the extents of both files
> after the swap for shared extents and set the reflink flags for each
> file appropriately.  EXCHRANGE isn't really performance sensitive,
> so the cost of the refcount scans shouldn't be an issue.
> 
> Setting the flags this way means it doesn't matter what file shared
> extent(s) ends up in, the inode(s) that owns it(them) will always
> have the reflink flag set correctly. Hence there is no way to use
> EXCHRANGE creatively to expose this "didn't COW when it should have'
> class of corruption bugs ever again...

That is what Norbert's patch actually does.  If you think removing
xfs_exchmaps_clear_reflink and all the stuff that calls it is a good
idea (and it probably is!) then let's move that discussion and review to
that patch's thread.

> > That enables an extending write of the broken file to rewrite the
> > shared block that's just past EOF.  This has become known colloquially
> > as refluxfs.
> > 
> > Once the file sizes are set correctly, the second exchange-range no
> > longer thinks that it's doing a full-contents swap, so it won't clear
> > the reflink flag on either of its file arguments.
> > 
> > Link: https://lore.kernel.org/linux-xfs/1E196589-DEBE-40AC-AFEA-D420DAAB067F@doyensec.com/
> > Reported-by: Norbert Szetei <norbert@doyensec.com>
> > Cc: <stable@vger.kernel.org> # v6.10
> > Fixes: 966ceafc7a4371 ("xfs: create deferred log items for file mapping exchanges")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_exchmaps.c |    8 ++++++--
> >  fs/xfs/xfs_exchrange.c       |   10 ++++++----
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_exchmaps.c b/fs/xfs/libxfs/xfs_exchmaps.c
> > index 6a66b6075e0af4..c9d9464e9d39f5 100644
> > --- a/fs/xfs/libxfs/xfs_exchmaps.c
> > +++ b/fs/xfs/libxfs/xfs_exchmaps.c
> > @@ -987,6 +987,7 @@ xfs_exchmaps_init_intent(
> >  	const struct xfs_exchmaps_req	*req)
> >  {
> >  	struct xfs_exchmaps_intent	*xmi;
> > +	struct xfs_mount		*mp = req->ip1->i_mount;
> >  	unsigned int			rs = 0;
> >  
> >  	xmi = kmem_cache_zalloc(xfs_exchmaps_intent_cache,
> > @@ -1006,9 +1007,12 @@ xfs_exchmaps_init_intent(
> >  	}
> >  
> >  	if (req->flags & XFS_EXCHMAPS_SET_SIZES) {
> > +		loff_t	off1 = XFS_FSB_TO_B(mp, xmi->xmi_startoff1);
> > +		loff_t	off2 = XFS_FSB_TO_B(mp, xmi->xmi_startoff2);
> > +
> >  		xmi->xmi_flags |= XFS_EXCHMAPS_SET_SIZES;
> > -		xmi->xmi_isize1 = req->ip2->i_disk_size;
> > -		xmi->xmi_isize2 = req->ip1->i_disk_size;
> > +		xmi->xmi_isize1 = off1 + (req->ip2->i_disk_size - off2);
> > +		xmi->xmi_isize2 = off2 + (req->ip1->i_disk_size - off1);
> >  	}
> 
> I think this really needs a comment to explain why the calculation
> is structured this way. I think it is trying to set the file sizes
> to the "destination offset" + "swap length from other file" because
> the ranges in each file might be different lengths, but I could be
> wrong...

That's correct.  I'll add that as a code comment.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> dgc@kernel.org
>