On Fri, Feb 2, 2024 at 1:44 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
> > Closes: https://lore.kernel.org/oe-lkp/202401312229.eddeb9a6-oliver.sang@xxxxxxxxx
> > Fixes: dfad37051ade ("remap_range: move permission hooks out of do_clone_file_range()")
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Nice and looks good to me. One curious question below but feel free to add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> > 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.
