Re: [PATCH v2 07/16] remap_range: move file_start_write() to after permission hook

[Jan Kara <jack@suse.cz>]

On Wed 22-11-23 14:27:06, Amir Goldstein wrote:
> In vfs code, file_start_write() is usually called after the permission
> hook in rw_verify_area().  vfs_dedupe_file_range_one() is an exception
> to this rule.
> 
> In vfs_dedupe_file_range_one(), move file_start_write() to after the
> the rw_verify_area() checks to make them "start-write-safe".
> 
> This is needed for fanotify "pre content" events.
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/remap_range.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index 42f79cb2b1b1..12131f2a6c9e 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -420,7 +420,7 @@ loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
>  EXPORT_SYMBOL(vfs_clone_file_range);
>  
>  /* Check whether we are allowed to dedupe the destination file */
> -static bool allow_file_dedupe(struct file *file)
> +static bool may_dedupe_file(struct file *file)
>  {
>  	struct mnt_idmap *idmap = file_mnt_idmap(file);
>  	struct inode *inode = file_inode(file);
> @@ -445,24 +445,29 @@ loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
>  	WARN_ON_ONCE(remap_flags & ~(REMAP_FILE_DEDUP |
>  				     REMAP_FILE_CAN_SHORTEN));
>  
> -	ret = mnt_want_write_file(dst_file);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * This is redundant if called from vfs_dedupe_file_range(), but other
>  	 * callers need it and it's not performance sesitive...
>  	 */
>  	ret = remap_verify_area(src_file, src_pos, len, false);
>  	if (ret)
> -		goto out_drop_write;
> +		return ret;
>  
>  	ret = remap_verify_area(dst_file, dst_pos, len, true);
>  	if (ret)
> -		goto out_drop_write;
> +		return ret;
> +
> +	/*
> +	 * This needs to be called after remap_verify_area() because of
> +	 * sb_start_write() and before may_dedupe_file() because the mount's
> +	 * MAY_WRITE need to be checked with mnt_get_write_access_file() held.
> +	 */
> +	ret = mnt_want_write_file(dst_file);
> +	if (ret)
> +		return ret;
>  
>  	ret = -EPERM;
> -	if (!allow_file_dedupe(dst_file))
> +	if (!may_dedupe_file(dst_file))
>  		goto out_drop_write;
>  
>  	ret = -EXDEV;
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR