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