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

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

On Tue, Oct 06, 2026 at 10:12:17PM +0200, Norbert Szetei wrote:
> > On Oct 6, 2026, at 17:00, Darrick J. Wong <djwong@kernel.org> wrote:
> > 
> > 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.
> 
> I realized the patch I proposed is not complete on its own either.
> XFS_IOC_SWAPEXT swaps the reflink flags as well, in xfs_swap_extents()
> (xfs_bmap_util.c:1704), which my patch does not touch. The test guarding it
> 
>         /* Verify all data are being swapped */
>         if (sxp->sx_offset != 0 ||
>             sxp->sx_length != ip->i_disk_size ||
>             sxp->sx_length != tip->i_disk_size) {
> 
> only proves the requested length matches i_disk_size, and i_disk_size does
> not bound the inode's mappings, so xfs_swap_extent_rmap() leaves mappings
> behind and the flags are traded anyway.

<nod> Could you repost your original patch but with this chunk removed
as well, please?  I no longer think that cross referencing every data
fork mapping with the refcount data on every exchrange is worth the
trouble.

--D

> N.
> 
> > 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
> 
>