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()?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
