* [PATCH for-next] io_uring: add async data clear/free helpers
@ 2025-08-26 14:04 Jens Axboe
2025-08-26 15:27 ` Caleb Sander Mateos
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2025-08-26 14:04 UTC (permalink / raw)
To: io-uring
Futex recently had an issue where it mishandled how ->async_data and
REQ_F_ASYNC_DATA is handled. To avoid future issues like that, add a set
of helpers that either clear or clear-and-free the async data assigned
to a struct io_kiocb.
Convert existing manual handling of that to use the helpers. No intended
functional changes in this patch.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
diff --git a/io_uring/futex.c b/io_uring/futex.c
index 9113a44984f3..64f3bd51c84c 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -43,7 +43,6 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
static void __io_futex_complete(struct io_kiocb *req, io_tw_token_t tw)
{
- req->async_data = NULL;
hlist_del_init(&req->hash_node);
io_req_task_complete(req, tw);
}
@@ -54,6 +53,7 @@ static void io_futex_complete(struct io_kiocb *req, io_tw_token_t tw)
io_tw_lock(ctx, tw);
io_cache_free(&ctx->futex_cache, req->async_data);
+ io_req_async_data_clear(req, 0);
__io_futex_complete(req, tw);
}
@@ -72,8 +72,7 @@ static void io_futexv_complete(struct io_kiocb *req, io_tw_token_t tw)
io_req_set_res(req, res, 0);
}
- kfree(req->async_data);
- req->flags &= ~REQ_F_ASYNC_DATA;
+ io_req_async_data_free(req);
__io_futex_complete(req, tw);
}
@@ -232,9 +231,7 @@ int io_futexv_wait(struct io_kiocb *req, unsigned int issue_flags)
io_ring_submit_unlock(ctx, issue_flags);
req_set_fail(req);
io_req_set_res(req, ret, 0);
- kfree(futexv);
- req->async_data = NULL;
- req->flags &= ~REQ_F_ASYNC_DATA;
+ io_req_async_data_free(req);
return IOU_COMPLETE;
}
@@ -310,9 +307,7 @@ int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
if (ret < 0)
req_set_fail(req);
io_req_set_res(req, ret, 0);
- req->async_data = NULL;
- req->flags &= ~REQ_F_ASYNC_DATA;
- kfree(ifd);
+ io_req_async_data_free(req);
return IOU_COMPLETE;
}
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 2e4f7223a767..86613b8224bd 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -281,6 +281,19 @@ static inline bool req_has_async_data(struct io_kiocb *req)
return req->flags & REQ_F_ASYNC_DATA;
}
+static inline void io_req_async_data_clear(struct io_kiocb *req,
+ io_req_flags_t extra_flags)
+{
+ req->flags &= ~(REQ_F_ASYNC_DATA|extra_flags);
+ req->async_data = NULL;
+}
+
+static inline void io_req_async_data_free(struct io_kiocb *req)
+{
+ kfree(req->async_data);
+ io_req_async_data_clear(req, 0);
+}
+
static inline void io_put_file(struct io_kiocb *req)
{
if (!(req->flags & REQ_F_FIXED_FILE) && req->file)
diff --git a/io_uring/net.c b/io_uring/net.c
index b00cd2f59091..d2ca49ceb79d 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -178,10 +178,8 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags)
if (hdr->vec.nr > IO_VEC_CACHE_SOFT_CAP)
io_vec_free(&hdr->vec);
- if (io_alloc_cache_put(&req->ctx->netmsg_cache, hdr)) {
- req->async_data = NULL;
- req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
- }
+ if (io_alloc_cache_put(&req->ctx->netmsg_cache, hdr))
+ io_req_async_data_clear(req, REQ_F_NEED_CLEANUP);
}
static struct io_async_msghdr *io_msg_alloc_async(struct io_kiocb *req)
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 906e869d330a..dcde5bb7421a 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -154,10 +154,8 @@ static void io_rw_recycle(struct io_kiocb *req, unsigned int issue_flags)
if (rw->vec.nr > IO_VEC_CACHE_SOFT_CAP)
io_vec_free(&rw->vec);
- if (io_alloc_cache_put(&req->ctx->rw_cache, rw)) {
- req->async_data = NULL;
- req->flags &= ~REQ_F_ASYNC_DATA;
- }
+ if (io_alloc_cache_put(&req->ctx->rw_cache, rw))
+ io_req_async_data_clear(req, 0);
}
static void io_req_rw_cleanup(struct io_kiocb *req, unsigned int issue_flags)
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index ff1d029633b8..09f2a47a0020 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -37,8 +37,7 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
if (io_alloc_cache_put(&req->ctx->cmd_cache, ac)) {
ioucmd->sqe = NULL;
- req->async_data = NULL;
- req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
+ io_req_async_data_clear(req, 0);
}
}
diff --git a/io_uring/waitid.c b/io_uring/waitid.c
index e07a94694397..26c118f3918d 100644
--- a/io_uring/waitid.c
+++ b/io_uring/waitid.c
@@ -37,9 +37,7 @@ static void io_waitid_free(struct io_kiocb *req)
struct io_waitid_async *iwa = req->async_data;
put_pid(iwa->wo.wo_pid);
- kfree(req->async_data);
- req->async_data = NULL;
- req->flags &= ~REQ_F_ASYNC_DATA;
+ io_req_async_data_free(req);
}
static bool io_waitid_compat_copy_si(struct io_waitid *iw, int signo)
--
Jens Axboe
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] io_uring: add async data clear/free helpers
2025-08-26 14:04 [PATCH for-next] io_uring: add async data clear/free helpers Jens Axboe
@ 2025-08-26 15:27 ` Caleb Sander Mateos
2025-08-26 16:04 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Caleb Sander Mateos @ 2025-08-26 15:27 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
On Tue, Aug 26, 2025 at 7:16 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> Futex recently had an issue where it mishandled how ->async_data and
> REQ_F_ASYNC_DATA is handled. To avoid future issues like that, add a set
> of helpers that either clear or clear-and-free the async data assigned
> to a struct io_kiocb.
>
> Convert existing manual handling of that to use the helpers. No intended
> functional changes in this patch.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> ---
>
> diff --git a/io_uring/futex.c b/io_uring/futex.c
> index 9113a44984f3..64f3bd51c84c 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c
> @@ -43,7 +43,6 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
>
> static void __io_futex_complete(struct io_kiocb *req, io_tw_token_t tw)
> {
> - req->async_data = NULL;
> hlist_del_init(&req->hash_node);
> io_req_task_complete(req, tw);
> }
> @@ -54,6 +53,7 @@ static void io_futex_complete(struct io_kiocb *req, io_tw_token_t tw)
>
> io_tw_lock(ctx, tw);
> io_cache_free(&ctx->futex_cache, req->async_data);
> + io_req_async_data_clear(req, 0);
> __io_futex_complete(req, tw);
> }
>
> @@ -72,8 +72,7 @@ static void io_futexv_complete(struct io_kiocb *req, io_tw_token_t tw)
> io_req_set_res(req, res, 0);
> }
>
> - kfree(req->async_data);
> - req->flags &= ~REQ_F_ASYNC_DATA;
> + io_req_async_data_free(req);
> __io_futex_complete(req, tw);
> }
>
> @@ -232,9 +231,7 @@ int io_futexv_wait(struct io_kiocb *req, unsigned int issue_flags)
> io_ring_submit_unlock(ctx, issue_flags);
> req_set_fail(req);
> io_req_set_res(req, ret, 0);
> - kfree(futexv);
> - req->async_data = NULL;
> - req->flags &= ~REQ_F_ASYNC_DATA;
> + io_req_async_data_free(req);
> return IOU_COMPLETE;
> }
>
> @@ -310,9 +307,7 @@ int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
> if (ret < 0)
> req_set_fail(req);
> io_req_set_res(req, ret, 0);
> - req->async_data = NULL;
> - req->flags &= ~REQ_F_ASYNC_DATA;
> - kfree(ifd);
> + io_req_async_data_free(req);
> return IOU_COMPLETE;
> }
>
> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> index 2e4f7223a767..86613b8224bd 100644
> --- a/io_uring/io_uring.h
> +++ b/io_uring/io_uring.h
> @@ -281,6 +281,19 @@ static inline bool req_has_async_data(struct io_kiocb *req)
> return req->flags & REQ_F_ASYNC_DATA;
> }
>
> +static inline void io_req_async_data_clear(struct io_kiocb *req,
> + io_req_flags_t extra_flags)
> +{
> + req->flags &= ~(REQ_F_ASYNC_DATA|extra_flags);
> + req->async_data = NULL;
> +}
> +
> +static inline void io_req_async_data_free(struct io_kiocb *req)
> +{
> + kfree(req->async_data);
> + io_req_async_data_clear(req, 0);
> +}
Would it make sense to also add a helper for assigning async_data that
would also make sure to set REQ_F_ASYNC_DATA?
> +
> static inline void io_put_file(struct io_kiocb *req)
> {
> if (!(req->flags & REQ_F_FIXED_FILE) && req->file)
> diff --git a/io_uring/net.c b/io_uring/net.c
> index b00cd2f59091..d2ca49ceb79d 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -178,10 +178,8 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags)
> if (hdr->vec.nr > IO_VEC_CACHE_SOFT_CAP)
> io_vec_free(&hdr->vec);
>
> - if (io_alloc_cache_put(&req->ctx->netmsg_cache, hdr)) {
> - req->async_data = NULL;
> - req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
> - }
> + if (io_alloc_cache_put(&req->ctx->netmsg_cache, hdr))
> + io_req_async_data_clear(req, REQ_F_NEED_CLEANUP);
> }
>
> static struct io_async_msghdr *io_msg_alloc_async(struct io_kiocb *req)
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 906e869d330a..dcde5bb7421a 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -154,10 +154,8 @@ static void io_rw_recycle(struct io_kiocb *req, unsigned int issue_flags)
> if (rw->vec.nr > IO_VEC_CACHE_SOFT_CAP)
> io_vec_free(&rw->vec);
>
> - if (io_alloc_cache_put(&req->ctx->rw_cache, rw)) {
> - req->async_data = NULL;
> - req->flags &= ~REQ_F_ASYNC_DATA;
> - }
> + if (io_alloc_cache_put(&req->ctx->rw_cache, rw))
> + io_req_async_data_clear(req, 0);
> }
>
> static void io_req_rw_cleanup(struct io_kiocb *req, unsigned int issue_flags)
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index ff1d029633b8..09f2a47a0020 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -37,8 +37,7 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
>
> if (io_alloc_cache_put(&req->ctx->cmd_cache, ac)) {
> ioucmd->sqe = NULL;
> - req->async_data = NULL;
> - req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
> + io_req_async_data_clear(req, 0);
Looks like the REQ_F_NEED_CLEANUP got lost here. Other than that,
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> }
> }
>
> diff --git a/io_uring/waitid.c b/io_uring/waitid.c
> index e07a94694397..26c118f3918d 100644
> --- a/io_uring/waitid.c
> +++ b/io_uring/waitid.c
> @@ -37,9 +37,7 @@ static void io_waitid_free(struct io_kiocb *req)
> struct io_waitid_async *iwa = req->async_data;
>
> put_pid(iwa->wo.wo_pid);
> - kfree(req->async_data);
> - req->async_data = NULL;
> - req->flags &= ~REQ_F_ASYNC_DATA;
> + io_req_async_data_free(req);
> }
>
> static bool io_waitid_compat_copy_si(struct io_waitid *iw, int signo)
>
> --
> Jens Axboe
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] io_uring: add async data clear/free helpers
2025-08-26 15:27 ` Caleb Sander Mateos
@ 2025-08-26 16:04 ` Jens Axboe
2025-08-26 16:10 ` Caleb Sander Mateos
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2025-08-26 16:04 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: io-uring
On 8/26/25 9:27 AM, Caleb Sander Mateos wrote:
>> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
>> index 2e4f7223a767..86613b8224bd 100644
>> --- a/io_uring/io_uring.h
>> +++ b/io_uring/io_uring.h
>> @@ -281,6 +281,19 @@ static inline bool req_has_async_data(struct io_kiocb *req)
>> return req->flags & REQ_F_ASYNC_DATA;
>> }
>>
>> +static inline void io_req_async_data_clear(struct io_kiocb *req,
>> + io_req_flags_t extra_flags)
>> +{
>> + req->flags &= ~(REQ_F_ASYNC_DATA|extra_flags);
>> + req->async_data = NULL;
>> +}
>> +
>> +static inline void io_req_async_data_free(struct io_kiocb *req)
>> +{
>> + kfree(req->async_data);
>> + io_req_async_data_clear(req, 0);
>> +}
>
> Would it make sense to also add a helper for assigning async_data that
> would also make sure to set REQ_F_ASYNC_DATA?
I did consider that, but it's only futex that'd use it and just in those
two spots. So decided against it. At least for now.
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index ff1d029633b8..09f2a47a0020 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -37,8 +37,7 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
>>
>> if (io_alloc_cache_put(&req->ctx->cmd_cache, ac)) {
>> ioucmd->sqe = NULL;
>> - req->async_data = NULL;
>> - req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
>> + io_req_async_data_clear(req, 0);
>
> Looks like the REQ_F_NEED_CLEANUP got lost here. Other than that,
Oops indeed, added back.
> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
Thanks for the review!
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] io_uring: add async data clear/free helpers
2025-08-26 16:04 ` Jens Axboe
@ 2025-08-26 16:10 ` Caleb Sander Mateos
2025-08-26 18:40 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Caleb Sander Mateos @ 2025-08-26 16:10 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
On Tue, Aug 26, 2025 at 9:04 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 8/26/25 9:27 AM, Caleb Sander Mateos wrote:
> >> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> >> index 2e4f7223a767..86613b8224bd 100644
> >> --- a/io_uring/io_uring.h
> >> +++ b/io_uring/io_uring.h
> >> @@ -281,6 +281,19 @@ static inline bool req_has_async_data(struct io_kiocb *req)
> >> return req->flags & REQ_F_ASYNC_DATA;
> >> }
> >>
> >> +static inline void io_req_async_data_clear(struct io_kiocb *req,
> >> + io_req_flags_t extra_flags)
> >> +{
> >> + req->flags &= ~(REQ_F_ASYNC_DATA|extra_flags);
> >> + req->async_data = NULL;
> >> +}
> >> +
> >> +static inline void io_req_async_data_free(struct io_kiocb *req)
> >> +{
> >> + kfree(req->async_data);
> >> + io_req_async_data_clear(req, 0);
> >> +}
> >
> > Would it make sense to also add a helper for assigning async_data that
> > would also make sure to set REQ_F_ASYNC_DATA?
>
> I did consider that, but it's only futex that'd use it and just in those
> two spots. So decided against it. At least for now.
Makes sense. I think it could also be used in
io_uring_alloc_async_data(), but it's true there are fewer sites
assigning to async_data than clearing it.
Best,
Caleb
>
> >> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> >> index ff1d029633b8..09f2a47a0020 100644
> >> --- a/io_uring/uring_cmd.c
> >> +++ b/io_uring/uring_cmd.c
> >> @@ -37,8 +37,7 @@ static void io_req_uring_cleanup(struct io_kiocb *req, unsigned int issue_flags)
> >>
> >> if (io_alloc_cache_put(&req->ctx->cmd_cache, ac)) {
> >> ioucmd->sqe = NULL;
> >> - req->async_data = NULL;
> >> - req->flags &= ~(REQ_F_ASYNC_DATA|REQ_F_NEED_CLEANUP);
> >> + io_req_async_data_clear(req, 0);
> >
> > Looks like the REQ_F_NEED_CLEANUP got lost here. Other than that,
>
> Oops indeed, added back.
>
> > Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
>
> Thanks for the review!
>
> --
> Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] io_uring: add async data clear/free helpers
2025-08-26 16:10 ` Caleb Sander Mateos
@ 2025-08-26 18:40 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2025-08-26 18:40 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: io-uring
On 8/26/25 10:10 AM, Caleb Sander Mateos wrote:
> On Tue, Aug 26, 2025 at 9:04 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 8/26/25 9:27 AM, Caleb Sander Mateos wrote:
>>>> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
>>>> index 2e4f7223a767..86613b8224bd 100644
>>>> --- a/io_uring/io_uring.h
>>>> +++ b/io_uring/io_uring.h
>>>> @@ -281,6 +281,19 @@ static inline bool req_has_async_data(struct io_kiocb *req)
>>>> return req->flags & REQ_F_ASYNC_DATA;
>>>> }
>>>>
>>>> +static inline void io_req_async_data_clear(struct io_kiocb *req,
>>>> + io_req_flags_t extra_flags)
>>>> +{
>>>> + req->flags &= ~(REQ_F_ASYNC_DATA|extra_flags);
>>>> + req->async_data = NULL;
>>>> +}
>>>> +
>>>> +static inline void io_req_async_data_free(struct io_kiocb *req)
>>>> +{
>>>> + kfree(req->async_data);
>>>> + io_req_async_data_clear(req, 0);
>>>> +}
>>>
>>> Would it make sense to also add a helper for assigning async_data that
>>> would also make sure to set REQ_F_ASYNC_DATA?
>>
>> I did consider that, but it's only futex that'd use it and just in those
>> two spots. So decided against it. At least for now.
>
> Makes sense. I think it could also be used in
> io_uring_alloc_async_data(), but it's true there are fewer sites
> assigning to async_data than clearing it.
Indeed, most of the setting happens through the alloc async data
helper already. And yes it could get unified with that, but then
we'd probably want to extend that a bit more to return -ENOMEM
too for the cached case, and futex has one of each. IOW, I just
stopped at that point :-)
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-26 18:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-26 14:04 [PATCH for-next] io_uring: add async data clear/free helpers Jens Axboe
2025-08-26 15:27 ` Caleb Sander Mateos
2025-08-26 16:04 ` Jens Axboe
2025-08-26 16:10 ` Caleb Sander Mateos
2025-08-26 18:40 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox