* [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
@ 2024-02-02 10:22 Amir Goldstein
2024-02-02 11:44 ` Jan Kara
2024-02-02 12:52 ` Christian Brauner
0 siblings, 2 replies; 5+ messages in thread
From: Amir Goldstein @ 2024-02-02 10:22 UTC (permalink / raw)
To: Christian Brauner
Cc: Jan Kara, Josef Bacik, Christoph Hellwig, Miklos Szeredi,
Al Viro, linux-fsdevel, kernel test robot
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(-)
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);
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
2024-02-02 10:22 [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range() Amir Goldstein
@ 2024-02-02 11:44 ` Jan Kara
2024-02-02 12:08 ` Amir Goldstein
2024-02-02 12:52 ` Christian Brauner
1 sibling, 1 reply; 5+ messages in thread
From: Jan Kara @ 2024-02-02 11:44 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Jan Kara, Josef Bacik, Christoph Hellwig,
Miklos Szeredi, Al Viro, linux-fsdevel, kernel test robot
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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
2024-02-02 11:44 ` Jan Kara
@ 2024-02-02 12:08 ` Amir Goldstein
2024-02-05 10:24 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Amir Goldstein @ 2024-02-02 12:08 UTC (permalink / raw)
To: Jan Kara
Cc: Christian Brauner, Josef Bacik, Christoph Hellwig,
Miklos Szeredi, Al Viro, linux-fsdevel, kernel test robot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
2024-02-02 12:08 ` Amir Goldstein
@ 2024-02-05 10:24 ` Jan Kara
0 siblings, 0 replies; 5+ messages in thread
From: Jan Kara @ 2024-02-05 10:24 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, Christian Brauner, Josef Bacik, Christoph Hellwig,
Miklos Szeredi, Al Viro, linux-fsdevel, kernel test robot
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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
2024-02-02 10:22 [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range() Amir Goldstein
2024-02-02 11:44 ` Jan Kara
@ 2024-02-02 12:52 ` Christian Brauner
1 sibling, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2024-02-02 12:52 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Jan Kara, Josef Bacik, Christoph Hellwig,
Miklos Szeredi, Al Viro, linux-fsdevel, kernel test robot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-05 10:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-02 10:22 [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range() Amir Goldstein
2024-02-02 11:44 ` Jan Kara
2024-02-02 12:08 ` Amir Goldstein
2024-02-05 10:24 ` Jan Kara
2024-02-02 12:52 ` Christian Brauner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).