Message ID | 164316352961.2600373.9191916389107843284.stgit@magnolia (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xfs: fix permission drop and flushing in fallocate | expand |
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;
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
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;
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;