Am 20.06.2025 um 02:08 hat Stefan Hajnoczi geschrieben:
> When an AioHandler is enqueued on ctx->submit_list for removal, the
> fill_sq_ring() function will submit an io_uring POLL_REMOVE operation to
> cancel the in-flight POLL_ADD operation.
>
> There is a race when another thread enqueues an AioHandler for deletion
> on ctx->submit_list when the POLL_ADD CQE has already appeared. In that
> case POLL_REMOVE is unnecessary. The code already handled this, but
> forgot that the AioHandler itself is still on ctx->submit_list when the
> POLL_ADD CQE is being processed. It's unsafe to delete the AioHandler at
> that point in time (use-after-free).
>
> Solve this problem by keeping the AioHandler alive but setting a flag so
> that it will be deleted by fill_sq_ring() when it runs.
>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> util/fdmon-io_uring.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
> index b0d68bdc44..2e40fff09a 100644
> --- a/util/fdmon-io_uring.c
> +++ b/util/fdmon-io_uring.c
> @@ -52,9 +52,10 @@ enum {
> FDMON_IO_URING_ENTRIES = 128, /* sq/cq ring size */
>
> /* AioHandler::flags */
> - FDMON_IO_URING_PENDING = (1 << 0),
> - FDMON_IO_URING_ADD = (1 << 1),
> - FDMON_IO_URING_REMOVE = (1 << 2),
> + FDMON_IO_URING_PENDING = (1 << 0),
> + FDMON_IO_URING_ADD = (1 << 1),
> + FDMON_IO_URING_REMOVE = (1 << 2),
> + FDMON_IO_URING_DELETE_AIO_HANDLER = (1 << 3),
> };
>
> static inline int poll_events_from_pfd(int pfd_events)
> @@ -218,6 +219,9 @@ static void fill_sq_ring(AioContext *ctx)
> if (flags & FDMON_IO_URING_REMOVE) {
> add_poll_remove_sqe(ctx, node);
> }
> + if (flags & FDMON_IO_URING_DELETE_AIO_HANDLER) {
> + QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node,
> node_deleted);
> + }
> }
> }
Why is it safe to add new SQEs for the node and then add it to
ctx->deleted_aio_handlers without waiting for the CQEs first? I
expected this to be the first check in the loop iteration and to
contain a 'continue;' statement.
The POLL_REMOVE case is clear when looking at more context, it doesn't
pass the node. As for POLL_ADD, I suppose both flags are actually never
set together in practice because FDMON_IO_URING_DELETE_AIO_HANDLER is
only set when processing the CQE of POLL_ADD, so no new POLL_ADD for the
same node will be pending yet. And checking the callers, I see that
adding is only ever done with newly allocated nodes, so something like
removing and re-adding the same node doesn't happen either.
Could we then assert that FDMON_IO_URING_DELETE_AIO_HANDLER is never
combined with FDMON_IO_URING_ADD, but always with FDMON_IO_URING_REMOVE,
to make the assumptions more explicit?
> @@ -347,10 +356,13 @@ void fdmon_io_uring_destroy(AioContext *ctx)
> unsigned flags = qatomic_fetch_and(&node->flags,
> ~(FDMON_IO_URING_PENDING |
> FDMON_IO_URING_ADD |
> - FDMON_IO_URING_REMOVE));
> + FDMON_IO_URING_REMOVE |
> + FDMON_IO_URING_DELETE_AIO_HANDLER));
>
> - if (flags & FDMON_IO_URING_REMOVE) {
> - QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node,
> node_deleted);
> + if ((flags & FDMON_IO_URING_REMOVE) ||
> + (flags & FDMON_IO_URING_DELETE_AIO_HANDLER)) {
If my conclusion above is right, FDMON_IO_URING_REMOVE will be set in
both cases, so checking FDMON_IO_URING_DELETE_AIO_HANDLER is redundant.
Maybe assert this, too, when setting FDMON_IO_URING_DELETE_AIO_HANDLER.
> + QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers,
> + node, node_deleted);
> }
>
> QSLIST_REMOVE_HEAD_RCU(&ctx->submit_list, node_submitted);
Kevin
