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

[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 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/opdef.c             |  3 ++-
 io_uring/uring_cmd.c         | 10 +++++-----
 io_uring/uring_cmd.h         |  4 ----
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 0d5448c0b86c..61f97a398e9d 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 d3403c8216db..ff691f37462c 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -320,7 +320,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/opdef.c b/io_uring/opdef.c
index 3de75eca1c92..e8baef4e5146 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,
 	},
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index af842e9b4eb9..629cb4266da6 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))
@@ -260,7 +260,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 7dba0f1efc58..f6837ee0955b 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/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 61f97a398e9d..a65c7043078f 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 629cb4266da6..ce7726a04883 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

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

[lizetao]

Hi,

> -----Original Message-----
> From: Mark Harmstone <maharmstone@fb.com>
> Sent: Friday, January 3, 2025 11:02 PM
> To: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
> Cc: Jens Axboe <axboe@kernel.dk>
> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
> io_uring_cmd_data
> 
> 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
> 61f97a398e9d..a65c7043078f 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
> 629cb4266da6..ce7726a04883 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;

Why is op_data set to NULL here? If you are worried about some omissions, would it be
better to use WARN_ON to assert that op_data is a null pointer? This will also make it easier
to analyze the cause of the problem.

---
Li Zetao

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

[Jens Axboe]

On 1/6/25 5:47 AM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Mark Harmstone <maharmstone@fb.com>
>> Sent: Friday, January 3, 2025 11:02 PM
>> To: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>> io_uring_cmd_data
>>
>> 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
>> 61f97a398e9d..a65c7043078f 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
>> 629cb4266da6..ce7726a04883 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;
> 
> Why is op_data set to NULL here? If you are worried about some
> omissions, would it be better to use WARN_ON to assert that op_data is
> a null pointer? This will also make it easier to analyze the cause of
> the problem.

Clearing the per-op data is prudent when allocating getting this struct,
to avoid previous garbage. The alternative would be clearing it when
it's freed, either way is fine imho. A WARN_ON would not make sense, as
it can validly be non-NULL already.

-- 
Jens Axboe

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

[lizetao]

Hi,

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Monday, January 6, 2025 10:46 PM
> To: lizetao <lizetao1@huawei.com>; Mark Harmstone <maharmstone@fb.com>
> Cc: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
> Subject: Re: [PATCH 2/4] io_uring/cmd: add per-op data to struct
> io_uring_cmd_data
> 
> On 1/6/25 5:47 AM, lizetao wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Mark Harmstone <maharmstone@fb.com>
> >> Sent: Friday, January 3, 2025 11:02 PM
> >> To: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
> >> Cc: Jens Axboe <axboe@kernel.dk>
> >> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
> >> io_uring_cmd_data
> >>
> >> 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 61f97a398e9d..a65c7043078f
> >> 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
> >> 629cb4266da6..ce7726a04883 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;
> >
> > Why is op_data set to NULL here? If you are worried about some
> > omissions, would it be better to use WARN_ON to assert that op_data is
> > a null pointer? This will also make it easier to analyze the cause of
> > the problem.
> 
> Clearing the per-op data is prudent when allocating getting this struct, to avoid
> previous garbage. The alternative would be clearing it when it's freed, either
> way is fine imho. A WARN_ON would not make sense, as it can validly be non-
> NULL already.

I still can't fully understand, the usage logic of op_data should be as follows:
When applying for and initializing the cache, op_data has been set to NULL.
In io_req_uring_cleanup, the op_data memory will be released and set to NULL.
So if the cache in uring_cache, its op_data should be NULL? If it is non-NULL, is there
a risk of memory leak if it is directly set to null?
> 
> --
> Jens Axboe

---
Li Zetao

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

[Jens Axboe]

On 1/6/25 7:04 PM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Jens Axboe <axboe@kernel.dk>
>> Sent: Monday, January 6, 2025 10:46 PM
>> To: lizetao <lizetao1@huawei.com>; Mark Harmstone <maharmstone@fb.com>
>> Cc: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
>> Subject: Re: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>> io_uring_cmd_data
>>
>> On 1/6/25 5:47 AM, lizetao wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Mark Harmstone <maharmstone@fb.com>
>>>> Sent: Friday, January 3, 2025 11:02 PM
>>>> To: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>>>> io_uring_cmd_data
>>>>
>>>> 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 61f97a398e9d..a65c7043078f
>>>> 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
>>>> 629cb4266da6..ce7726a04883 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;
>>>
>>> Why is op_data set to NULL here? If you are worried about some
>>> omissions, would it be better to use WARN_ON to assert that op_data is
>>> a null pointer? This will also make it easier to analyze the cause of
>>> the problem.
>>
>> Clearing the per-op data is prudent when allocating getting this struct, to avoid
>> previous garbage. The alternative would be clearing it when it's freed, either
>> way is fine imho. A WARN_ON would not make sense, as it can validly be non-
>> NULL already.
> 
> I still can't fully understand, the usage logic of op_data should be
> as follows: When applying for and initializing the cache, op_data has
> been set to NULL. In io_req_uring_cleanup, the op_data memory will be
> released and set to NULL. So if the cache in uring_cache, its op_data
> should be NULL? If it is non-NULL, is there a risk of memory leak if
> it is directly set to null?

Ah forgot I did clear it for freeing. So yes, this NULL setting on the
alloc side is redundant. But let's just leave it for now, once this gets
merged with the alloc cache cleanups that are pending for 6.14, it'll go
away anyway.

-- 
Jens Axboe

[PATCH 3/4] io_uring: add io_uring_cmd_get_async_data helper

[Mark Harmstone]

Add a helper function in include/linux/io_uring/cmd.h to read the
async_data pointer from a struct io_uring_cmd.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---
 include/linux/io_uring/cmd.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index a65c7043078f..a3ce553413de 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -118,4 +118,9 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
 	return cmd_to_io_kiocb(cmd)->tctx->task;
 }
 
+static inline struct io_uring_cmd_data *io_uring_cmd_get_async_data(struct io_uring_cmd *cmd)
+{
+	return cmd_to_io_kiocb(cmd)->async_data;
+}
+
 #endif /* _LINUX_IO_URING_CMD_H */
-- 
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

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

[Jens Axboe]

On 1/3/25 8:02 AM, Mark Harmstone wrote:
> 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.

Looks fine to me, and we really should get this into 6.13. The encoded
reads are somewhat broken without it, violating the usual expectations
on how persistent passed in data should be.

-- 
Jens Axboe

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

[David Sterba]

On Fri, Jan 03, 2025 at 10:55:42AM -0700, Jens Axboe wrote:
> On 1/3/25 8:02 AM, Mark Harmstone wrote:
> > 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.
> 
> Looks fine to me, and we really should get this into 6.13. The encoded
> reads are somewhat broken without it, violating the usual expectations
> on how persistent passed in data should be.

Ok, I'll add the to the queue for the next RC.

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

[David Sterba]

On Fri, Jan 03, 2025 at 03:02:22PM +0000, Mark Harmstone wrote:
> 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).

V3 lacked the cover letter and it was not obvious if it's urgent fix,
new devlopemnent or a regular fix. Also it touched code outside of
btrfs, did not have any acks or word agreement that it would be ok to
take the fixes via btrfs tree.

[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 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