[PATCH 0/5 v2] xfs: fallocate() vs xfs_update_prealloc_flags()

[PATCH 0/5 v2] xfs: fallocate() vs xfs_update_prealloc_flags()

[Dave Chinner]

Hi folks,

From Darrick's original patch set:

https://lore.kernel.org/linux-xfs/164351876356.4177728.10148216594418485828.stgit@magnolia/T/#m8785dae565ff0f68ade64ca720debd483f566d6c

While auditing the file permission dropping for fallocate, I reached the
conclusion that fallocate can modify file contents, and therefore should
be treated as a file write.  As such, it needs to update the file
modification and file (metadata) change timestamps, and it needs to drop
file privileges such as setuid and capabilities, just like a regular
write.  Moreover, if the inode is configured for synchronous writes,
then all the fallocate changes really ought to be persisted to disk
before fallocate returns to userspace.

Unfortunately, the XFS fallocate implementation doesn't do this
correctly.  setgid without group-exec is a mandatory locking mark and is
left alone by write(), which means that we shouldn't drop it
unconditionally.  Furthermore, file capabilities are another vector for
setuid to be set on a program file, and XFS ignores these.

I also noticed that fallocate doesn't flush the log to disk after
fallocate when the fs is mounted with -o sync or if the DIFLAG_SYNC flag
is set on the inode.

Therefore, refactor the XFS fallocate implementation to use the VFS
helper file_modified to update file metadata instead of open-coding it
incorrectly.  Refactor it further to use xfs_file_sync_writes to decide
if we need to flush the log; and then fix the log flushing so that it
flushes after we've made /all/ the changes.

--

And from my reply:

https://lore.kernel.org/linux-xfs/164351876356.4177728.10148216594418485828.stgit@magnolia/T/#m58cedaa33368c619fcdfb53639fce881bacfa9d3

This is more along the lines of what I was thinking. Unfortunately,
xfs_fs_map_blocks() can't be made to use file based VFS helpers, so
the whole "open code the permissions stripping on data extent
allocation" thing needs to remain in that code. Otherwise, we can
significantly clean up xfs_file_fallocate() and completely remove
the entire transaction that sets the prealloc flag. And given that
xfs_ioc_space() no longer exists, most of the option functionality
that xfs_update_prealloc_flags() provides is no longer necessary...

--

This is an updated version based on 5.17-rc2 that has been run
through fstests now and there are no apparent regressions from these
changes. 

Version 2:
- add missing error handling (patch 1)
- fix whitespace damage (patch 3)
- remove redundant comments in xfs_alloc_file_space (patch 3)
- rework comments to provide context to security issues around
  PNFS operations (patch 4)

[PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Callers can acheive the same thing by calling xfs_log_force_inode()
after making their modifications. There is no need for
xfs_update_prealloc_flags() to do this.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_file.c  | 13 +++++++------
 fs/xfs/xfs_inode.h |  3 +--
 fs/xfs/xfs_pnfs.c  |  6 ++++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 22ad207bedf4..ed375b3d0614 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -95,8 +95,6 @@ xfs_update_prealloc_flags(
 		ip->i_diflags &= ~XFS_DIFLAG_PREALLOC;
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	if (flags & XFS_PREALLOC_SYNC)
-		xfs_trans_set_sync(tp);
 	return xfs_trans_commit(tp);
 }
 
@@ -1057,9 +1055,6 @@ xfs_file_fallocate(
 		}
 	}
 
-	if (file->f_flags & O_DSYNC)
-		flags |= XFS_PREALLOC_SYNC;
-
 	error = xfs_update_prealloc_flags(ip, flags);
 	if (error)
 		goto out_unlock;
@@ -1082,8 +1077,14 @@ xfs_file_fallocate(
 	 * leave shifted extents past EOF and hence losing access to
 	 * the data that is contained within them.
 	 */
-	if (do_file_insert)
+	if (do_file_insert) {
 		error = xfs_insert_file_space(ip, offset, len);
+		if (error)
+			goto out_unlock;
+	}
+
+	if (file->f_flags & O_DSYNC)
+		error = xfs_log_force_inode(ip);
 
 out_unlock:
 	xfs_iunlock(ip, iolock);
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c447bf04205a..3fc6d77f5be9 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -465,8 +465,7 @@ xfs_itruncate_extents(
 enum xfs_prealloc_flags {
 	XFS_PREALLOC_SET	= (1 << 1),
 	XFS_PREALLOC_CLEAR	= (1 << 2),
-	XFS_PREALLOC_SYNC	= (1 << 3),
-	XFS_PREALLOC_INVISIBLE	= (1 << 4),
+	XFS_PREALLOC_INVISIBLE	= (1 << 3),
 };
 
 int	xfs_update_prealloc_flags(struct xfs_inode *ip,
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index d6334abbc0b3..ce6d66f20385 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -164,10 +164,12 @@ xfs_fs_map_blocks(
 		 * that the blocks allocated and handed out to the client are
 		 * guaranteed to be present even after a server crash.
 		 */
-		error = xfs_update_prealloc_flags(ip,
-				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
+		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
+		if (!error)
+			error = xfs_log_force_inode(ip);
 		if (error)
 			goto out_unlock;
+
 	} else {
 		xfs_iunlock(ip, lock_flags);
 	}
-- 
2.33.0

[PATCH 2/5] xfs: fallocate() should call file_modified()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

In XFS, we always update the inode change and modification time when
any fallocate() operation succeeds.  Furthermore, as various
fallocate modes can change the file contents (extending EOF,
punching holes, zeroing things, shifting extents), we should drop
file privileges like suid just like we do for a regular write().
There's already a VFS helper that figures all this out for us, so
use that.

The net effect of this is that we no longer drop suid/sgid if the
caller is root, but we also now drop file capabilities.

We also move the xfs_update_prealloc_flags() function so that it now
is only called by the scope that needs to set the the prealloc flag.

Based on a patch from Darrick Wong.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index ed375b3d0614..7846d55cba01 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -953,6 +953,10 @@ xfs_file_fallocate(
 			goto out_unlock;
 	}
 
+	error = file_modified(file);
+	if (error)
+		goto out_unlock;
+
 	if (mode & FALLOC_FL_PUNCH_HOLE) {
 		error = xfs_free_file_space(ip, offset, len);
 		if (error)
@@ -1053,11 +1057,12 @@ xfs_file_fallocate(
 			if (error)
 				goto out_unlock;
 		}
-	}
 
-	error = xfs_update_prealloc_flags(ip, flags);
-	if (error)
-		goto out_unlock;
+		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
+		if (error)
+			goto out_unlock;
+
+	}
 
 	/* Change file size if needed */
 	if (new_size) {
-- 
2.33.0

[PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now that we only call xfs_update_prealloc_flags() from
xfs_file_fallocate() in the case where we need to set the
preallocation flag, do this in xfs_alloc_file_space() where we
already have the inode joined into a transaction and get
rid of the call to xfs_update_prealloc_flags() from the fallocate
code.

This also means that we now correctly avoid setting the
XFS_DIFLAG_PREALLOC flag when xfs_is_always_cow_inode() is true, as
these inodes will never have preallocated extents.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_bmap_util.c | 9 +++------
 fs/xfs/xfs_file.c      | 8 --------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index d4a387d3d0ce..eb2e387ba528 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -850,9 +850,6 @@ xfs_alloc_file_space(
 			rblocks = 0;
 		}
 
-		/*
-		 * Allocate and setup the transaction.
-		 */
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
 				dblocks, rblocks, false, &tp);
 		if (error)
@@ -869,9 +866,9 @@ xfs_alloc_file_space(
 		if (error)
 			goto error;
 
-		/*
-		 * Complete the transaction
-		 */
+		ip->i_diflags |= XFS_DIFLAG_PREALLOC;
+		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 7846d55cba01..082e3ef81418 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -908,7 +908,6 @@ xfs_file_fallocate(
 	struct inode		*inode = file_inode(file);
 	struct xfs_inode	*ip = XFS_I(inode);
 	long			error;
-	enum xfs_prealloc_flags	flags = 0;
 	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
 	loff_t			new_size = 0;
 	bool			do_file_insert = false;
@@ -1006,8 +1005,6 @@ xfs_file_fallocate(
 		}
 		do_file_insert = true;
 	} else {
-		flags |= XFS_PREALLOC_SET;
-
 		if (!(mode & FALLOC_FL_KEEP_SIZE) &&
 		    offset + len > i_size_read(inode)) {
 			new_size = offset + len;
@@ -1057,11 +1054,6 @@ xfs_file_fallocate(
 			if (error)
 				goto out_unlock;
 		}
-
-		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
-		if (error)
-			goto out_unlock;
-
 	}
 
 	/* Change file size if needed */
-- 
2.33.0

[PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The operations that xfs_update_prealloc_flags() perform are now
unique to xfs_fs_map_blocks(), so move xfs_update_prealloc_flags()
to be a static function in xfs_pnfs.c and cut out all the
other functionality that is doesn't use anymore.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_file.c  | 32 -------------------------------
 fs/xfs/xfs_inode.h |  8 --------
 fs/xfs/xfs_pnfs.c  | 47 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 082e3ef81418..cecc5dedddff 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -66,38 +66,6 @@ xfs_is_falloc_aligned(
 	return !((pos | len) & mask);
 }
 
-int
-xfs_update_prealloc_flags(
-	struct xfs_inode	*ip,
-	enum xfs_prealloc_flags	flags)
-{
-	struct xfs_trans	*tp;
-	int			error;
-
-	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid,
-			0, 0, 0, &tp);
-	if (error)
-		return error;
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-
-	if (!(flags & XFS_PREALLOC_INVISIBLE)) {
-		VFS_I(ip)->i_mode &= ~S_ISUID;
-		if (VFS_I(ip)->i_mode & S_IXGRP)
-			VFS_I(ip)->i_mode &= ~S_ISGID;
-		xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
-	}
-
-	if (flags & XFS_PREALLOC_SET)
-		ip->i_diflags |= XFS_DIFLAG_PREALLOC;
-	if (flags & XFS_PREALLOC_CLEAR)
-		ip->i_diflags &= ~XFS_DIFLAG_PREALLOC;
-
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	return xfs_trans_commit(tp);
-}
-
 /*
  * Fsync operations on directories are much simpler than on regular files,
  * as there is no file data to flush, and thus also no need for explicit
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 3fc6d77f5be9..b7e8f14d9fca 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -462,14 +462,6 @@ xfs_itruncate_extents(
 }
 
 /* from xfs_file.c */
-enum xfs_prealloc_flags {
-	XFS_PREALLOC_SET	= (1 << 1),
-	XFS_PREALLOC_CLEAR	= (1 << 2),
-	XFS_PREALLOC_INVISIBLE	= (1 << 3),
-};
-
-int	xfs_update_prealloc_flags(struct xfs_inode *ip,
-				  enum xfs_prealloc_flags flags);
 int	xfs_break_layouts(struct inode *inode, uint *iolock,
 		enum layout_break_reason reason);
 
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index ce6d66f20385..b5e5c7ddfe67 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -70,6 +70,49 @@ xfs_fs_get_uuid(
 	return 0;
 }
 
+/*
+ * We cannot use file based VFS helpers such as file_modified() to update inode
+ * state as PNFS doesn't provide us with an open file context that the VFS
+ * helpers require. Hence we open code a best effort timestamp update and
+ * SUID/SGID stripping here, knowing that server side security in PNFS settings
+ * is largely non-existent as clients have storage level remote write access.
+ * Hence clients have the capability to overwrite filesystem metadata, and so
+ * the filesystem trust domain extends to untrusted, uncontrollable remote
+ * clients.  Hence server side enforced filesystem "security" in filesystem
+ * based PNFS block layout settings is pure theatre: friends don't let friends
+ * host executables on PNFS exported XFS volumes, let alone SUID executables.
+ *
+ * We also need to set the inode prealloc flag to ensure that the extents we
+ * allocate beyond the existing EOF and hand to the PNFS client are not removed
+ * by background blockgc scanning, ENOSPC mitigations or inode reclaim before
+ * the PNFS client calls xfs_fs_block_commit() to indicate that data has been
+ * written and the file size can be extended.
+ */
+static int
+xfs_fs_map_update_inode(
+	struct xfs_inode	*ip)
+{
+	struct xfs_trans	*tp;
+	int			error;
+
+	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid,
+			0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+
+	VFS_I(ip)->i_mode &= ~S_ISUID;
+	if (VFS_I(ip)->i_mode & S_IXGRP)
+		VFS_I(ip)->i_mode &= ~S_ISGID;
+	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+	ip->i_diflags |= XFS_DIFLAG_PREALLOC;
+
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	return xfs_trans_commit(tp);
+}
+
 /*
  * Get a layout for the pNFS client.
  */
@@ -164,7 +207,7 @@ xfs_fs_map_blocks(
 		 * that the blocks allocated and handed out to the client are
 		 * guaranteed to be present even after a server crash.
 		 */
-		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
+		error = xfs_fs_map_update_inode(ip);
 		if (!error)
 			error = xfs_log_force_inode(ip);
 		if (error)
@@ -257,7 +300,7 @@ xfs_fs_commit_blocks(
 		length = end - start;
 		if (!length)
 			continue;
-	
+
 		/*
 		 * Make sure reads through the pagecache see the new data.
 		 */
-- 
2.33.0

[PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call

[Dave Chinner]

From: "Darrick J. Wong" <djwong@kernel.org>

Since we've started treating fallocate more like a file write, we
should flush the log to disk if the user has asked for synchronous
writes either by setting it via fcntl flags, or inode flags, or with
the sync mount option.  We've already got a helper for this, so use
it.

[Slightly massaged by <dchinner@redhat.com> to fit this patchset]

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_file.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index cecc5dedddff..5bddb1e9e0b3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -861,6 +861,21 @@ xfs_break_layouts(
 	return error;
 }
 
+/* Does this file, inode, or mount want synchronous writes? */
+static inline bool xfs_file_sync_writes(struct file *filp)
+{
+	struct xfs_inode	*ip = XFS_I(file_inode(filp));
+
+	if (xfs_has_wsync(ip->i_mount))
+		return true;
+	if (filp->f_flags & (__O_SYNC | O_DSYNC))
+		return true;
+	if (IS_SYNC(file_inode(filp)))
+		return true;
+
+	return false;
+}
+
 #define	XFS_FALLOC_FL_SUPPORTED						\
 		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
 		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |	\
@@ -1048,7 +1063,7 @@ xfs_file_fallocate(
 			goto out_unlock;
 	}
 
-	if (file->f_flags & O_DSYNC)
+	if (xfs_file_sync_writes(file))
 		error = xfs_log_force_inode(ip);
 
 out_unlock:
@@ -1081,21 +1096,6 @@ xfs_file_fadvise(
 	return ret;
 }
 
-/* Does this file, inode, or mount want synchronous writes? */
-static inline bool xfs_file_sync_writes(struct file *filp)
-{
-	struct xfs_inode	*ip = XFS_I(file_inode(filp));
-
-	if (xfs_has_wsync(ip->i_mount))
-		return true;
-	if (filp->f_flags & (__O_SYNC | O_DSYNC))
-		return true;
-	if (IS_SYNC(file_inode(filp)))
-		return true;
-
-	return false;
-}
-
 STATIC loff_t
 xfs_file_remap_range(
 	struct file		*file_in,
-- 
2.33.0

Re: [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC

[Darrick J. Wong]

On Tue, Feb 01, 2022 at 10:39:16AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Callers can acheive the same thing by calling xfs_log_force_inode()
> after making their modifications. There is no need for
> xfs_update_prealloc_flags() to do this.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c  | 13 +++++++------
>  fs/xfs/xfs_inode.h |  3 +--
>  fs/xfs/xfs_pnfs.c  |  6 ++++--
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 22ad207bedf4..ed375b3d0614 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -95,8 +95,6 @@ xfs_update_prealloc_flags(
>  		ip->i_diflags &= ~XFS_DIFLAG_PREALLOC;
>  
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	if (flags & XFS_PREALLOC_SYNC)
> -		xfs_trans_set_sync(tp);
>  	return xfs_trans_commit(tp);
>  }
>  
> @@ -1057,9 +1055,6 @@ xfs_file_fallocate(
>  		}
>  	}
>  
> -	if (file->f_flags & O_DSYNC)
> -		flags |= XFS_PREALLOC_SYNC;
> -
>  	error = xfs_update_prealloc_flags(ip, flags);
>  	if (error)
>  		goto out_unlock;
> @@ -1082,8 +1077,14 @@ xfs_file_fallocate(
>  	 * leave shifted extents past EOF and hence losing access to
>  	 * the data that is contained within them.
>  	 */
> -	if (do_file_insert)
> +	if (do_file_insert) {
>  		error = xfs_insert_file_space(ip, offset, len);
> +		if (error)
> +			goto out_unlock;
> +	}
> +
> +	if (file->f_flags & O_DSYNC)
> +		error = xfs_log_force_inode(ip);
>  
>  out_unlock:
>  	xfs_iunlock(ip, iolock);
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index c447bf04205a..3fc6d77f5be9 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -465,8 +465,7 @@ xfs_itruncate_extents(
>  enum xfs_prealloc_flags {
>  	XFS_PREALLOC_SET	= (1 << 1),
>  	XFS_PREALLOC_CLEAR	= (1 << 2),
> -	XFS_PREALLOC_SYNC	= (1 << 3),
> -	XFS_PREALLOC_INVISIBLE	= (1 << 4),
> +	XFS_PREALLOC_INVISIBLE	= (1 << 3),
>  };
>  
>  int	xfs_update_prealloc_flags(struct xfs_inode *ip,
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index d6334abbc0b3..ce6d66f20385 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -164,10 +164,12 @@ xfs_fs_map_blocks(
>  		 * that the blocks allocated and handed out to the client are
>  		 * guaranteed to be present even after a server crash.
>  		 */
> -		error = xfs_update_prealloc_flags(ip,
> -				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
> +		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
> +		if (!error)
> +			error = xfs_log_force_inode(ip);
>  		if (error)
>  			goto out_unlock;
> +
>  	} else {
>  		xfs_iunlock(ip, lock_flags);
>  	}
> -- 
> 2.33.0
> 

Re: [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c

[Darrick J. Wong]

On Tue, Feb 01, 2022 at 10:39:19AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The operations that xfs_update_prealloc_flags() perform are now
> unique to xfs_fs_map_blocks(), so move xfs_update_prealloc_flags()
> to be a static function in xfs_pnfs.c and cut out all the
> other functionality that is doesn't use anymore.
> 

LGTM,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_file.c  | 32 -------------------------------
>  fs/xfs/xfs_inode.h |  8 --------
>  fs/xfs/xfs_pnfs.c  | 47 ++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 45 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 082e3ef81418..cecc5dedddff 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -66,38 +66,6 @@ xfs_is_falloc_aligned(
>  	return !((pos | len) & mask);
>  }
>  
> -int
> -xfs_update_prealloc_flags(
> -	struct xfs_inode	*ip,
> -	enum xfs_prealloc_flags	flags)
> -{
> -	struct xfs_trans	*tp;
> -	int			error;
> -
> -	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid,
> -			0, 0, 0, &tp);
> -	if (error)
> -		return error;
> -
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> -
> -	if (!(flags & XFS_PREALLOC_INVISIBLE)) {
> -		VFS_I(ip)->i_mode &= ~S_ISUID;
> -		if (VFS_I(ip)->i_mode & S_IXGRP)
> -			VFS_I(ip)->i_mode &= ~S_ISGID;
> -		xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> -	}
> -
> -	if (flags & XFS_PREALLOC_SET)
> -		ip->i_diflags |= XFS_DIFLAG_PREALLOC;
> -	if (flags & XFS_PREALLOC_CLEAR)
> -		ip->i_diflags &= ~XFS_DIFLAG_PREALLOC;
> -
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	return xfs_trans_commit(tp);
> -}
> -
>  /*
>   * Fsync operations on directories are much simpler than on regular files,
>   * as there is no file data to flush, and thus also no need for explicit
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 3fc6d77f5be9..b7e8f14d9fca 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -462,14 +462,6 @@ xfs_itruncate_extents(
>  }
>  
>  /* from xfs_file.c */
> -enum xfs_prealloc_flags {
> -	XFS_PREALLOC_SET	= (1 << 1),
> -	XFS_PREALLOC_CLEAR	= (1 << 2),
> -	XFS_PREALLOC_INVISIBLE	= (1 << 3),
> -};
> -
> -int	xfs_update_prealloc_flags(struct xfs_inode *ip,
> -				  enum xfs_prealloc_flags flags);
>  int	xfs_break_layouts(struct inode *inode, uint *iolock,
>  		enum layout_break_reason reason);
>  
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index ce6d66f20385..b5e5c7ddfe67 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -70,6 +70,49 @@ xfs_fs_get_uuid(
>  	return 0;
>  }
>  
> +/*
> + * We cannot use file based VFS helpers such as file_modified() to update inode
> + * state as PNFS doesn't provide us with an open file context that the VFS
> + * helpers require. Hence we open code a best effort timestamp update and
> + * SUID/SGID stripping here, knowing that server side security in PNFS settings
> + * is largely non-existent as clients have storage level remote write access.
> + * Hence clients have the capability to overwrite filesystem metadata, and so
> + * the filesystem trust domain extends to untrusted, uncontrollable remote
> + * clients.  Hence server side enforced filesystem "security" in filesystem
> + * based PNFS block layout settings is pure theatre: friends don't let friends
> + * host executables on PNFS exported XFS volumes, let alone SUID executables.
> + *
> + * We also need to set the inode prealloc flag to ensure that the extents we
> + * allocate beyond the existing EOF and hand to the PNFS client are not removed
> + * by background blockgc scanning, ENOSPC mitigations or inode reclaim before
> + * the PNFS client calls xfs_fs_block_commit() to indicate that data has been
> + * written and the file size can be extended.
> + */
> +static int
> +xfs_fs_map_update_inode(
> +	struct xfs_inode	*ip)
> +{
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid,
> +			0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +
> +	VFS_I(ip)->i_mode &= ~S_ISUID;
> +	if (VFS_I(ip)->i_mode & S_IXGRP)
> +		VFS_I(ip)->i_mode &= ~S_ISGID;
> +	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +	ip->i_diflags |= XFS_DIFLAG_PREALLOC;
> +
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	return xfs_trans_commit(tp);
> +}
> +
>  /*
>   * Get a layout for the pNFS client.
>   */
> @@ -164,7 +207,7 @@ xfs_fs_map_blocks(
>  		 * that the blocks allocated and handed out to the client are
>  		 * guaranteed to be present even after a server crash.
>  		 */
> -		error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET);
> +		error = xfs_fs_map_update_inode(ip);
>  		if (!error)
>  			error = xfs_log_force_inode(ip);
>  		if (error)
> @@ -257,7 +300,7 @@ xfs_fs_commit_blocks(
>  		length = end - start;
>  		if (!length)
>  			continue;
> -	
> +
>  		/*
>  		 * Make sure reads through the pagecache see the new data.
>  		 */
> -- 
> 2.33.0
>