* [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate @ 2022-01-30 4:59 Darrick J. Wong 2022-01-30 4:59 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong ` (3 more replies) 0 siblings, 4 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-30 4:59 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, chandan.babu, linux-fsdevel Hi all, While auditing the file permission dropping for fallocate, I reached the conclusion that fallocate can modify file contents, and therefore should be treated as a file write. As such, it needs to update the file modification and file (metadata) change timestamps, and it needs to drop file privileges such as setuid and capabilities, just like a regular write. Moreover, if the inode is configured for synchronous writes, then all the fallocate changes really ought to be persisted to disk before fallocate returns to userspace. Unfortunately, the XFS fallocate implementation doesn't do this correctly. setgid without group-exec is a mandatory locking mark and is left alone by write(), which means that we shouldn't drop it unconditionally. Furthermore, file capabilities are another vector for setuid to be set on a program file, and XFS ignores these. I also noticed that fallocate doesn't flush the log to disk after fallocate when the fs is mounted with -o sync or if the DIFLAG_SYNC flag is set on the inode. Therefore, refactor the XFS fallocate implementation to use the VFS helper file_modified to update file metadata instead of open-coding it incorrectly. Refactor it further to use xfs_file_sync_writes to decide if we need to flush the log; and then fix the log flushing so that it flushes after we've made /all/ the changes. v2: fix some bisection problems If you're going to start using this mess, you probably ought to just pull from my git trees, which are linked below. This is an extraordinary way to destroy everything. Enjoy! Comments and questions are, as always, welcome. --D kernel git tree: https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=falloc-fix-perm-updates-5.17 --- fs/xfs/xfs_file.c | 72 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 20 deletions(-) ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-30 4:59 [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong @ 2022-01-30 4:59 ` Darrick J. Wong 2022-01-30 22:30 ` Dave Chinner 2022-01-30 4:59 ` [PATCH 2/3] xfs: flush log after fallocate for sync mounts and sync inodes Darrick J. Wong ` (2 subsequent siblings) 3 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2022-01-30 4:59 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, chandan.babu, linux-fsdevel 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; + } /* Change file size if needed */ if (new_size) { ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-30 4:59 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong @ 2022-01-30 22:30 ` Dave Chinner 0 siblings, 0 replies; 22+ messages in thread From: Dave Chinner @ 2022-01-30 22:30 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, chandan.babu, linux-fsdevel 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] xfs: flush log after fallocate for sync mounts and sync inodes 2022-01-30 4:59 [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong 2022-01-30 4:59 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong @ 2022-01-30 4:59 ` Darrick J. Wong 2022-01-30 4:59 ` [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call Darrick J. Wong 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner 3 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-30 4:59 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, chandan.babu, linux-fsdevel From: Darrick J. Wong <djwong@kernel.org> Since we've started treating fallocate more like a file write, we should flush the log to disk if the user has asked for synchronous writes either by setting it via fcntl flags, or inode flags, or with the sync mount option. We've already got a helper for this, so use it. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/xfs_file.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 3b0d026396e5..a54a38e66744 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -895,6 +895,21 @@ xfs_break_layouts( return error; } +/* Does this file, inode, or mount want synchronous writes? */ +static inline bool xfs_file_sync_writes(struct file *filp) +{ + struct xfs_inode *ip = XFS_I(file_inode(filp)); + + if (xfs_has_wsync(ip->i_mount)) + return true; + if (filp->f_flags & (__O_SYNC | O_DSYNC)) + return true; + if (IS_SYNC(file_inode(filp))) + return true; + + return false; +} + #define XFS_FALLOC_FL_SUPPORTED \ (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | \ FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE | \ @@ -1068,7 +1083,7 @@ xfs_file_fallocate( * 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) + if (xfs_file_sync_writes(file)) flags |= XFS_PREALLOC_SYNC; if (flags) { flags |= XFS_PREALLOC_INVISIBLE; @@ -1129,21 +1144,6 @@ xfs_file_fadvise( return ret; } -/* Does this file, inode, or mount want synchronous writes? */ -static inline bool xfs_file_sync_writes(struct file *filp) -{ - struct xfs_inode *ip = XFS_I(file_inode(filp)); - - if (xfs_has_wsync(ip->i_mount)) - return true; - if (filp->f_flags & (__O_SYNC | O_DSYNC)) - return true; - if (IS_SYNC(file_inode(filp))) - return true; - - return false; -} - STATIC loff_t xfs_file_remap_range( struct file *file_in, ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call 2022-01-30 4:59 [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong 2022-01-30 4:59 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong 2022-01-30 4:59 ` [PATCH 2/3] xfs: flush log after fallocate for sync mounts and sync inodes Darrick J. Wong @ 2022-01-30 4:59 ` Darrick J. Wong 2022-01-30 21:59 ` Dave Chinner 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner 3 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2022-01-30 4:59 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, chandan.babu, linux-fsdevel From: Darrick J. Wong <djwong@kernel.org> If the caller wanted us to persist the preallocation to disk before returning to userspace, make sure we force the log to disk after making all metadata updates. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/xfs_file.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index a54a38e66744..8f2372b96fc4 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -929,6 +929,7 @@ xfs_file_fallocate( uint iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL; loff_t new_size = 0; bool do_file_insert = false; + bool flush_log; if (!S_ISREG(inode->i_mode)) return -EINVAL; @@ -1078,16 +1079,19 @@ xfs_file_fallocate( 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 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. If this is the last + * transaction we're going to make, make the update synchronous too. */ - if (xfs_file_sync_writes(file)) - flags |= XFS_PREALLOC_SYNC; + flush_log = xfs_file_sync_writes(file); if (flags) { flags |= XFS_PREALLOC_INVISIBLE; + if (flush_log && !(do_file_insert || new_size)) + flags |= XFS_PREALLOC_SYNC; + error = xfs_update_prealloc_flags(ip, flags); if (error) goto out_unlock; @@ -1111,8 +1115,22 @@ xfs_file_fallocate( * leave shifted extents past EOF and hence losing access to * the data that is contained within them. */ - if (do_file_insert) + if (do_file_insert) { error = xfs_insert_file_space(ip, offset, len); + if (error) + goto out_unlock; + } + + /* + * If the caller wants us to flush the log and either we've made + * changes since updating the PREALLOC flag or we didn't need to + * update the PREALLOC flag, then flush the log now. + */ + if (flush_log && (do_file_insert || new_size || flags == 0)) { + error = xfs_log_force_inode(ip); + if (error) + goto out_unlock; + } out_unlock: xfs_iunlock(ip, iolock); ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call 2022-01-30 4:59 ` [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call Darrick J. Wong @ 2022-01-30 21:59 ` Dave Chinner 0 siblings, 0 replies; 22+ messages in thread From: Dave Chinner @ 2022-01-30 21:59 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, chandan.babu, linux-fsdevel On Sat, Jan 29, 2022 at 08:59:40PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > If the caller wanted us to persist the preallocation to disk before > returning to userspace, make sure we force the log to disk after making > all metadata updates. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/xfs_file.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index a54a38e66744..8f2372b96fc4 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -929,6 +929,7 @@ xfs_file_fallocate( > uint iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL; > loff_t new_size = 0; > bool do_file_insert = false; > + bool flush_log; > > if (!S_ISREG(inode->i_mode)) > return -EINVAL; > @@ -1078,16 +1079,19 @@ xfs_file_fallocate( > 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 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. If this is the last > + * transaction we're going to make, make the update synchronous too. > */ > - if (xfs_file_sync_writes(file)) > - flags |= XFS_PREALLOC_SYNC; > + flush_log = xfs_file_sync_writes(file); > if (flags) { > flags |= XFS_PREALLOC_INVISIBLE; > > + if (flush_log && !(do_file_insert || new_size)) > + flags |= XFS_PREALLOC_SYNC; > + > error = xfs_update_prealloc_flags(ip, flags); > if (error) > goto out_unlock; > @@ -1111,8 +1115,22 @@ xfs_file_fallocate( > * leave shifted extents past EOF and hence losing access to > * the data that is contained within them. > */ > - if (do_file_insert) > + if (do_file_insert) { > error = xfs_insert_file_space(ip, offset, len); > + if (error) > + goto out_unlock; > + } > + > + /* > + * If the caller wants us to flush the log and either we've made > + * changes since updating the PREALLOC flag or we didn't need to > + * update the PREALLOC flag, then flush the log now. > + */ > + if (flush_log && (do_file_insert || new_size || flags == 0)) { > + error = xfs_log_force_inode(ip); > + if (error) > + goto out_unlock; > + } That's pretty crazy. We don't need to do synchronous transactions for every operation in fallocate(), just guarantee that the transactions have hit stable storage before we return to userspace. Hence we don't need to pass SYNC flags anywhere or have stuff like xfs_update_prealloc_flags() even have to support sync transactions. All we need is this: if (xfs_file_sync_writes(file)) error = xfs_log_force_inode(ip); And that will force out all the changes to the journal at the end of fallocate if required. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() 2022-01-30 4:59 [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong ` (2 preceding siblings ...) 2022-01-30 4:59 ` [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call Darrick J. Wong @ 2022-01-31 6:43 ` Dave Chinner 2022-01-31 6:43 ` [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC Dave Chinner ` (4 more replies) 3 siblings, 5 replies; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs Hi Darrick, This is more along the lines of what I was thinking. Unfortunately, xfs_fs_map_blocks() can't be made to use file based VFS helpers, so the whole "open code the permissions stripping on data extent allocation" thing needs to remain in that code. Otherwise, we can significantly clean up xfs_file_fallocate() and completely remove the entire transaction that sets the prealloc flag. And given that xfs_ioc_space() no longer exists, most of the option functionality that xfs_update_prealloc_flags() provides is no longer necessary... This is only smoke tested so far, but I think it gets us the same benefits but also makes the code a lot simpler at the same time. Your thoughts? -Dave. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner @ 2022-01-31 6:43 ` Dave Chinner 2022-01-31 17:25 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 2/5] xfs: fallocate() should call file_modified() Dave Chinner ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Callers can acheive the same thing by calling xfs_log_force_inode() after making their modifications. There is no need for xfs_update_prealloc_flags() to do this. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_file.c | 8 +++----- fs/xfs/xfs_inode.h | 3 +-- fs/xfs/xfs_pnfs.c | 6 ++++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 22ad207bedf4..6eda41710a5a 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -95,8 +95,6 @@ xfs_update_prealloc_flags( ip->i_diflags &= ~XFS_DIFLAG_PREALLOC; xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - if (flags & XFS_PREALLOC_SYNC) - xfs_trans_set_sync(tp); return xfs_trans_commit(tp); } @@ -1057,9 +1055,6 @@ xfs_file_fallocate( } } - if (file->f_flags & O_DSYNC) - flags |= XFS_PREALLOC_SYNC; - error = xfs_update_prealloc_flags(ip, flags); if (error) goto out_unlock; @@ -1085,6 +1080,9 @@ xfs_file_fallocate( if (do_file_insert) error = xfs_insert_file_space(ip, offset, len); + if (file->f_flags & O_DSYNC) + error = xfs_log_force_inode(ip); + out_unlock: xfs_iunlock(ip, iolock); return error; diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index c447bf04205a..3fc6d77f5be9 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -465,8 +465,7 @@ xfs_itruncate_extents( enum xfs_prealloc_flags { XFS_PREALLOC_SET = (1 << 1), XFS_PREALLOC_CLEAR = (1 << 2), - XFS_PREALLOC_SYNC = (1 << 3), - XFS_PREALLOC_INVISIBLE = (1 << 4), + XFS_PREALLOC_INVISIBLE = (1 << 3), }; int xfs_update_prealloc_flags(struct xfs_inode *ip, diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c index d6334abbc0b3..ce6d66f20385 100644 --- a/fs/xfs/xfs_pnfs.c +++ b/fs/xfs/xfs_pnfs.c @@ -164,10 +164,12 @@ xfs_fs_map_blocks( * that the blocks allocated and handed out to the client are * guaranteed to be present even after a server crash. */ - error = xfs_update_prealloc_flags(ip, - XFS_PREALLOC_SET | XFS_PREALLOC_SYNC); + error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); + if (!error) + error = xfs_log_force_inode(ip); if (error) goto out_unlock; + } else { xfs_iunlock(ip, lock_flags); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC 2022-01-31 6:43 ` [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC Dave Chinner @ 2022-01-31 17:25 ` Darrick J. Wong 0 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-31 17:25 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Mon, Jan 31, 2022 at 05:43:46PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Callers can acheive the same thing by calling xfs_log_force_inode() > after making their modifications. There is no need for > xfs_update_prealloc_flags() to do this. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_file.c | 8 +++----- > fs/xfs/xfs_inode.h | 3 +-- > fs/xfs/xfs_pnfs.c | 6 ++++-- > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 22ad207bedf4..6eda41710a5a 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -95,8 +95,6 @@ xfs_update_prealloc_flags( > ip->i_diflags &= ~XFS_DIFLAG_PREALLOC; > > xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > - if (flags & XFS_PREALLOC_SYNC) > - xfs_trans_set_sync(tp); > return xfs_trans_commit(tp); > } > > @@ -1057,9 +1055,6 @@ xfs_file_fallocate( > } > } > > - if (file->f_flags & O_DSYNC) > - flags |= XFS_PREALLOC_SYNC; > - > error = xfs_update_prealloc_flags(ip, flags); > if (error) > goto out_unlock; > @@ -1085,6 +1080,9 @@ xfs_file_fallocate( > if (do_file_insert) > error = xfs_insert_file_space(ip, offset, len); This needs to handle a nonzero error case here. The rest of the logic makes sense though. --D > > + if (file->f_flags & O_DSYNC) > + error = xfs_log_force_inode(ip); > + > out_unlock: > xfs_iunlock(ip, iolock); > return error; > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index c447bf04205a..3fc6d77f5be9 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -465,8 +465,7 @@ xfs_itruncate_extents( > enum xfs_prealloc_flags { > XFS_PREALLOC_SET = (1 << 1), > XFS_PREALLOC_CLEAR = (1 << 2), > - XFS_PREALLOC_SYNC = (1 << 3), > - XFS_PREALLOC_INVISIBLE = (1 << 4), > + XFS_PREALLOC_INVISIBLE = (1 << 3), > }; > > int xfs_update_prealloc_flags(struct xfs_inode *ip, > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c > index d6334abbc0b3..ce6d66f20385 100644 > --- a/fs/xfs/xfs_pnfs.c > +++ b/fs/xfs/xfs_pnfs.c > @@ -164,10 +164,12 @@ xfs_fs_map_blocks( > * that the blocks allocated and handed out to the client are > * guaranteed to be present even after a server crash. > */ > - error = xfs_update_prealloc_flags(ip, > - XFS_PREALLOC_SET | XFS_PREALLOC_SYNC); > + error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); > + if (!error) > + error = xfs_log_force_inode(ip); > if (error) > goto out_unlock; > + > } else { > xfs_iunlock(ip, lock_flags); > } > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/5] xfs: fallocate() should call file_modified() 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner 2022-01-31 6:43 ` [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC Dave Chinner @ 2022-01-31 6:43 ` Dave Chinner 2022-01-31 17:27 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space() Dave Chinner ` (2 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> In XFS, we always update the inode change and modification time when any fallocate() 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. We also move the xfs_update_prealloc_flags() function so that it now is only called by the scope that needs to set the the prealloc flag. Based on a patch from Darrick Wong. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_file.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 6eda41710a5a..223996822d84 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -953,6 +953,10 @@ xfs_file_fallocate( goto out_unlock; } + error = file_modified(file); + if (error) + goto out_unlock; + if (mode & FALLOC_FL_PUNCH_HOLE) { error = xfs_free_file_space(ip, offset, len); if (error) @@ -1053,11 +1057,12 @@ xfs_file_fallocate( if (error) goto out_unlock; } - } - error = xfs_update_prealloc_flags(ip, flags); - if (error) - goto out_unlock; + error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); + if (error) + goto out_unlock; + + } /* Change file size if needed */ if (new_size) { -- 2.33.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] xfs: fallocate() should call file_modified() 2022-01-31 6:43 ` [PATCH 2/5] xfs: fallocate() should call file_modified() Dave Chinner @ 2022-01-31 17:27 ` Darrick J. Wong 0 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-31 17:27 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Mon, Jan 31, 2022 at 05:43:47PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > In XFS, we always update the inode change and modification time when > any fallocate() 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. > > We also move the xfs_update_prealloc_flags() function so that it now > is only called by the scope that needs to set the the prealloc flag. > > Based on a patch from Darrick Wong. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> I think you can get rid of @flags entirely, right? With that fixed, I think this looks good. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_file.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 6eda41710a5a..223996822d84 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -953,6 +953,10 @@ xfs_file_fallocate( > goto out_unlock; > } > > + error = file_modified(file); > + if (error) > + goto out_unlock; > + > if (mode & FALLOC_FL_PUNCH_HOLE) { > error = xfs_free_file_space(ip, offset, len); > if (error) > @@ -1053,11 +1057,12 @@ xfs_file_fallocate( > if (error) > goto out_unlock; > } > - } > > - error = xfs_update_prealloc_flags(ip, flags); > - if (error) > - goto out_unlock; > + error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); > + if (error) > + goto out_unlock; > + > + } > > /* Change file size if needed */ > if (new_size) { > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space() 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner 2022-01-31 6:43 ` [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC Dave Chinner 2022-01-31 6:43 ` [PATCH 2/5] xfs: fallocate() should call file_modified() Dave Chinner @ 2022-01-31 6:43 ` Dave Chinner 2022-01-31 7:02 ` [PATCH v1.1 " Dave Chinner 2022-01-31 6:43 ` [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c Dave Chinner 2022-01-31 6:43 ` [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call Dave Chinner 4 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Now that we only call xfs_update_prealloc_flags() from xfs_file_fallocate() in the case where we need to set the preallocation flag, do this in xfs_alloc_file_space() where we already have the inode joined into a transaction and get rid of the call to xfs_update_prealloc_flags() from the fallocate code. This also means that we now correctly avoid setting the XFS_DIFLAG_PREALLOC flag when xfs_is_always_cow_inode() is true, as these inodes will never have preallocated extents. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_bmap_util.c | 3 +++ fs/xfs/xfs_file.c | 8 -------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index d4a387d3d0ce..44fd91deeca8 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -869,6 +869,9 @@ xfs_alloc_file_space( if (error) goto error; + ip->i_diflags |= XFS_DIFLAG_PREALLOC; + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + /* * Complete the transaction */ diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 223996822d84..ae6f5b15a023 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -908,7 +908,6 @@ xfs_file_fallocate( struct inode *inode = file_inode(file); struct xfs_inode *ip = XFS_I(inode); long error; - enum xfs_prealloc_flags flags = 0; uint iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL; loff_t new_size = 0; bool do_file_insert = false; @@ -1006,8 +1005,6 @@ xfs_file_fallocate( } do_file_insert = true; } else { - flags |= XFS_PREALLOC_SET; - if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > i_size_read(inode)) { new_size = offset + len; @@ -1057,11 +1054,6 @@ xfs_file_fallocate( if (error) goto out_unlock; } - - error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); - if (error) - goto out_unlock; - } /* Change file size if needed */ -- 2.33.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1.1 3/5] xfs: set prealloc flag in xfs_alloc_file_space() 2022-01-31 6:43 ` [PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space() Dave Chinner @ 2022-01-31 7:02 ` Dave Chinner 2022-01-31 17:30 ` Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 7:02 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Now that we only call xfs_update_prealloc_flags() from xfs_file_fallocate() in the case where we need to set the preallocation flag, do this in xfs_alloc_file_space() where we already have the inode joined into a transaction and get rid of the call to xfs_update_prealloc_flags() from the fallocate code. This also means that we now correctly avoid setting the XFS_DIFLAG_PREALLOC flag when xfs_is_always_cow_inode() is true, as these inodes will never have preallocated extents. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- V1.1 - fix whitespace damage - remove redundant comments in xfs_alloc_file_space(). fs/xfs/xfs_bmap_util.c | 9 +++------ fs/xfs/xfs_file.c | 8 -------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index d4a387d3d0ce..eb2e387ba528 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -850,9 +850,6 @@ xfs_alloc_file_space( rblocks = 0; } - /* - * Allocate and setup the transaction. - */ error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks, rblocks, false, &tp); if (error) @@ -869,9 +866,9 @@ xfs_alloc_file_space( if (error) goto error; - /* - * Complete the transaction - */ + ip->i_diflags |= XFS_DIFLAG_PREALLOC; + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + error = xfs_trans_commit(tp); xfs_iunlock(ip, XFS_ILOCK_EXCL); if (error) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 223996822d84..ae6f5b15a023 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -908,7 +908,6 @@ xfs_file_fallocate( struct inode *inode = file_inode(file); struct xfs_inode *ip = XFS_I(inode); long error; - enum xfs_prealloc_flags flags = 0; uint iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL; loff_t new_size = 0; bool do_file_insert = false; @@ -1006,8 +1005,6 @@ xfs_file_fallocate( } do_file_insert = true; } else { - flags |= XFS_PREALLOC_SET; - if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > i_size_read(inode)) { new_size = offset + len; @@ -1057,11 +1054,6 @@ xfs_file_fallocate( if (error) goto out_unlock; } - - error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); - if (error) - goto out_unlock; - } /* Change file size if needed */ -- Dave Chinner david@fromorbit.com ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1.1 3/5] xfs: set prealloc flag in xfs_alloc_file_space() 2022-01-31 7:02 ` [PATCH v1.1 " Dave Chinner @ 2022-01-31 17:30 ` Darrick J. Wong 0 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-31 17:30 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Mon, Jan 31, 2022 at 06:02:26PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Now that we only call xfs_update_prealloc_flags() from > xfs_file_fallocate() in the case where we need to set the > preallocation flag, do this in xfs_alloc_file_space() where we > already have the inode joined into a transaction and get > rid of the call to xfs_update_prealloc_flags() from the fallocate > code. > > This also means that we now correctly avoid setting the > XFS_DIFLAG_PREALLOC flag when xfs_is_always_cow_inode() is true, as > these inodes will never have preallocated extents. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Aha, there's the @flags elision. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > V1.1 > - fix whitespace damage > - remove redundant comments in xfs_alloc_file_space(). > > fs/xfs/xfs_bmap_util.c | 9 +++------ > fs/xfs/xfs_file.c | 8 -------- > 2 files changed, 3 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index d4a387d3d0ce..eb2e387ba528 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -850,9 +850,6 @@ xfs_alloc_file_space( > rblocks = 0; > } > > - /* > - * Allocate and setup the transaction. > - */ > error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, > dblocks, rblocks, false, &tp); > if (error) > @@ -869,9 +866,9 @@ xfs_alloc_file_space( > if (error) > goto error; > > - /* > - * Complete the transaction > - */ > + ip->i_diflags |= XFS_DIFLAG_PREALLOC; > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > + > error = xfs_trans_commit(tp); > xfs_iunlock(ip, XFS_ILOCK_EXCL); > if (error) > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 223996822d84..ae6f5b15a023 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -908,7 +908,6 @@ xfs_file_fallocate( > struct inode *inode = file_inode(file); > struct xfs_inode *ip = XFS_I(inode); > long error; > - enum xfs_prealloc_flags flags = 0; > uint iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL; > loff_t new_size = 0; > bool do_file_insert = false; > @@ -1006,8 +1005,6 @@ xfs_file_fallocate( > } > do_file_insert = true; > } else { > - flags |= XFS_PREALLOC_SET; > - > if (!(mode & FALLOC_FL_KEEP_SIZE) && > offset + len > i_size_read(inode)) { > new_size = offset + len; > @@ -1057,11 +1054,6 @@ xfs_file_fallocate( > if (error) > goto out_unlock; > } > - > - error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); > - if (error) > - goto out_unlock; > - > } > > /* Change file size if needed */ > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner ` (2 preceding siblings ...) 2022-01-31 6:43 ` [PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space() Dave Chinner @ 2022-01-31 6:43 ` Dave Chinner 2022-01-31 17:37 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call Dave Chinner 4 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> The operations that xfs_update_prealloc_flags() perform are now unique to xfs_fs_map_blocks(), so move xfs_update_prealloc_flags() to be a static function in xfs_pnfs.c and cut out all the other functionality that is doesn't use anymore. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_file.c | 32 -------------------------------- fs/xfs/xfs_inode.h | 8 -------- fs/xfs/xfs_pnfs.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 36 insertions(+), 42 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index ae6f5b15a023..ddc3336e8f84 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -66,38 +66,6 @@ xfs_is_falloc_aligned( return !((pos | len) & mask); } -int -xfs_update_prealloc_flags( - struct xfs_inode *ip, - enum xfs_prealloc_flags flags) -{ - struct xfs_trans *tp; - int error; - - error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid, - 0, 0, 0, &tp); - if (error) - return error; - - xfs_ilock(ip, XFS_ILOCK_EXCL); - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); - - if (!(flags & XFS_PREALLOC_INVISIBLE)) { - VFS_I(ip)->i_mode &= ~S_ISUID; - if (VFS_I(ip)->i_mode & S_IXGRP) - VFS_I(ip)->i_mode &= ~S_ISGID; - xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); - } - - if (flags & XFS_PREALLOC_SET) - ip->i_diflags |= XFS_DIFLAG_PREALLOC; - if (flags & XFS_PREALLOC_CLEAR) - ip->i_diflags &= ~XFS_DIFLAG_PREALLOC; - - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - return xfs_trans_commit(tp); -} - /* * Fsync operations on directories are much simpler than on regular files, * as there is no file data to flush, and thus also no need for explicit diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 3fc6d77f5be9..b7e8f14d9fca 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -462,14 +462,6 @@ xfs_itruncate_extents( } /* from xfs_file.c */ -enum xfs_prealloc_flags { - XFS_PREALLOC_SET = (1 << 1), - XFS_PREALLOC_CLEAR = (1 << 2), - XFS_PREALLOC_INVISIBLE = (1 << 3), -}; - -int xfs_update_prealloc_flags(struct xfs_inode *ip, - enum xfs_prealloc_flags flags); int xfs_break_layouts(struct inode *inode, uint *iolock, enum layout_break_reason reason); diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c index ce6d66f20385..4abe17312c2b 100644 --- a/fs/xfs/xfs_pnfs.c +++ b/fs/xfs/xfs_pnfs.c @@ -70,6 +70,40 @@ xfs_fs_get_uuid( return 0; } +/* + * We cannot use file based VFS helpers such as file_modified() to update + * inode state as we modify the data/metadata in the inode here. Hence we have + * to open code the timestamp updates and SUID/SGID stripping. We also need + * to set the inode prealloc flag to ensure that the extents we allocate are not + * removed if the inode is reclaimed from memory before xfs_fs_block_commit() + * is from the client to indicate that data has been written and the file size + * can be extended. + */ +static int +xfs_fs_map_update_inode( + struct xfs_inode *ip) +{ + struct xfs_trans *tp; + int error; + + error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid, + 0, 0, 0, &tp); + if (error) + return error; + + xfs_ilock(ip, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); + + VFS_I(ip)->i_mode &= ~S_ISUID; + if (VFS_I(ip)->i_mode & S_IXGRP) + VFS_I(ip)->i_mode &= ~S_ISGID; + xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); + ip->i_diflags |= XFS_DIFLAG_PREALLOC; + + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + return xfs_trans_commit(tp); +} + /* * Get a layout for the pNFS client. */ @@ -164,7 +198,7 @@ xfs_fs_map_blocks( * that the blocks allocated and handed out to the client are * guaranteed to be present even after a server crash. */ - error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); + error = xfs_fs_map_update_inode(ip); if (!error) error = xfs_log_force_inode(ip); if (error) @@ -257,7 +291,7 @@ xfs_fs_commit_blocks( length = end - start; if (!length) continue; - + /* * Make sure reads through the pagecache see the new data. */ -- 2.33.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c 2022-01-31 6:43 ` [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c Dave Chinner @ 2022-01-31 17:37 ` Darrick J. Wong 0 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-01-31 17:37 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Mon, Jan 31, 2022 at 05:43:49PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > The operations that xfs_update_prealloc_flags() perform are now > unique to xfs_fs_map_blocks(), so move xfs_update_prealloc_flags() > to be a static function in xfs_pnfs.c and cut out all the > other functionality that is doesn't use anymore. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_file.c | 32 -------------------------------- > fs/xfs/xfs_inode.h | 8 -------- > fs/xfs/xfs_pnfs.c | 38 ++++++++++++++++++++++++++++++++++++-- > 3 files changed, 36 insertions(+), 42 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index ae6f5b15a023..ddc3336e8f84 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -66,38 +66,6 @@ xfs_is_falloc_aligned( > return !((pos | len) & mask); > } > > -int > -xfs_update_prealloc_flags( > - struct xfs_inode *ip, > - enum xfs_prealloc_flags flags) > -{ > - struct xfs_trans *tp; > - int error; > - > - error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid, > - 0, 0, 0, &tp); > - if (error) > - return error; > - > - xfs_ilock(ip, XFS_ILOCK_EXCL); > - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); > - > - if (!(flags & XFS_PREALLOC_INVISIBLE)) { > - VFS_I(ip)->i_mode &= ~S_ISUID; > - if (VFS_I(ip)->i_mode & S_IXGRP) > - VFS_I(ip)->i_mode &= ~S_ISGID; > - xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); > - } > - > - if (flags & XFS_PREALLOC_SET) > - ip->i_diflags |= XFS_DIFLAG_PREALLOC; > - if (flags & XFS_PREALLOC_CLEAR) > - ip->i_diflags &= ~XFS_DIFLAG_PREALLOC; > - > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > - return xfs_trans_commit(tp); > -} > - > /* > * Fsync operations on directories are much simpler than on regular files, > * as there is no file data to flush, and thus also no need for explicit > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 3fc6d77f5be9..b7e8f14d9fca 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -462,14 +462,6 @@ xfs_itruncate_extents( > } > > /* from xfs_file.c */ > -enum xfs_prealloc_flags { > - XFS_PREALLOC_SET = (1 << 1), > - XFS_PREALLOC_CLEAR = (1 << 2), > - XFS_PREALLOC_INVISIBLE = (1 << 3), > -}; > - > -int xfs_update_prealloc_flags(struct xfs_inode *ip, > - enum xfs_prealloc_flags flags); > int xfs_break_layouts(struct inode *inode, uint *iolock, > enum layout_break_reason reason); > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c > index ce6d66f20385..4abe17312c2b 100644 > --- a/fs/xfs/xfs_pnfs.c > +++ b/fs/xfs/xfs_pnfs.c > @@ -70,6 +70,40 @@ xfs_fs_get_uuid( > return 0; > } > > +/* > + * We cannot use file based VFS helpers such as file_modified() to update > + * inode state as we modify the data/metadata in the inode here. Hence we have Hmm. I'm a little fuzzy on the significance of "...as we modify the data/metadata" here. fallocate also modifies file contents and metadata, so why is pnfs special? Is it because pnfs doesn't check for freeze/ro state before calling ->map_blocks, and we don't want to stall on file_update_time? > + * to open code the timestamp updates and SUID/SGID stripping. We also need > + * to set the inode prealloc flag to ensure that the extents we allocate are not > + * removed if the inode is reclaimed from memory before xfs_fs_block_commit() > + * is from the client to indicate that data has been written and the file size "is called from the client" ? --D > + * can be extended. > + */ > +static int > +xfs_fs_map_update_inode( > + struct xfs_inode *ip) > +{ > + struct xfs_trans *tp; > + int error; > + > + error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid, > + 0, 0, 0, &tp); > + if (error) > + return error; > + > + xfs_ilock(ip, XFS_ILOCK_EXCL); > + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); > + > + VFS_I(ip)->i_mode &= ~S_ISUID; > + if (VFS_I(ip)->i_mode & S_IXGRP) > + VFS_I(ip)->i_mode &= ~S_ISGID; > + xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); > + ip->i_diflags |= XFS_DIFLAG_PREALLOC; > + > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > + return xfs_trans_commit(tp); > +} > + > /* > * Get a layout for the pNFS client. > */ > @@ -164,7 +198,7 @@ xfs_fs_map_blocks( > * that the blocks allocated and handed out to the client are > * guaranteed to be present even after a server crash. > */ > - error = xfs_update_prealloc_flags(ip, XFS_PREALLOC_SET); > + error = xfs_fs_map_update_inode(ip); > if (!error) > error = xfs_log_force_inode(ip); > if (error) > @@ -257,7 +291,7 @@ xfs_fs_commit_blocks( > length = end - start; > if (!length) > continue; > - > + > /* > * Make sure reads through the pagecache see the new data. > */ > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner ` (3 preceding siblings ...) 2022-01-31 6:43 ` [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c Dave Chinner @ 2022-01-31 6:43 ` Dave Chinner 2022-02-01 16:37 ` Darrick J. Wong 4 siblings, 1 reply; 22+ messages in thread From: Dave Chinner @ 2022-01-31 6:43 UTC (permalink / raw) To: linux-xfs From: "Darrick J. Wong" <djwong@kernel.org> Since we've started treating fallocate more like a file write, we should flush the log to disk if the user has asked for synchronous writes either by setting it via fcntl flags, or inode flags, or with the sync mount option. We've already got a helper for this, so use it. [Slightly massaged by <dchinner@redhat.com> to fit this patchset] Signed-off-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_file.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index ddc3336e8f84..209cba0f0ddc 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -861,6 +861,21 @@ xfs_break_layouts( return error; } +/* Does this file, inode, or mount want synchronous writes? */ +static inline bool xfs_file_sync_writes(struct file *filp) +{ + struct xfs_inode *ip = XFS_I(file_inode(filp)); + + if (xfs_has_wsync(ip->i_mount)) + return true; + if (filp->f_flags & (__O_SYNC | O_DSYNC)) + return true; + if (IS_SYNC(file_inode(filp))) + return true; + + return false; +} + #define XFS_FALLOC_FL_SUPPORTED \ (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | \ FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE | \ @@ -1045,7 +1060,7 @@ xfs_file_fallocate( if (do_file_insert) error = xfs_insert_file_space(ip, offset, len); - if (file->f_flags & O_DSYNC) + if (xfs_file_sync_writes(file)) error = xfs_log_force_inode(ip); out_unlock: @@ -1078,21 +1093,6 @@ xfs_file_fadvise( return ret; } -/* Does this file, inode, or mount want synchronous writes? */ -static inline bool xfs_file_sync_writes(struct file *filp) -{ - struct xfs_inode *ip = XFS_I(file_inode(filp)); - - if (xfs_has_wsync(ip->i_mount)) - return true; - if (filp->f_flags & (__O_SYNC | O_DSYNC)) - return true; - if (IS_SYNC(file_inode(filp))) - return true; - - return false; -} - STATIC loff_t xfs_file_remap_range( struct file *file_in, -- 2.33.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call 2022-01-31 6:43 ` [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call Dave Chinner @ 2022-02-01 16:37 ` Darrick J. Wong 0 siblings, 0 replies; 22+ messages in thread From: Darrick J. Wong @ 2022-02-01 16:37 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Mon, Jan 31, 2022 at 05:43:50PM +1100, Dave Chinner wrote: > From: "Darrick J. Wong" <djwong@kernel.org> > > Since we've started treating fallocate more like a file write, we > should flush the log to disk if the user has asked for synchronous > writes either by setting it via fcntl flags, or inode flags, or with > the sync mount option. We've already got a helper for this, so use > it. > > [Slightly massaged by <dchinner@redhat.com> to fit this patchset] I think you made more than 'slight massage' changes... > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_file.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index ddc3336e8f84..209cba0f0ddc 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -861,6 +861,21 @@ xfs_break_layouts( > return error; > } > > +/* Does this file, inode, or mount want synchronous writes? */ > +static inline bool xfs_file_sync_writes(struct file *filp) > +{ > + struct xfs_inode *ip = XFS_I(file_inode(filp)); > + > + if (xfs_has_wsync(ip->i_mount)) > + return true; > + if (filp->f_flags & (__O_SYNC | O_DSYNC)) > + return true; > + if (IS_SYNC(file_inode(filp))) > + return true; > + > + return false; > +} > + > #define XFS_FALLOC_FL_SUPPORTED \ > (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | \ > FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE | \ > @@ -1045,7 +1060,7 @@ xfs_file_fallocate( > if (do_file_insert) > error = xfs_insert_file_space(ip, offset, len); > > - if (file->f_flags & O_DSYNC) > + if (xfs_file_sync_writes(file)) > error = xfs_log_force_inode(ip); ...since the preceeding patches that you wrote enable simpler logic here. You've done all the (re)thinking necessary to get here, so I think on those grounds: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > out_unlock: > @@ -1078,21 +1093,6 @@ xfs_file_fadvise( > return ret; > } > > -/* Does this file, inode, or mount want synchronous writes? */ > -static inline bool xfs_file_sync_writes(struct file *filp) > -{ > - struct xfs_inode *ip = XFS_I(file_inode(filp)); > - > - if (xfs_has_wsync(ip->i_mount)) > - return true; > - if (filp->f_flags & (__O_SYNC | O_DSYNC)) > - return true; > - if (IS_SYNC(file_inode(filp))) > - return true; > - > - return false; > -} > - > STATIC loff_t > xfs_file_remap_range( > struct file *file_in, > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCHSET 0/3] xfs: fix permission drop and flushing in fallocate @ 2022-01-26 2:18 Darrick J. Wong 2022-01-26 2:18 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2022-01-26 2:18 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, linux-fsdevel Hi all, While auditing the file permission dropping for fallocate, I reached the conclusion that fallocate can modify file contents, and therefore should be treated as a file write. As such, it needs to update the file modification and file (metadata) change timestamps, and it needs to drop file privileges such as setuid and capabilities, just like a regular write. Moreover, if the inode is configured for synchronous writes, then all the fallocate changes really ought to be persisted to disk before fallocate returns to userspace. Unfortunately, the XFS fallocate implementation doesn't do this correctly. setgid without group-exec is a mandatory locking mark and is left alone by write(), which means that we shouldn't drop it unconditionally. Furthermore, file capabilities are another vector for setuid to be set on a program file, and XFS ignores these. I also noticed that fallocate doesn't flush the log to disk after fallocate when the fs is mounted with -o sync or if the DIFLAG_SYNC flag is set on the inode. Therefore, refactor the XFS fallocate implementation to use the VFS helper file_modified to update file metadata instead of open-coding it incorrectly. Refactor it further to use xfs_file_sync_writes to decide if we need to flush the log; and then fix the log flushing so that it flushes after we've made /all/ the changes. If you're going to start using this mess, you probably ought to just pull from my git trees, which are linked below. This is an extraordinary way to destroy everything. Enjoy! Comments and questions are, as always, welcome. --D kernel git tree: https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=falloc-fix-perm-updates-5.17 --- fs/xfs/xfs_file.c | 72 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 20 deletions(-) ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-26 2:18 [PATCHSET 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong @ 2022-01-26 2:18 ` Darrick J. Wong 2022-01-28 9:32 ` Chandan Babu R 0 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2022-01-26 2:18 UTC (permalink / raw) To: djwong; +Cc: linux-xfs, linux-fsdevel 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; - - 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. + */ + 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; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-26 2:18 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong @ 2022-01-28 9:32 ` Chandan Babu R 2022-01-28 22:23 ` Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: Chandan Babu R @ 2022-01-28 9:32 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, linux-fsdevel 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. > - 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. > + */ > + 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-28 9:32 ` Chandan Babu R @ 2022-01-28 22:23 ` Darrick J. Wong 2022-01-29 7:43 ` Chandan Babu R 0 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2022-01-28 22:23 UTC (permalink / raw) To: Chandan Babu R; +Cc: linux-xfs, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate 2022-01-28 22:23 ` Darrick J. Wong @ 2022-01-29 7:43 ` Chandan Babu R 0 siblings, 0 replies; 22+ messages in thread From: Chandan Babu R @ 2022-01-29 7:43 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-02-01 16:37 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-30 4:59 [PATCHSET v2 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong 2022-01-30 4:59 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong 2022-01-30 22:30 ` Dave Chinner 2022-01-30 4:59 ` [PATCH 2/3] xfs: flush log after fallocate for sync mounts and sync inodes Darrick J. Wong 2022-01-30 4:59 ` [PATCH 3/3] xfs: ensure log flush at the end of a synchronous fallocate call Darrick J. Wong 2022-01-30 21:59 ` Dave Chinner 2022-01-31 6:43 ` [PATCH 0/5] xfs: fallocate() vs xfs_update_prealloc_flags() Dave Chinner 2022-01-31 6:43 ` [PATCH 1/5] xfs: remove XFS_PREALLOC_SYNC Dave Chinner 2022-01-31 17:25 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 2/5] xfs: fallocate() should call file_modified() Dave Chinner 2022-01-31 17:27 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 3/5] xfs: set prealloc flag in xfs_alloc_file_space() Dave Chinner 2022-01-31 7:02 ` [PATCH v1.1 " Dave Chinner 2022-01-31 17:30 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 4/5] xfs: move xfs_update_prealloc_flags() to xfs_pnfs.c Dave Chinner 2022-01-31 17:37 ` Darrick J. Wong 2022-01-31 6:43 ` [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call Dave Chinner 2022-02-01 16:37 ` Darrick J. Wong -- strict thread matches above, loose matches on Subject: below -- 2022-01-26 2:18 [PATCHSET 0/3] xfs: fix permission drop and flushing in fallocate Darrick J. Wong 2022-01-26 2:18 ` [PATCH 1/3] xfs: use vfs helper to update file attributes after fallocate Darrick J. Wong 2022-01-28 9:32 ` Chandan Babu R 2022-01-28 22:23 ` Darrick J. Wong 2022-01-29 7:43 ` Chandan Babu R
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.