On Wed, Jul 02, 2025 at 02:10:45PM +0200, Kevin Wolf wrote:
> 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?
Yes, the new flag cannot be set at the same time as ADD and is always
set together with REMOVE. I made that assumption in the code, which is a
bit ugly now that you mention it.
An assert is a good idea, that will make the code clearer and more
robust. Thanks!
>
> > @@ -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.
Will fix in v3.
Stefan
signature.asc
Description: PGP signature
