Re: [PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Jan Kara <jack@suse.cz>]

On Thu 30-11-23 16:16:24, Amir Goldstein wrote:
> nfsd/ksmbd call vfs_copy_file_range() with flag COPY_FILE_SPLICE to
> perform kernel copy between two files on any two filesystems.
> 
> Splicing input file, while holding file_start_write() on the output file
> which is on a different sb, posses a risk for fanotify related deadlocks.
> 
> We only need to call splice_file_range() from within the context of
> ->copy_file_range() filesystem methods with file_start_write() held.
> 
> To avoid the possible deadlocks, always use do_splice_direct() instead of
> splice_file_range() for the kernel copy fallback in vfs_copy_file_range()
> without holding file_start_write().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/read_write.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 0bc99f38e623..e0c2c1b5962b 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1421,6 +1421,10 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
>  				struct file *file_out, loff_t pos_out,
>  				size_t len, unsigned int flags)
>  {
> +	/* May only be called from within ->copy_file_range() methods */
> +	if (WARN_ON_ONCE(flags))
> +		return -EINVAL;
> +
>  	return splice_file_range(file_in, &pos_in, file_out, &pos_out,
>  				 min_t(size_t, len, MAX_RW_COUNT));
>  }
> @@ -1541,19 +1545,22 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  		ret = file_out->f_op->copy_file_range(file_in, pos_in,
>  						      file_out, pos_out,
>  						      len, flags);
> -		goto done;
> -	}
> -
> -	if (!splice && file_in->f_op->remap_file_range &&
> -	    file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
> +	} else if (!splice && file_in->f_op->remap_file_range &&
> +		   file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
>  		ret = file_in->f_op->remap_file_range(file_in, pos_in,
>  				file_out, pos_out,
>  				min_t(loff_t, MAX_RW_COUNT, len),
>  				REMAP_FILE_CAN_SHORTEN);
> -		if (ret > 0)
> -			goto done;
> +		/* fallback to splice */
> +		if (ret <= 0)
> +			splice = true;
>  	}
>  
> +	file_end_write(file_out);
> +
> +	if (!splice)
> +		goto done;
> +
>  	/*
>  	 * We can get here for same sb copy of filesystems that do not implement
>  	 * ->copy_file_range() in case filesystem does not support clone or in
> @@ -1565,11 +1572,16 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	 * and which filesystems do not, that will allow userspace tools to
>  	 * make consistent desicions w.r.t using copy_file_range().
>  	 *
> -	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE.
> +	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE
> +	 * for server-side-copy between any two sb.
> +	 *
> +	 * In any case, we call do_splice_direct() and not splice_file_range(),
> +	 * without file_start_write() held, to avoid possible deadlocks related
> +	 * to splicing from input file, while file_start_write() is held on
> +	 * the output file on a different sb.
>  	 */
> -	ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
> -				      flags);
> -
> +	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> +			       min_t(size_t, len, MAX_RW_COUNT), 0);
>  done:
>  	if (ret > 0) {
>  		fsnotify_access(file_in);
> @@ -1581,8 +1593,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	inc_syscr(current);
>  	inc_syscw(current);
>  
> -	file_end_write(file_out);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL(vfs_copy_file_range);
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR