Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
	Josef Bacik <josef@toxicpanda.com>,
	Christoph Hellwig <hch@lst.de>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org,
	kernel test robot <oliver.sang@intel.com>
Subject: Re: [PATCH] remap_range: merge do_clone_file_range() into vfs_clone_file_range()
Date: Fri, 2 Feb 2024 12:44:05 +0100	[thread overview]
Message-ID: <20240202114405.xvgo5zbrhhlskwqk@quack3> (raw)
In-Reply-To: <20240202102258.1582671-1-amir73il@gmail.com>

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