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