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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Jan 28, 2022 at 03:02:40PM +0530, Chandan Babu R wrote:
> On 26 Jan 2022 at 07:48, 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 |   23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 22ad207bedf4..eee5fb20cf8d 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1057,13 +1057,28 @@ xfs_file_fallocate(
> >  		}
> >  	}
> >  
> > -	if (file->f_flags & O_DSYNC)
> > -		flags |= XFS_PREALLOC_SYNC;
> > -
> 
> Without the above change, if fallocate() is invoked with FALLOC_FL_PUNCH_HOLE,
> FALLOC_FL_COLLAPSE_RANGE or FALLOC_FL_INSERT_RANGE, we used to update inode's
> timestamp, remove setuid/setgid bits and then perform a synchronous
> transaction commit if O_DSYNC flag is set.
> 
> However, with this patch applied, the transaction (inside
> xfs_vn_update_time()) that updates file's inode contents (i.e. timestamps and
> setuid/setgid bits) is not synchronous and hence the O_DSYNC flag is not
> honored if the fallocate operation is one of FALLOC_FL_PUNCH_HOLE,
> FALLOC_FL_COLLAPSE_RANGE or FALLOC_FL_INSERT_RANGE.

Ah, right.  This bug is covered up by the changes in the last patch, but
it would break bisection, so I'll clean that up and resubmit.  Thanks
for the comments!

> > -	error = xfs_update_prealloc_flags(ip, flags);
> > +	/* 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, 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.
> > +	 */

So the code ends up looking like:

	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;
	}

--D

> > +	if (flags) {
> > +		flags |= XFS_PREALLOC_INVISIBLE;
> > +
> > +		if (file->f_flags & O_DSYNC)
> > +			flags |= XFS_PREALLOC_SYNC;
> > +
> > +		error = xfs_update_prealloc_flags(ip, flags);
> > +		if (error)
> > +			goto out_unlock;
> > +	}
> > +
> >  	/* Change file size if needed */
> >  	if (new_size) {
> >  		struct iattr iattr;
> 
> -- 
> chandan