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

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, chandan.babu@oracle.com,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate
Date: Mon, 31 Jan 2022 09:30:28 +1100	[thread overview]
Message-ID: <20220130223028.GV59729@dread.disaster.area> (raw)
In-Reply-To: <164351876914.4177728.15972280389302582854.stgit@magnolia>

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