[PATCH 1/4] io_uring/cmd: rename struct uring_cache to io_uring_cmd_data

[PATCH 1/4] io_uring/cmd: rename struct uring_cache to io_uring_cmd_data

[Mark Harmstone]

From: Jens Axboe <axboe@kernel.dk>

In preparation for making this more generically available for
->uring_cmd() usage that needs stable command data, rename it and move
it to io_uring/cmd.h instead.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/io_uring/cmd.h |  4 ++++
 io_uring/io_uring.c          |  2 +-
 io_uring/uring_cmd.c         | 10 +++++-----
 io_uring/uring_cmd.h         |  4 ----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index c189d36ad55e..24cff2b9b9d4 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -18,6 +18,10 @@ struct io_uring_cmd {
 	u8		pdu[32]; /* available inline for free use */
 };
 
+struct io_uring_cmd_data {
+	struct io_uring_sqe	sqes[2];
+};
+
 static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)
 {
 	return sqe->cmd;
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index b2736e3491b8..8ae6bf746fcc 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -315,7 +315,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	ret |= io_alloc_cache_init(&ctx->rw_cache, IO_ALLOC_CACHE_MAX,
 			    sizeof(struct io_async_rw));
 	ret |= io_alloc_cache_init(&ctx->uring_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct uring_cache));
+			    sizeof(struct io_uring_cmd_data));
 	spin_lock_init(&ctx->msg_lock);
 	ret |= io_alloc_cache_init(&ctx->msg_cache, IO_ALLOC_CACHE_MAX,
 			    sizeof(struct io_kiocb));
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index e2e8485932d6..eefc203a1214 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -16,10 +16,10 @@
 #include "rsrc.h"
 #include "uring_cmd.h"
 
-static struct uring_cache *io_uring_async_get(struct io_kiocb *req)
+static struct io_uring_cmd_data *io_uring_async_get(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
-	struct uring_cache *cache;
+	struct io_uring_cmd_data *cache;
 
 	cache = io_alloc_cache_get(&ctx->uring_cache);
 	if (cache) {
@@ -35,7 +35,7 @@ static struct uring_cache *io_uring_async_get(struct io_kiocb *req)
 static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
-	struct uring_cache *cache = req->async_data;
+	struct io_uring_cmd_data *cache = req->async_data;
 
 	if (issue_flags & IO_URING_F_UNLOCKED)
 		return;
@@ -183,7 +183,7 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 				   const struct io_uring_sqe *sqe)
 {
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
-	struct uring_cache *cache;
+	struct io_uring_cmd_data *cache;
 
 	cache = io_uring_async_get(req);
 	if (unlikely(!cache))
@@ -256,7 +256,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 
 	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
 	if (ret == -EAGAIN) {
-		struct uring_cache *cache = req->async_data;
+		struct io_uring_cmd_data *cache = req->async_data;
 
 		if (ioucmd->sqe != (void *) cache)
 			memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
diff --git a/io_uring/uring_cmd.h b/io_uring/uring_cmd.h
index a361f98664d2..515823ca68b8 100644
--- a/io_uring/uring_cmd.h
+++ b/io_uring/uring_cmd.h
@@ -1,9 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 
-struct uring_cache {
-	struct io_uring_sqe sqes[2];
-};
-
 int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags);
 int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 
-- 
2.45.2

[PATCH 2/4] io_uring: use sizeof for io_issue_defs[IORING_OP_URING_CMD].async_size

[Mark Harmstone]

Correct the value of io_issue_defs[IORING_OP_URING_CMD].async_size so
that it is derived from the size of the struct rather than being
calculated.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---
 io_uring/opdef.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index a2be3bbca5ff..c7746f67cc65 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -7,6 +7,7 @@
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/io_uring.h>
+#include <linux/io_uring/cmd.h>
 
 #include "io_uring.h"
 #include "opdef.h"
@@ -414,7 +415,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.plug			= 1,
 		.iopoll			= 1,
 		.iopoll_queue		= 1,
-		.async_size		= 2 * sizeof(struct io_uring_sqe),
+		.async_size		= sizeof(struct io_uring_cmd_data),
 		.prep			= io_uring_cmd_prep,
 		.issue			= io_uring_cmd,
 	},
-- 
2.45.2

Re: [PATCH 2/4] io_uring: use sizeof for io_issue_defs[IORING_OP_URING_CMD].async_size

[Jens Axboe]

On 11/20/24 9:02 AM, Mark Harmstone wrote:
> Correct the value of io_issue_defs[IORING_OP_URING_CMD].async_size so
> that it is derived from the size of the struct rather than being
> calculated.

You should probably just fold this with patch 1, where the type is
changed.

-- 
Jens Axboe

[PATCH 3/4] io_uring/cmd: add per-op data to struct io_uring_cmd_data

[Mark Harmstone]

From: Jens Axboe <axboe@kernel.dk>

In case an op handler for ->uring_cmd() needs stable storage for user
data, it can allocate io_uring_cmd_data->op_data and use it for the
duration of the request. When the request gets cleaned up, uring_cmd
will free it automatically.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/io_uring/cmd.h |  1 +
 io_uring/uring_cmd.c         | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 24cff2b9b9d4..3df6636ec3a3 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -20,6 +20,7 @@ struct io_uring_cmd {
 
 struct io_uring_cmd_data {
 	struct io_uring_sqe	sqes[2];
+	void			*op_data;
 };
 
 static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index eefc203a1214..019d6f49ff20 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -23,12 +23,16 @@ static struct io_uring_cmd_data *io_uring_async_get(struct io_kiocb *req)
 
 	cache = io_alloc_cache_get(&ctx->uring_cache);
 	if (cache) {
+		cache->op_data = NULL;
 		req->flags |= REQ_F_ASYNC_DATA;
 		req->async_data = cache;
 		return cache;
 	}
-	if (!io_alloc_async_data(req))
-		return req->async_data;
+	if (!io_alloc_async_data(req)) {
+		cache = req->async_data;
+		cache->op_data = NULL;
+		return cache;
+	}
 	return NULL;
 }
 
@@ -37,6 +41,11 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
 	struct io_uring_cmd_data *cache = req->async_data;
 
+	if (cache->op_data) {
+		kfree(cache->op_data);
+		cache->op_data = NULL;
+	}
+
 	if (issue_flags & IO_URING_F_UNLOCKED)
 		return;
 	if (io_alloc_cache_put(&req->ctx->uring_cache, cache)) {
-- 
2.45.2

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

[Mark Harmstone]

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;
 	}
-- 
2.45.2

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

[Mark Harmstone]

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;
>   	}

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

[Jens Axboe]

On 11/20/24 9:02 AM, 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;
> +	}

I'd probably get rid of this need_copy variable and just do the copy
here? Might look cleaner with an btrfs_alloc_copy_foo() helper, as you
could just do:

ret = btrfs_alloc_copy_foo(...);
if (unlikely(ret))
	return ret;

and hide all that ugly compat business outside the meat of this
function.

More of a style thing, the change itself looks fine.

-- 
Jens Axboe

[PATCH v4 0/4] btrfs: fix reading from userspace in btrfs_uring_encoded_read()

[Mark Harmstone]

Version 4 of mine and Jens' patches, to make sure that when our io_uring
function gets called a second time, it doesn't accidentally read
something from userspace that's gone out of scope or otherwise gotten
corrupted.

I sent a version 3 on December 17, but it looks like that got forgotten
about over Christmas (unsurprisingly). Version 4 fixes a problem that I
noticed, namely that we weren't taking a copy of the iovs, which also
necessitated creating a struct to store these things in. This does
simplify things by removing the need for the kmemdup, however.

I also have a patch for io_uring encoded writes ready to go, but it's
waiting on some of the stuff introduced here.

Jens Axboe (2):
  io_uring/cmd: rename struct uring_cache to io_uring_cmd_data
  io_uring/cmd: add per-op data to struct io_uring_cmd_data

Mark Harmstone (2):
  io_uring: add io_uring_cmd_get_async_data helper
  btrfs: don't read from userspace twice in btrfs_uring_encoded_read()

 fs/btrfs/ioctl.c             | 125 +++++++++++++++++++----------------
 include/linux/io_uring/cmd.h |  10 +++
 io_uring/io_uring.c          |   2 +-
 io_uring/opdef.c             |   3 +-
 io_uring/uring_cmd.c         |  23 +++++--
 io_uring/uring_cmd.h         |   4 --
 6 files changed, 97 insertions(+), 70 deletions(-)

-- 
2.45.2

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

[Mark Harmstone]

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 iovs, and the iter the first time, as by the time we are called the
second time these may have gone out of scope.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
Reported-by: Jens Axboe <axboe@kernel.dk>
Fixes: 34310c442e17 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
---
 fs/btrfs/ioctl.c | 125 ++++++++++++++++++++++++++---------------------
 1 file changed, 68 insertions(+), 57 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7872de140489..eb7b330d7aab 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4787,25 +4787,29 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
 	return ret;
 }
 
+struct btrfs_uring_encoded_data {
+	struct btrfs_ioctl_encoded_io_args args;
+	struct iovec iovstack[UIO_FASTIOV];
+	struct iovec *iov;
+	struct iov_iter iter;
+};
+
 static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
 	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 };
 	int ret;
 	u64 disk_bytenr, disk_io_size;
 	struct file *file;
 	struct btrfs_inode *inode;
 	struct btrfs_fs_info *fs_info;
 	struct extent_io_tree *io_tree;
-	struct iovec iovstack[UIO_FASTIOV];
-	struct iovec *iov = iovstack;
-	struct iov_iter iter;
 	loff_t pos;
 	struct kiocb kiocb;
 	struct extent_state *cached_state = NULL;
 	u64 start, lockend;
 	void __user *sqe_addr;
+	struct btrfs_uring_encoded_data *data = io_uring_cmd_get_async_data(cmd)->op_data;
 
 	if (!capable(CAP_SYS_ADMIN)) {
 		ret = -EPERM;
@@ -4819,43 +4823,64 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 
 	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;
-		}
-		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;
+	}
+
+	if (!data) {
+		data = kzalloc(sizeof(*data), GFP_NOFS);
+		if (!data) {
+			ret = -ENOMEM;
 			goto out_acct;
 		}
-	}
 
-	if (args.flags != 0)
-		return -EINVAL;
+		io_uring_cmd_get_async_data(cmd)->op_data = data;
 
-	ret = import_iovec(ITER_DEST, args.iov, args.iovcnt, ARRAY_SIZE(iovstack),
-			   &iov, &iter);
-	if (ret < 0)
-		goto out_acct;
+		if (issue_flags & IO_URING_F_COMPAT) {
+#if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
+			struct btrfs_ioctl_encoded_io_args_32 args32;
 
-	if (iov_iter_count(&iter) == 0) {
-		ret = 0;
-		goto out_free;
+			if (copy_from_user(&args32, sqe_addr, copy_end)) {
+				ret = -EFAULT;
+				goto out_acct;
+			}
+
+			data->args.iov = compat_ptr(args32.iov);
+			data->args.iovcnt = args32.iovcnt;
+			data->args.offset = args32.offset;
+			data->args.flags = args32.flags;
+#endif
+		} else {
+			if (copy_from_user(&data->args, sqe_addr, copy_end)) {
+				ret = -EFAULT;
+				goto out_acct;
+			}
+		}
+
+		if (data->args.flags != 0) {
+			ret = -EINVAL;
+			goto out_acct;
+		}
+
+		data->iov = data->iovstack;
+		ret = import_iovec(ITER_DEST, data->args.iov, data->args.iovcnt,
+				   ARRAY_SIZE(data->iovstack), &data->iov,
+				   &data->iter);
+		if (ret < 0)
+			goto out_acct;
+
+		if (iov_iter_count(&data->iter) == 0) {
+			ret = 0;
+			goto out_free;
+		}
 	}
 
-	pos = args.offset;
-	ret = rw_verify_area(READ, file, &pos, args.len);
+	pos = data->args.offset;
+	ret = rw_verify_area(READ, file, &pos, data->args.len);
 	if (ret < 0)
 		goto out_free;
 
@@ -4868,15 +4893,16 @@ 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,
-				 &disk_bytenr, &disk_io_size);
+	ret = btrfs_encoded_read(&kiocb, &data->iter, &data->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 *)&data->args + copy_end_kernel,
+			 sizeof(data->args) - copy_end_kernel)) {
 		if (ret == -EIOCBQUEUED) {
 			unlock_extent(io_tree, start, lockend, &cached_state);
 			btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
@@ -4886,39 +4912,24 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 	}
 
 	if (ret == -EIOCBQUEUED) {
-		u64 count;
-
-		/*
-		 * If we've optimized things by storing the iovecs on the stack,
-		 * undo this.
-		 */
-		if (!iov) {
-			iov = kmemdup(iovstack, sizeof(struct iovec) * args.iovcnt,
-				      GFP_NOFS);
-			if (!iov) {
-				unlock_extent(io_tree, start, lockend, &cached_state);
-				btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
-				ret = -ENOMEM;
-				goto out_acct;
-			}
-		}
-
-		count = min_t(u64, iov_iter_count(&iter), disk_io_size);
+		u64 count = min_t(u64, iov_iter_count(&data->iter),
+				  disk_io_size);
 
 		/* Match ioctl by not returning past EOF if uncompressed. */
-		if (!args.compression)
-			count = min_t(u64, count, args.len);
+		if (!data->args.compression)
+			count = min_t(u64, count, data->args.len);
 
-		ret = btrfs_uring_read_extent(&kiocb, &iter, start, lockend,
-					      cached_state, disk_bytenr,
-					      disk_io_size, count,
-					      args.compression, iov, cmd);
+		ret = btrfs_uring_read_extent(&kiocb, &data->iter, start,
+					      lockend, cached_state,
+					      disk_bytenr, disk_io_size, count,
+					      data->args.compression,
+					      data->iov, cmd);
 
 		goto out_acct;
 	}
 
 out_free:
-	kfree(iov);
+	kfree(data->iov);
 
 out_acct:
 	if (ret > 0)
-- 
2.45.2