diff mbox series

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

Message ID 164351876914.4177728.15972280389302582854.stgit@magnolia (mailing list archive)
State New
Headers show
Series xfs: fix permission drop and flushing in fallocate | expand

Commit Message

Darrick J. Wong Jan. 30, 2022, 4:59 a.m. UTC
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(-)

Comments

Dave Chinner Jan. 30, 2022, 10:30 p.m. UTC | #1
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.
diff mbox series

Patch

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) {