[PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate

[PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate

[Darrick J. Wong]

Hi all,

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.

v2: fix some bisection problems

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=falloc-fix-perm-updates-5.17
---
 fs/xfs/xfs_file.c |   72 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 20 deletions(-)

[PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate

[Darrick J. Wong]

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

In XFS, we always update the inode change and modification time when any
preallocation 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.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_file.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 22ad207bedf4..3b0d026396e5 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1057,12 +1057,26 @@ xfs_file_fallocate(
 		}
 	}
 
+	/* Update [cm]time and drop file privileges like a regular write. */
+	error = file_modified(file);
+	if (error)
+		goto out_unlock;
+
+	/*
+	 * If we need to change the PREALLOC flag or flush the log, do so.
+	 * We already updated the timestamps and cleared the suid flags, so we
+	 * don't need to do that again.  This must be committed before the size
+	 * change so that we don't trim post-EOF preallocations.
+	 */
 	if (file->f_flags & O_DSYNC)
 		flags |= XFS_PREALLOC_SYNC;
+	if (flags) {
+		flags |= XFS_PREALLOC_INVISIBLE;
 
-	error = xfs_update_prealloc_flags(ip, flags);
-	if (error)
-		goto out_unlock;
+		error = xfs_update_prealloc_flags(ip, flags);
+		if (error)
+			goto out_unlock;
+	}
 
 	/* Change file size if needed */
 	if (new_size) {

Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate

[Dave Chinner]

On Sat, Jan 29, 2022 at 08:59:29PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In XFS, we always update the inode change and modification time when any
> preallocation 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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_file.c |   20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 22ad207bedf4..3b0d026396e5 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1057,12 +1057,26 @@ xfs_file_fallocate(
>  		}
>  	}
>  
> +	/* Update [cm]time and drop file privileges like a regular write. */
> +	error = file_modified(file);
> +	if (error)
> +		goto out_unlock;
> +
> +	/*
> +	 * If we need to change the PREALLOC flag or flush the log, do so.
> +	 * We already updated the timestamps and cleared the suid flags, so we
> +	 * don't need to do that again.  This must be committed before the size
> +	 * change so that we don't trim post-EOF preallocations.
> +	 */
>  	if (file->f_flags & O_DSYNC)
>  		flags |= XFS_PREALLOC_SYNC;
> +	if (flags) {
> +		flags |= XFS_PREALLOC_INVISIBLE;
> -	error = xfs_update_prealloc_flags(ip, flags);
> -	if (error)
> -		goto out_unlock;
> +		error = xfs_update_prealloc_flags(ip, flags);
> +		if (error)
> +			goto out_unlock;
> +	}

That's a change of behaviour in that if O_DSYNC is not used, we
won't call xfs_update_prealloc_flags() and so won't always log the
inode here, regardless of whether the timestamps are changed or not.

Regardless, the only other caller of xfs_update_prealloc_flags() is
xfs_fs_map_blocks(), and that clearly modifies the layout of the
file so it has the same issue w.r.t. stripping privileges via
xfs_update_prealloc_flags(). So it should really also
and not the open coded stripping done in
xfs_update_prealloc_flags().

As such, I think that the use of XFS_PREALLOC_INVISIBLE here is not
a very nice workaround to avoid repeating the work done by
file_modified(). All the code that does direct extent modification
should perform the same actions for the same reasons. And if you
xfs_fs_map_blocks() to use xfs_log_force_inode() like patch 3 in
this series does for fallocate(), then XFS_PREALLOC_SYNC and that
code in xfs_update_prealloc_flags() can go away as well....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/3] xfs: flush log after fallocate for sync mounts and sync inodes

[Darrick J. Wong]

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.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 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 3b0d026396e5..a54a38e66744 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -895,6 +895,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 |	\
@@ -1068,7 +1083,7 @@ xfs_file_fallocate(
 	 * don't need to do that again.  This must be committed before the size
 	 * change so that we don't trim post-EOF preallocations.
 	 */
-	if (file->f_flags & O_DSYNC)
+	if (xfs_file_sync_writes(file))
 		flags |= XFS_PREALLOC_SYNC;
 	if (flags) {
 		flags |= XFS_PREALLOC_INVISIBLE;
@@ -1129,21 +1144,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,

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

[Darrick J. Wong]

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

If the caller wanted us to persist the preallocation to disk before
returning to userspace, make sure we force the log to disk after making
all metadata updates.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_file.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a54a38e66744..8f2372b96fc4 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -929,6 +929,7 @@ xfs_file_fallocate(
 	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
 	loff_t			new_size = 0;
 	bool			do_file_insert = false;
+	bool			flush_log;
 
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
@@ -1078,16 +1079,19 @@ xfs_file_fallocate(
 		goto out_unlock;
 
 	/*
-	 * If we need to change the PREALLOC flag or flush the log, do so.
-	 * We already updated the timestamps and cleared the suid flags, so we
-	 * don't need to do that again.  This must be committed before the size
-	 * change so that we don't trim post-EOF preallocations.
+	 * If we need to change the PREALLOC flag, do so.  We already updated
+	 * the timestamps and cleared the suid flags, so we don't need to do
+	 * that again.  This must be committed before the size change so that
+	 * we don't trim post-EOF preallocations.  If this is the last
+	 * transaction we're going to make, make the update synchronous too.
 	 */
-	if (xfs_file_sync_writes(file))
-		flags |= XFS_PREALLOC_SYNC;
+	flush_log = xfs_file_sync_writes(file);
 	if (flags) {
 		flags |= XFS_PREALLOC_INVISIBLE;
 
+		if (flush_log && !(do_file_insert || new_size))
+			flags |= XFS_PREALLOC_SYNC;
+
 		error = xfs_update_prealloc_flags(ip, flags);
 		if (error)
 			goto out_unlock;
@@ -1111,8 +1115,22 @@ 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 the caller wants us to flush the log and either we've made
+	 * changes since updating the PREALLOC flag or we didn't need to
+	 * update the PREALLOC flag, then flush the log now.
+	 */
+	if (flush_log && (do_file_insert || new_size || flags == 0)) {
+		error = xfs_log_force_inode(ip);
+		if (error)
+			goto out_unlock;
+	}
 
 out_unlock:
 	xfs_iunlock(ip, iolock);

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

[Dave Chinner]

On Sat, Jan 29, 2022 at 08:59:40PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If the caller wanted us to persist the preallocation to disk before
> returning to userspace, make sure we force the log to disk after making
> all metadata updates.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_file.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index a54a38e66744..8f2372b96fc4 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -929,6 +929,7 @@ xfs_file_fallocate(
>  	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
>  	loff_t			new_size = 0;
>  	bool			do_file_insert = false;
> +	bool			flush_log;
>  
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
> @@ -1078,16 +1079,19 @@ xfs_file_fallocate(
>  		goto out_unlock;
>  
>  	/*
> -	 * If we need to change the PREALLOC flag or flush the log, do so.
> -	 * We already updated the timestamps and cleared the suid flags, so we
> -	 * don't need to do that again.  This must be committed before the size
> -	 * change so that we don't trim post-EOF preallocations.
> +	 * If we need to change the PREALLOC flag, do so.  We already updated
> +	 * the timestamps and cleared the suid flags, so we don't need to do
> +	 * that again.  This must be committed before the size change so that
> +	 * we don't trim post-EOF preallocations.  If this is the last
> +	 * transaction we're going to make, make the update synchronous too.
>  	 */
> -	if (xfs_file_sync_writes(file))
> -		flags |= XFS_PREALLOC_SYNC;
> +	flush_log = xfs_file_sync_writes(file);
>  	if (flags) {
>  		flags |= XFS_PREALLOC_INVISIBLE;
>  
> +		if (flush_log && !(do_file_insert || new_size))
> +			flags |= XFS_PREALLOC_SYNC;
> +
>  		error = xfs_update_prealloc_flags(ip, flags);
>  		if (error)
>  			goto out_unlock;
> @@ -1111,8 +1115,22 @@ 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 the caller wants us to flush the log and either we've made
> +	 * changes since updating the PREALLOC flag or we didn't need to
> +	 * update the PREALLOC flag, then flush the log now.
> +	 */
> +	if (flush_log && (do_file_insert || new_size || flags == 0)) {
> +		error = xfs_log_force_inode(ip);
> +		if (error)
> +			goto out_unlock;
> +	}

That's pretty crazy. We don't need to do synchronous transactions
for every operation in fallocate(), just guarantee that the
transactions have hit stable storage before we return to userspace.
Hence we don't need to pass SYNC flags anywhere or have stuff like
xfs_update_prealloc_flags() even have to support sync transactions.
All we need is this:

	if (xfs_file_sync_writes(file))
		error = xfs_log_force_inode(ip);

And that will force out all the changes to the journal at the end
of fallocate if required.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Dave Chinner]

Hi Darrick,

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 only smoke tested so far, but I think it gets us the same
benefits but also makes the code a lot simpler at the same time.
Your thoughts?

-Dave.

[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  | 8 +++-----
 fs/xfs/xfs_inode.h | 3 +--
 fs/xfs/xfs_pnfs.c  | 6 ++++--
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 22ad207bedf4..6eda41710a5a 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;
@@ -1085,6 +1080,9 @@ xfs_file_fallocate(
 	if (do_file_insert)
 		error = xfs_insert_file_space(ip, offset, len);
 
+	if (file->f_flags & O_DSYNC)
+		error = xfs_log_force_inode(ip);
+
 out_unlock:
 	xfs_iunlock(ip, iolock);
 	return error;
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 1/5] xfs: remove XFS_PREALLOC_SYNC

[Darrick J. Wong]

On Mon, Jan 31, 2022 at 05:43:46PM +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>
> ---
>  fs/xfs/xfs_file.c  | 8 +++-----
>  fs/xfs/xfs_inode.h | 3 +--
>  fs/xfs/xfs_pnfs.c  | 6 ++++--
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 22ad207bedf4..6eda41710a5a 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;
> @@ -1085,6 +1080,9 @@ xfs_file_fallocate(
>  	if (do_file_insert)
>  		error = xfs_insert_file_space(ip, offset, len);

This needs to handle a nonzero error case here.

The rest of the logic makes sense though.

--D

>  
> +	if (file->f_flags & O_DSYNC)
> +		error = xfs_log_force_inode(ip);
> +
>  out_unlock:
>  	xfs_iunlock(ip, iolock);
>  	return error;
> 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>
---
 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 6eda41710a5a..223996822d84 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

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

[Darrick J. Wong]

On Mon, Jan 31, 2022 at 05:43:47PM +1100, Dave Chinner wrote:
> 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>

I think you can get rid of @flags entirely, right?

With that fixed, I think this looks good.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  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 6eda41710a5a..223996822d84 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>
---
 fs/xfs/xfs_bmap_util.c | 3 +++
 fs/xfs/xfs_file.c      | 8 --------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index d4a387d3d0ce..44fd91deeca8 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -869,6 +869,9 @@ xfs_alloc_file_space(
 		if (error)
 			goto error;
 
+		ip->i_diflags |= XFS_DIFLAG_PREALLOC;
+                xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 		/*
 		 * Complete the transaction
 		 */
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 223996822d84..ae6f5b15a023 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 v1.1 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>
---
V1.1
- fix whitespace damage
- remove redundant comments in xfs_alloc_file_space().

 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 223996822d84..ae6f5b15a023 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 */
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1.1 3/5] xfs: set prealloc flag in xfs_alloc_file_space()

[Darrick J. Wong]

On Mon, Jan 31, 2022 at 06:02:26PM +1100, Dave Chinner wrote:
> 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>

Aha, there's the @flags elision.

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

--D

> ---
> V1.1
> - fix whitespace damage
> - remove redundant comments in xfs_alloc_file_space().
> 
>  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 223996822d84..ae6f5b15a023 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 */
> -- 
> Dave Chinner
> david@fromorbit.com

[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  | 38 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index ae6f5b15a023..ddc3336e8f84 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..4abe17312c2b 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -70,6 +70,40 @@ xfs_fs_get_uuid(
 	return 0;
 }
 
+/*
+ * We cannot use file based VFS helpers such as file_modified() to update
+ * inode state as we modify the data/metadata in the inode here. Hence we have
+ * to open code the timestamp updates and SUID/SGID stripping. We also need
+ * to set the inode prealloc flag to ensure that the extents we allocate are not
+ * removed if the inode is reclaimed from memory before xfs_fs_block_commit()
+ * is from the client 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 +198,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 +291,7 @@ xfs_fs_commit_blocks(
 		length = end - start;
 		if (!length)
 			continue;
-	
+
 		/*
 		 * Make sure reads through the pagecache see the new data.
 		 */
-- 
2.33.0

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

[Darrick J. Wong]

On Mon, Jan 31, 2022 at 05:43:49PM +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.
> 
> 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  | 38 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 36 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ae6f5b15a023..ddc3336e8f84 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..4abe17312c2b 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -70,6 +70,40 @@ xfs_fs_get_uuid(
>  	return 0;
>  }
>  
> +/*
> + * We cannot use file based VFS helpers such as file_modified() to update
> + * inode state as we modify the data/metadata in the inode here. Hence we have

Hmm.  I'm a little fuzzy on the significance of "...as we modify the
data/metadata" here.  fallocate also modifies file contents and
metadata, so why is pnfs special?

Is it because pnfs doesn't check for freeze/ro state before calling
->map_blocks, and we don't want to stall on file_update_time?

> + * to open code the timestamp updates and SUID/SGID stripping. We also need
> + * to set the inode prealloc flag to ensure that the extents we allocate are not
> + * removed if the inode is reclaimed from memory before xfs_fs_block_commit()
> + * is from the client to indicate that data has been written and the file size

"is called from the client" ?

--D

> + * 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 +198,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 +291,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 ddc3336e8f84..209cba0f0ddc 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 |	\
@@ -1045,7 +1060,7 @@ xfs_file_fallocate(
 	if (do_file_insert)
 		error = xfs_insert_file_space(ip, offset, len);
 
-	if (file->f_flags & O_DSYNC)
+	if (xfs_file_sync_writes(file))
 		error = xfs_log_force_inode(ip);
 
 out_unlock:
@@ -1078,21 +1093,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 5/5] xfs: ensure log flush at the end of a synchronous fallocate call

[Darrick J. Wong]

On Mon, Jan 31, 2022 at 05:43:50PM +1100, Dave Chinner wrote:
> 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]

I think you made more than 'slight massage' changes...

> 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 ddc3336e8f84..209cba0f0ddc 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 |	\
> @@ -1045,7 +1060,7 @@ xfs_file_fallocate(
>  	if (do_file_insert)
>  		error = xfs_insert_file_space(ip, offset, len);
>  
> -	if (file->f_flags & O_DSYNC)
> +	if (xfs_file_sync_writes(file))
>  		error = xfs_log_force_inode(ip);

...since the preceeding patches that you wrote enable simpler logic
here.  You've done all the (re)thinking necessary to get here, so I
think on those grounds:

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

--D


>  
>  out_unlock:
> @@ -1078,21 +1093,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
> 

[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

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
>