From: Caleb Sander Mateos <csander@purestorage.com>
To: Jens Axboe <axboe@kernel.dk>
Cc: io-uring <io-uring@vger.kernel.org>
Subject: Re: [PATCH for-next] io_uring: add async data clear/free helpers
Date: Tue, 26 Aug 2025 08:27:09 -0700 [thread overview]
Message-ID: <CADUfDZqcgkpbdY5jH8UwQNc5RBi-QKR=sw2fsgALPFzKBNcbUw@mail.gmail.com> (raw)
In-Reply-To: <c93fcc03-ff41-4fe5-bea1-5fe3837eef73@kernel.dk>
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
>
>
next prev parent reply other threads:[~2025-08-26 15:27 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2025-08-26 16:04 ` Jens Axboe
2025-08-26 16:10 ` Caleb Sander Mateos
2025-08-26 18:40 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CADUfDZqcgkpbdY5jH8UwQNc5RBi-QKR=sw2fsgALPFzKBNcbUw@mail.gmail.com' \
--to=csander@purestorage.com \
--cc=axboe@kernel.dk \
--cc=io-uring@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).