On Sat, Jan 29, 2022 at 08:59:29PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
