| Message ID | 20240202102258.1582671-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remap_range: merge do_clone_file_range() into vfs_clone_file_range() | expand |
On Fri 02-02-24 12:22:58, Amir Goldstein wrote: > commit dfad37051ade ("remap_range: move permission hooks out of > do_clone_file_range()") moved the permission hooks from > do_clone_file_range() out to its caller vfs_clone_file_range(), > but left all the fast sanity checks in do_clone_file_range(). > > This makes the expensive security hooks be called in situations > that they would not have been called before (e.g. fs does not support > clone). > > The only reason for the do_clone_file_range() helper was that overlayfs > did not use to be able to call vfs_clone_file_range() from copy up > context with sb_writers lock held. However, since commit c63e56a4a652 > ("ovl: do not open/llseek lower file with upper sb_writers held"), > overlayfs just uses an open coded version of vfs_clone_file_range(). > > Merge_clone_file_range() into vfs_clone_file_range(), restoring the > original order of checks as it was before the regressing commit and adapt > the overlayfs code to call vfs_clone_file_range() before the permission > hooks that were added by commit ca7ab482401c ("ovl: add permission hooks > outside of do_splice_direct()"). > > Note that in the merge of do_clone_file_range(), the file_start_write() > context was reduced to cover ->remap_file_range() without holding it > over the permission hooks, which was the reason for doing the regressing > commit in the first place. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202401312229.eddeb9a6-oliver.sang@intel.com > Fixes: dfad37051ade ("remap_range: move permission hooks out of do_clone_file_range()") > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Nice and looks good to me. One curious question below but feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > index b8e25ca51016..8586e2f5d243 100644 > --- a/fs/overlayfs/copy_up.c > +++ b/fs/overlayfs/copy_up.c > @@ -265,20 +265,18 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, > if (IS_ERR(old_file)) > return PTR_ERR(old_file); > > + /* Try to use clone_file_range to clone up within the same fs */ > + cloned = vfs_clone_file_range(old_file, 0, new_file, 0, len, 0); > + if (cloned == len) > + goto out_fput; > + > + /* Couldn't clone, so now we try to copy the data */ > error = rw_verify_area(READ, old_file, &old_pos, len); > if (!error) > error = rw_verify_area(WRITE, new_file, &new_pos, len); > if (error) > goto out_fput; Do we need to keep these rw_verify_area() checks here when vfs_clone_file_range() already did remap_verify_area()? Honza
On Fri, Feb 2, 2024 at 1:44 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 02-02-24 12:22:58, Amir Goldstein wrote: > > commit dfad37051ade ("remap_range: move permission hooks out of > > do_clone_file_range()") moved the permission hooks from > > do_clone_file_range() out to its caller vfs_clone_file_range(), > > but left all the fast sanity checks in do_clone_file_range(). > > > > This makes the expensive security hooks be called in situations > > that they would not have been called before (e.g. fs does not support > > clone). > > > > The only reason for the do_clone_file_range() helper was that overlayfs > > did not use to be able to call vfs_clone_file_range() from copy up > > context with sb_writers lock held. However, since commit c63e56a4a652 > > ("ovl: do not open/llseek lower file with upper sb_writers held"), > > overlayfs just uses an open coded version of vfs_clone_file_range(). > > > > Merge_clone_file_range() into vfs_clone_file_range(), restoring the > > original order of checks as it was before the regressing commit and adapt > > the overlayfs code to call vfs_clone_file_range() before the permission > > hooks that were added by commit ca7ab482401c ("ovl: add permission hooks > > outside of do_splice_direct()"). > > > > Note that in the merge of do_clone_file_range(), the file_start_write() > > context was reduced to cover ->remap_file_range() without holding it > > over the permission hooks, which was the reason for doing the regressing > > commit in the first place. > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Closes: https://lore.kernel.org/oe-lkp/202401312229.eddeb9a6-oliver.sang@intel.com > > Fixes: dfad37051ade ("remap_range: move permission hooks out of do_clone_file_range()") > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Nice and looks good to me. One curious question below but feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > index b8e25ca51016..8586e2f5d243 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -265,20 +265,18 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, > > if (IS_ERR(old_file)) > > return PTR_ERR(old_file); > > > > + /* Try to use clone_file_range to clone up within the same fs */ > > + cloned = vfs_clone_file_range(old_file, 0, new_file, 0, len, 0); > > + if (cloned == len) > > + goto out_fput; > > + > > + /* Couldn't clone, so now we try to copy the data */ > > error = rw_verify_area(READ, old_file, &old_pos, len); > > if (!error) > > error = rw_verify_area(WRITE, new_file, &new_pos, len); > > if (error) > > goto out_fput; > > Do we need to keep these rw_verify_area() checks here when > vfs_clone_file_range() already did remap_verify_area()? Yes, because in the common case of no clone support (e.g. ext4), the permission hooks in vfs_clone_file_range() will not be called. There is a corner case where fs supports clone, but for some reason rejects this specific clone request, although there is no apparent reason to reject a clone request for the entire file range. In that case, permission hooks will be called twice - no big deal - that is exactly like a fallback in userspace cp --reflink=auto that will end up calling permission hooks twice in this corner case. Thanks, Amir.
On Fri, 02 Feb 2024 12:22:58 +0200, Amir Goldstein wrote: > commit dfad37051ade ("remap_range: move permission hooks out of > do_clone_file_range()") moved the permission hooks from > do_clone_file_range() out to its caller vfs_clone_file_range(), > but left all the fast sanity checks in do_clone_file_range(). > > This makes the expensive security hooks be called in situations > that they would not have been called before (e.g. fs does not support > clone). > > [...] Fyi, this will probably only go in on Monday because I won't be around on Saturday. It'll be accompanied by a few fixes to the netfs library. --- Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.fixes [1/1] remap_range: merge do_clone_file_range() into vfs_clone_file_range() https://git.kernel.org/vfs/vfs/c/f7530a139f4c
On Fri 02-02-24 14:08:07, Amir Goldstein wrote: > On Fri, Feb 2, 2024 at 1:44 PM Jan Kara <jack@suse.cz> wrote: > > > + /* Try to use clone_file_range to clone up within the same fs */ > > > + cloned = vfs_clone_file_range(old_file, 0, new_file, 0, len, 0); > > > + if (cloned == len) > > > + goto out_fput; > > > + > > > + /* Couldn't clone, so now we try to copy the data */ > > > error = rw_verify_area(READ, old_file, &old_pos, len); > > > if (!error) > > > error = rw_verify_area(WRITE, new_file, &new_pos, len); > > > if (error) > > > goto out_fput; > > > > Do we need to keep these rw_verify_area() checks here when > > vfs_clone_file_range() already did remap_verify_area()? > > Yes, because in the common case of no clone support (e.g. ext4), > the permission hooks in vfs_clone_file_range() will not be called. > > There is a corner case where fs supports clone, but for some reason > rejects this specific clone request, although there is no apparent > reason to reject a clone request for the entire file range. > > In that case, permission hooks will be called twice - no big deal - > that is exactly like a fallback in userspace cp --reflink=auto that > will end up calling permission hooks twice in this corner case. I see. Thanks for explanation! Honza
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index b8e25ca51016..8586e2f5d243 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -265,20 +265,18 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, if (IS_ERR(old_file)) return PTR_ERR(old_file); + /* Try to use clone_file_range to clone up within the same fs */ + cloned = vfs_clone_file_range(old_file, 0, new_file, 0, len, 0); + if (cloned == len) + goto out_fput; + + /* Couldn't clone, so now we try to copy the data */ error = rw_verify_area(READ, old_file, &old_pos, len); if (!error) error = rw_verify_area(WRITE, new_file, &new_pos, len); if (error) goto out_fput; - /* Try to use clone_file_range to clone up within the same fs */ - ovl_start_write(dentry); - cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0); - ovl_end_write(dentry); - if (cloned == len) - goto out_fput; - /* Couldn't clone, so now we try to copy the data */ - /* Check if lower fs supports seek operation */ if (old_file->f_mode & FMODE_LSEEK) skip_hole = true; diff --git a/fs/remap_range.c b/fs/remap_range.c index f8c1120b8311..de07f978ce3e 100644 --- a/fs/remap_range.c +++ b/fs/remap_range.c @@ -373,9 +373,9 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, } EXPORT_SYMBOL(generic_remap_file_range_prep); -loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, - struct file *file_out, loff_t pos_out, - loff_t len, unsigned int remap_flags) +loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in, + struct file *file_out, loff_t pos_out, + loff_t len, unsigned int remap_flags) { loff_t ret; @@ -391,23 +391,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, if (!file_in->f_op->remap_file_range) return -EOPNOTSUPP; - ret = file_in->f_op->remap_file_range(file_in, pos_in, - file_out, pos_out, len, remap_flags); - if (ret < 0) - return ret; - - fsnotify_access(file_in); - fsnotify_modify(file_out); - return ret; -} -EXPORT_SYMBOL(do_clone_file_range); - -loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in, - struct file *file_out, loff_t pos_out, - loff_t len, unsigned int remap_flags) -{ - loff_t ret; - ret = remap_verify_area(file_in, pos_in, len, false); if (ret) return ret; @@ -417,10 +400,14 @@ loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in, return ret; file_start_write(file_out); - ret = do_clone_file_range(file_in, pos_in, file_out, pos_out, len, - remap_flags); + ret = file_in->f_op->remap_file_range(file_in, pos_in, + file_out, pos_out, len, remap_flags); file_end_write(file_out); + if (ret < 0) + return ret; + fsnotify_access(file_in); + fsnotify_modify(file_out); return ret; } EXPORT_SYMBOL(vfs_clone_file_range); diff --git a/include/linux/fs.h b/include/linux/fs.h index ed5966a70495..023f37c60709 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2101,9 +2101,6 @@ int __generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, struct file *file_out, loff_t pos_out, loff_t *count, unsigned int remap_flags); -extern loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, - struct file *file_out, loff_t pos_out, - loff_t len, unsigned int remap_flags); extern loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in, struct file *file_out, loff_t pos_out, loff_t len, unsigned int remap_flags);
commit dfad37051ade ("remap_range: move permission hooks out of do_clone_file_range()") moved the permission hooks from do_clone_file_range() out to its caller vfs_clone_file_range(), but left all the fast sanity checks in do_clone_file_range(). This makes the expensive security hooks be called in situations that they would not have been called before (e.g. fs does not support clone). The only reason for the do_clone_file_range() helper was that overlayfs did not use to be able to call vfs_clone_file_range() from copy up context with sb_writers lock held. However, since commit c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held"), overlayfs just uses an open coded version of vfs_clone_file_range(). Merge_clone_file_range() into vfs_clone_file_range(), restoring the original order of checks as it was before the regressing commit and adapt the overlayfs code to call vfs_clone_file_range() before the permission hooks that were added by commit ca7ab482401c ("ovl: add permission hooks outside of do_splice_direct()"). Note that in the merge of do_clone_file_range(), the file_start_write() context was reduced to cover ->remap_file_range() without holding it over the permission hooks, which was the reason for doing the regressing commit in the first place. Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202401312229.eddeb9a6-oliver.sang@intel.com Fixes: dfad37051ade ("remap_range: move permission hooks out of do_clone_file_range()") Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Christian, It was I who introduced do_clone_file_range() in commit 031a072a0b8a ("vfs: call vfs_clone_file_range() under freeze protection") 8 years ago. It was actually called vfs_clone_file_range() and later renamed to do_clone_file_range(), but it was always a bit of a hack. With recent changes to overlayfs, we can finally get rid of it and on the way, hopefully, solve the v6.8-rc1 performance regression reported by kernel test robot. Thanks, Amir. fs/overlayfs/copy_up.c | 14 ++++++-------- fs/remap_range.c | 31 +++++++++---------------------- include/linux/fs.h | 3 --- 3 files changed, 15 insertions(+), 33 deletions(-)