Re: [PATCH v8 11/15] xfs: restart faults racing with realtime flag changes

[Christoph Hellwig <hch@lst.de>]

On Wed, Nov 11, 2026 at 08:02:40PM -0500, Chris Mason wrote:
> On a zoned XFS, a store through a shared mapping can dereference a NULL
> pointer, or corrupt the free block counter, if the realtime flag of the
> file changes during the write fault.
> 
> xfs_write_fault() reserves zoned space only for a zoned inode, and tests
> for that with no lock held. xfs_buffered_write_iomap_begin() tests again
> before it takes the ILOCK. FS_IOC_FSSETXATTR can change the flag of a
> file with no blocks between the two tests, since a fault never takes the
> IOLOCK:
> 
>     write fault                           FS_IOC_FSSETXATTR
>     ===========                           =================
>     xfs_write_fault()
>       sees that the inode is not zoned
>                                           sets the realtime flag
>     xfs_buffered_write_iomap_begin()
>       sees that the inode is zoned
>       /* ac is NULL */
> 
> If the flag is cleared instead, xfs_zoned_buffered_write_iomap_begin()
> can be waiting for the ILOCK at that time. It then reserves blocks the
> zoned way on a regular inode, and the free block counter ends up too
> high.
> 
> The fix used here is to test again under the ILOCK, before blocks are
> reserved or allocated, and to make the fault start over if the flag has
> changed. Any other caller that gets there fails, as a write does.
> 
> Fixes: 058dd70c65ab ("xfs: implement buffered writes to zoned RT devices")
> Signed-off-by: Chris Mason <mason@kernel.org>
> ---
> 
> Notes:
>     v7->v8:
>     - Only a page fault gets -EAGAIN. Any other caller that gets to one of
>       the tests fails with -EINVAL, as a write and an fallocate do. pNFS can
>       get there: xfs_fs_map_blocks() tests the flag before it takes its
>       locks (Christoph Hellwig).
>     - The warning now says "during block mapping".
>     - The helper reads the inode number with I_INO(), and its comment no
>       longer says that the flag is stable once the inode has blocks.
>     - Was 11/11.
>     
>     v6->v7:
>     - xfs_direct_write_iomap_begin() tests the flag too, as soon as it holds
>       the ILOCK. It could allocate blocks for a COW fork extent before it
>       got to the test in xfs_iomap_write_direct().
>     - The assert on the extent size hint in
>       xfs_zoned_buffered_write_iomap_begin() moved below the test of the
>       flag.
>     - Dropped the Reviewed-by of Christoph Hellwig, since the patch changed.
>     - Was 10/10.
> 
>  fs/xfs/xfs_iomap.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c3465204b8d5..d36650450cbb 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -264,6 +264,25 @@ xfs_iomap_eof_align_last_fsb(
>  	return end_fsb;
>  }
>  
> +/*
> + * A write fault picks the zoned or the regular path before it takes a lock, and
> + * it never takes the IOLOCK.  The realtime flag cannot change under the ILOCK,
> + * so look at it again there, before reserving or allocating blocks.  Only a
> + * fault can start over, so refuse any other caller, as the write path does.
> + */
> +static int
> +xfs_iomap_rtflag_changed(
> +	struct xfs_inode	*ip,
> +	unsigned int		flags)
> +{
> +	xfs_warn_ratelimited(ip->i_mount,
> +		"RT flag modifying during block mapping for inode 0x%llx.",
> +		I_INO(ip));
> +	if (flags & IOMAP_FAULT)
> +		return -EAGAIN;
> +	return -EINVAL;
> +}
> +
>  int
>  xfs_iomap_write_direct(
>  	struct xfs_inode	*ip,
> @@ -334,6 +353,11 @@ xfs_iomap_write_direct(
>  	if (error)
>  		return error;
>  
> +	if (xfs_is_zoned_inode(ip)) {
> +		error = xfs_iomap_rtflag_changed(ip, flags);
> +		goto out_trans_cancel;
> +	}
> +
>  	error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, nr_exts);
>  	if (error)
>  		goto out_trans_cancel;
> @@ -919,6 +943,11 @@ xfs_direct_write_iomap_begin(
>  	if (error)
>  		return error;
>  
> +	if (xfs_is_zoned_inode(ip)) {
> +		error = xfs_iomap_rtflag_changed(ip, flags);
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * The reflink iflag could have changed since the earlier unlocked
>  	 * check, check if it again and relock if needed.
> @@ -1666,9 +1695,7 @@ xfs_zoned_buffered_write_iomap_begin(
>  	struct xfs_iext_cursor	icur;
>  	int			error = 0;
>  
> -	ASSERT(!xfs_get_extsz_hint(ip));
>  	ASSERT(!(flags & IOMAP_UNSHARE));
> -	ASSERT(ac);
>  
>  	if (xfs_is_shutdown(mp))
>  		return -EIO;
> @@ -1682,6 +1709,12 @@ xfs_zoned_buffered_write_iomap_begin(
>  	if (error)
>  		return error;
>  
> +	if (!xfs_is_zoned_inode(ip) || !ac) {
> +		error = xfs_iomap_rtflag_changed(ip, flags);
> +		goto out_unlock;
> +	}

For actual writes this can't happens as we're under the IOLOCK (i_rwsem)
taken in xfs_file_buffered_write here, and the only way to change
the REALTIME flag is through vfs_fileattr_set which also takes
i_rwsem in exclusive mode.  So this should probably assert that
IOMAP_FAULT is set.

Given that the MMAPLOCK (invalidate_lock) nests inside the IOLOCK,
it might makes sense to also take the MMAPLOCK exclusive in
xfs_fileattr_set and just check for the rtflag check early in
__xfs_write_fault instead of leaking this into iomap for buffered
I/O.  This would also remove the need for the extra ac check.

> @@ -1865,6 +1898,11 @@ xfs_buffered_write_iomap_begin(
>  	if (error)
>  		return error;
>  
> +	if (xfs_is_zoned_inode(ip)) {
> +		error = xfs_iomap_rtflag_changed(ip, flags);
> +		goto out_unlock;
> +	}

Same here.

>  	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) ||
>  	    XFS_TEST_ERROR(mp, XFS_ERRTAG_BMAPIFORMAT)) {
>  		xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
> -- 
> 2.55.0
---end quoted text---