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

[Chris Mason <mason@kernel.org>]

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;
+	}
+	ASSERT(!xfs_get_extsz_hint(ip));
+
 	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);
@@ -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;
+	}
+
 	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