| Message ID | 20250103150233.2340306-3-maharmstone@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: fix reading from userspace in btrfs_uring_encoded_read() | expand |
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
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.
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
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.
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)) {