Re: [PATCH 4/4] btrfs: don't read from userspace twice in btrfs_uring_encoded_read()

[Mark Harmstone <maharmstone@meta.com>]

Could someone please apply my kmemdup patch from last week so that this 
applies cleanly? "btrfs: use kmemdup in btrfs_uring_encoded_read"

On 20/11/24 16:02, Mark Harmstone wrote:
> If we return -EAGAIN the first time because we need to block,
> btrfs_uring_encoded_read() will get called twice. Take a copy of args
> the first time, to prevent userspace from messing around with it.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>   fs/btrfs/ioctl.c | 74 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 488dcd022dea..97f7812cbf7c 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4873,7 +4873,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   {
>   	size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args, flags);
>   	size_t copy_end;
> -	struct btrfs_ioctl_encoded_io_args args = { 0 };
> +	struct btrfs_ioctl_encoded_io_args *args;
>   	int ret;
>   	u64 disk_bytenr, disk_io_size;
>   	struct file *file;
> @@ -4888,6 +4888,9 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   	struct extent_state *cached_state = NULL;
>   	u64 start, lockend;
>   	void __user *sqe_addr;
> +	struct io_kiocb *req = cmd_to_io_kiocb(cmd);
> +	struct io_uring_cmd_data *data = req->async_data;
> +	bool need_copy = false;
>   
>   	if (!capable(CAP_SYS_ADMIN)) {
>   		ret = -EPERM;
> @@ -4899,34 +4902,55 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   	io_tree = &inode->io_tree;
>   	sqe_addr = u64_to_user_ptr(READ_ONCE(cmd->sqe->addr));
>   
> +	if (!data->op_data) {
> +		data->op_data = kzalloc(sizeof(*args), GFP_NOFS);
> +		if (!data->op_data) {
> +			ret = -ENOMEM;
> +			goto out_acct;
> +		}
> +
> +		need_copy = true;
> +	}
> +
> +	args = data->op_data;
> +
>   	if (issue_flags & IO_URING_F_COMPAT) {
>   #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
> -		struct btrfs_ioctl_encoded_io_args_32 args32;
> -
>   		copy_end = offsetofend(struct btrfs_ioctl_encoded_io_args_32, flags);
> -		if (copy_from_user(&args32, sqe_addr, copy_end)) {
> -			ret = -EFAULT;
> -			goto out_acct;
> +
> +		if (need_copy) {
> +			struct btrfs_ioctl_encoded_io_args_32 args32;
> +
> +			if (copy_from_user(&args32, sqe_addr, copy_end)) {
> +				ret = -EFAULT;
> +				goto out_acct;
> +			}
> +
> +			args->iov = compat_ptr(args32.iov);
> +			args->iovcnt = args32.iovcnt;
> +			args->offset = args32.offset;
> +			args->flags = args32.flags;
>   		}
> -		args.iov = compat_ptr(args32.iov);
> -		args.iovcnt = args32.iovcnt;
> -		args.offset = args32.offset;
> -		args.flags = args32.flags;
>   #else
>   		return -ENOTTY;
>   #endif
>   	} else {
>   		copy_end = copy_end_kernel;
> -		if (copy_from_user(&args, sqe_addr, copy_end)) {
> -			ret = -EFAULT;
> -			goto out_acct;
> +
> +		if (need_copy) {
> +			if (copy_from_user(args, sqe_addr, copy_end)) {
> +				ret = -EFAULT;
> +				goto out_acct;
> +			}
>   		}
>   	}
>   
> -	if (args.flags != 0)
> -		return -EINVAL;
> +	if (args->flags != 0) {
> +		ret = -EINVAL;
> +		goto out_acct;
> +	}
>   
> -	ret = import_iovec(ITER_DEST, args.iov, args.iovcnt, ARRAY_SIZE(iovstack),
> +	ret = import_iovec(ITER_DEST, args->iov, args->iovcnt, ARRAY_SIZE(iovstack),
>   			   &iov, &iter);
>   	if (ret < 0)
>   		goto out_acct;
> @@ -4936,8 +4960,8 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   		goto out_free;
>   	}
>   
> -	pos = args.offset;
> -	ret = rw_verify_area(READ, file, &pos, args.len);
> +	pos = args->offset;
> +	ret = rw_verify_area(READ, file, &pos, args->len);
>   	if (ret < 0)
>   		goto out_free;
>   
> @@ -4950,15 +4974,15 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   	start = ALIGN_DOWN(pos, fs_info->sectorsize);
>   	lockend = start + BTRFS_MAX_UNCOMPRESSED - 1;
>   
> -	ret = btrfs_encoded_read(&kiocb, &iter, &args, &cached_state,
> +	ret = btrfs_encoded_read(&kiocb, &iter, args, &cached_state,
>   				 &disk_bytenr, &disk_io_size);
>   	if (ret < 0 && ret != -EIOCBQUEUED)
>   		goto out_free;
>   
>   	file_accessed(file);
>   
> -	if (copy_to_user(sqe_addr + copy_end, (const char *)&args + copy_end_kernel,
> -			 sizeof(args) - copy_end_kernel)) {
> +	if (copy_to_user(sqe_addr + copy_end, (const char *)args + copy_end_kernel,
> +			 sizeof(*args) - copy_end_kernel)) {
>   		if (ret == -EIOCBQUEUED) {
>   			unlock_extent(io_tree, start, lockend, &cached_state);
>   			btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
> @@ -4975,7 +4999,7 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   		 * undo this.
>   		 */
>   		if (!iov) {
> -			iov = kmemdup(iovstack, sizeof(struct iovec) * args.iovcnt,
> +			iov = kmemdup(iovstack, sizeof(struct iovec) * args->iovcnt,
>   				      GFP_NOFS);
>   			if (!iov) {
>   				unlock_extent(io_tree, start, lockend, &cached_state);
> @@ -4988,13 +5012,13 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   		count = min_t(u64, iov_iter_count(&iter), disk_io_size);
>   
>   		/* Match ioctl by not returning past EOF if uncompressed. */
> -		if (!args.compression)
> -			count = min_t(u64, count, args.len);
> +		if (!args->compression)
> +			count = min_t(u64, count, args->len);
>   
>   		ret = btrfs_uring_read_extent(&kiocb, &iter, start, lockend,
>   					      cached_state, disk_bytenr,
>   					      disk_io_size, count,
> -					      args.compression, iov, cmd);
> +					      args->compression, iov, cmd);
>   
>   		goto out_acct;
>   	}