| Message ID | 164351876914.4177728.15972280389302582854.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: fix permission drop and flushing in fallocate | expand |
On Sat, Jan 29, 2022 at 08:59:29PM -0800, 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 | 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.
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; + } /* Change file size if needed */ if (new_size) {