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

[Chandan Babu R <chandan.babu@oracle.com>]

On 29 Jan 2022 at 03:53, Darrick J. Wong wrote:
> 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;
> 	}
>

The above change looks good to me.

>> > +	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