Re: [PATCH v2 01/12] aio-posix: fix race between io_uring CQE and AioHandler deletion

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

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 <stefanha@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]