| Message ID | 20190529174318.22424-7-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for major copy_file_range() issues | expand |
On Wed, May 29, 2019 at 08:43:10PM +0300, Amir Goldstein wrote: > The combination of file_remove_privs() and file_update_mtime() is > quite common in filesystem ->write_iter() methods. > > Modelled after the helper file_accessed(), introduce file_modified() > and use it from generic_remap_file_range_prep(). > > Note that the order of calling file_remove_privs() before > file_update_mtime() in the helper was matched to the more common order by > filesystems and not the current order in generic_remap_file_range_prep(). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/inode.c | 20 ++++++++++++++++++++ > fs/read_write.c | 21 +++------------------ > include/linux/fs.h | 2 ++ > 3 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index df6542ec3b88..2885f2f2c7a5 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1899,6 +1899,26 @@ int file_update_time(struct file *file) > } > EXPORT_SYMBOL(file_update_time); > > +/* Caller must hold the file's inode lock */ > +int file_modified(struct file *file) > +{ > + int err; > + > + /* > + * Clear the security bits if the process is not being run by root. > + * This keeps people from modifying setuid and setgid binaries. > + */ > + err = file_remove_privs(file); > + if (err) > + return err; > + > + if (likely(file->f_mode & FMODE_NOCMTIME)) I would not have thought NOCMTIME is likely? Maybe it is for io requests coming from overlayfs, but for regular uses I don't think that's true. --D > + return 0; > + > + return file_update_time(file); > +} > +EXPORT_SYMBOL(file_modified); > + > int inode_needs_sync(struct inode *inode) > { > if (IS_SYNC(inode)) > diff --git a/fs/read_write.c b/fs/read_write.c > index b0fb1176b628..cec7e7b1f693 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1980,25 +1980,10 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, > return ret; > > /* If can't alter the file contents, we're done. */ > - if (!(remap_flags & REMAP_FILE_DEDUP)) { > - /* Update the timestamps, since we can alter file contents. */ > - if (!(file_out->f_mode & FMODE_NOCMTIME)) { > - ret = file_update_time(file_out); > - if (ret) > - return ret; > - } > + if (!(remap_flags & REMAP_FILE_DEDUP)) > + ret = file_modified(file_out); > > - /* > - * Clear the security bits if the process is not being run by > - * root. This keeps people from modifying setuid and setgid > - * binaries. > - */ > - ret = file_remove_privs(file_out); > - if (ret) > - return ret; > - } > - > - return 0; > + return ret; > } > EXPORT_SYMBOL(generic_remap_file_range_prep); > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index e4d382c4342a..79ffa2958bd8 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2177,6 +2177,8 @@ static inline void file_accessed(struct file *file) > touch_atime(&file->f_path); > } > > +extern int file_modified(struct file *file); > + > int sync_inode(struct inode *inode, struct writeback_control *wbc); > int sync_inode_metadata(struct inode *inode, int wait); > > -- > 2.17.1 >
On Wed, May 29, 2019 at 9:27 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Wed, May 29, 2019 at 08:43:10PM +0300, Amir Goldstein wrote: > > The combination of file_remove_privs() and file_update_mtime() is > > quite common in filesystem ->write_iter() methods. > > > > Modelled after the helper file_accessed(), introduce file_modified() > > and use it from generic_remap_file_range_prep(). > > > > Note that the order of calling file_remove_privs() before > > file_update_mtime() in the helper was matched to the more common order by > > filesystems and not the current order in generic_remap_file_range_prep(). > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/inode.c | 20 ++++++++++++++++++++ > > fs/read_write.c | 21 +++------------------ > > include/linux/fs.h | 2 ++ > > 3 files changed, 25 insertions(+), 18 deletions(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index df6542ec3b88..2885f2f2c7a5 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -1899,6 +1899,26 @@ int file_update_time(struct file *file) > > } > > EXPORT_SYMBOL(file_update_time); > > > > +/* Caller must hold the file's inode lock */ > > +int file_modified(struct file *file) > > +{ > > + int err; > > + > > + /* > > + * Clear the security bits if the process is not being run by root. > > + * This keeps people from modifying setuid and setgid binaries. > > + */ > > + err = file_remove_privs(file); > > + if (err) > > + return err; > > + > > + if (likely(file->f_mode & FMODE_NOCMTIME)) > > I would not have thought NOCMTIME is likely? > > Maybe it is for io requests coming from overlayfs, but for regular uses > I don't think that's true. Nope that's a typo. Good spotting. Overlayfs doesn't set FMODE_NOCMTIME (yet). Only xfs does from XFS_IOC_OPEN_BY_HANDLE, but I think Dave said that is a deprecated API. so should have been very_unlikely(). Thanks, Amir.
On Wed, May 29, 2019 at 10:08 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, May 29, 2019 at 9:27 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Wed, May 29, 2019 at 08:43:10PM +0300, Amir Goldstein wrote: > > > The combination of file_remove_privs() and file_update_mtime() is > > > quite common in filesystem ->write_iter() methods. > > > > > > Modelled after the helper file_accessed(), introduce file_modified() > > > and use it from generic_remap_file_range_prep(). > > > > > > Note that the order of calling file_remove_privs() before > > > file_update_mtime() in the helper was matched to the more common order by > > > filesystems and not the current order in generic_remap_file_range_prep(). > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/inode.c | 20 ++++++++++++++++++++ > > > fs/read_write.c | 21 +++------------------ > > > include/linux/fs.h | 2 ++ > > > 3 files changed, 25 insertions(+), 18 deletions(-) > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > index df6542ec3b88..2885f2f2c7a5 100644 > > > --- a/fs/inode.c > > > +++ b/fs/inode.c > > > @@ -1899,6 +1899,26 @@ int file_update_time(struct file *file) > > > } > > > EXPORT_SYMBOL(file_update_time); > > > > > > +/* Caller must hold the file's inode lock */ > > > +int file_modified(struct file *file) > > > +{ > > > + int err; > > > + > > > + /* > > > + * Clear the security bits if the process is not being run by root. > > > + * This keeps people from modifying setuid and setgid binaries. > > > + */ > > > + err = file_remove_privs(file); > > > + if (err) > > > + return err; > > > + > > > + if (likely(file->f_mode & FMODE_NOCMTIME)) > > > > I would not have thought NOCMTIME is likely? > > > > Maybe it is for io requests coming from overlayfs, but for regular uses > > I don't think that's true. > > Nope that's a typo. Good spotting. > Overlayfs doesn't set FMODE_NOCMTIME (yet). Only xfs does from > XFS_IOC_OPEN_BY_HANDLE, but I think Dave said that is a deprecated > API. so should have been very_unlikely(). > Is that an ACK with likely converted to unlikely? Thanks, Amir.
On Wed, May 29, 2019 at 10:08:44PM +0300, Amir Goldstein wrote: > On Wed, May 29, 2019 at 9:27 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Wed, May 29, 2019 at 08:43:10PM +0300, Amir Goldstein wrote: > > > The combination of file_remove_privs() and file_update_mtime() is > > > quite common in filesystem ->write_iter() methods. > > > > > > Modelled after the helper file_accessed(), introduce file_modified() > > > and use it from generic_remap_file_range_prep(). > > > > > > Note that the order of calling file_remove_privs() before > > > file_update_mtime() in the helper was matched to the more common order by > > > filesystems and not the current order in generic_remap_file_range_prep(). > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/inode.c | 20 ++++++++++++++++++++ > > > fs/read_write.c | 21 +++------------------ > > > include/linux/fs.h | 2 ++ > > > 3 files changed, 25 insertions(+), 18 deletions(-) > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > index df6542ec3b88..2885f2f2c7a5 100644 > > > --- a/fs/inode.c > > > +++ b/fs/inode.c > > > @@ -1899,6 +1899,26 @@ int file_update_time(struct file *file) > > > } > > > EXPORT_SYMBOL(file_update_time); > > > > > > +/* Caller must hold the file's inode lock */ > > > +int file_modified(struct file *file) > > > +{ > > > + int err; > > > + > > > + /* > > > + * Clear the security bits if the process is not being run by root. > > > + * This keeps people from modifying setuid and setgid binaries. > > > + */ > > > + err = file_remove_privs(file); > > > + if (err) > > > + return err; > > > + > > > + if (likely(file->f_mode & FMODE_NOCMTIME)) > > > > I would not have thought NOCMTIME is likely? > > > > Maybe it is for io requests coming from overlayfs, but for regular uses > > I don't think that's true. > > Nope that's a typo. Good spotting. > Overlayfs doesn't set FMODE_NOCMTIME (yet). Only xfs does from > XFS_IOC_OPEN_BY_HANDLE, but I think Dave said that is a deprecated > API. so should have been very_unlikely(). It is most definitely not a deprecated API. I don't know where you got that idea from. It's used explicitly by the xfs utilities to perform invisible IO. Anyone who runs xfs_fsr or xfsdump or has an application that links to libhandle is using XFS_IOC_OPEN_BY_HANDLE and FMODE_NOCMTIME.... -Dave.
diff --git a/fs/inode.c b/fs/inode.c index df6542ec3b88..2885f2f2c7a5 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1899,6 +1899,26 @@ int file_update_time(struct file *file) } EXPORT_SYMBOL(file_update_time); +/* Caller must hold the file's inode lock */ +int file_modified(struct file *file) +{ + int err; + + /* + * Clear the security bits if the process is not being run by root. + * This keeps people from modifying setuid and setgid binaries. + */ + err = file_remove_privs(file); + if (err) + return err; + + if (likely(file->f_mode & FMODE_NOCMTIME)) + return 0; + + return file_update_time(file); +} +EXPORT_SYMBOL(file_modified); + int inode_needs_sync(struct inode *inode) { if (IS_SYNC(inode)) diff --git a/fs/read_write.c b/fs/read_write.c index b0fb1176b628..cec7e7b1f693 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1980,25 +1980,10 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, return ret; /* If can't alter the file contents, we're done. */ - if (!(remap_flags & REMAP_FILE_DEDUP)) { - /* Update the timestamps, since we can alter file contents. */ - if (!(file_out->f_mode & FMODE_NOCMTIME)) { - ret = file_update_time(file_out); - if (ret) - return ret; - } + if (!(remap_flags & REMAP_FILE_DEDUP)) + ret = file_modified(file_out); - /* - * Clear the security bits if the process is not being run by - * root. This keeps people from modifying setuid and setgid - * binaries. - */ - ret = file_remove_privs(file_out); - if (ret) - return ret; - } - - return 0; + return ret; } EXPORT_SYMBOL(generic_remap_file_range_prep); diff --git a/include/linux/fs.h b/include/linux/fs.h index e4d382c4342a..79ffa2958bd8 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2177,6 +2177,8 @@ static inline void file_accessed(struct file *file) touch_atime(&file->f_path); } +extern int file_modified(struct file *file); + int sync_inode(struct inode *inode, struct writeback_control *wbc); int sync_inode_metadata(struct inode *inode, int wait);
The combination of file_remove_privs() and file_update_mtime() is quite common in filesystem ->write_iter() methods. Modelled after the helper file_accessed(), introduce file_modified() and use it from generic_remap_file_range_prep(). Note that the order of calling file_remove_privs() before file_update_mtime() in the helper was matched to the more common order by filesystems and not the current order in generic_remap_file_range_prep(). Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/inode.c | 20 ++++++++++++++++++++ fs/read_write.c | 21 +++------------------ include/linux/fs.h | 2 ++ 3 files changed, 25 insertions(+), 18 deletions(-)