Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path

[Bui Quang Minh]

On 1/11/25 19:02, Pavel Begunkov wrote:
> On 1/11/25 10:59, Bui Quang Minh wrote:
> > The sqd->thread access in io_uring_cancel_generic is just for debug check
> > so we can safely ignore the data race.
> >
> > The sqd->thread access in io_uring_try_cancel_requests is to check if the
> > caller is the sq threadi with the check ctx->sq_data->thread == 
> > current. In
> > case this is called in a task other than the sq thread, we expect the
> > expression to be false. And in that case, the sq_data->thread read can 
> > race
> > with the NULL write in the sq thread termination. However, the race will
> > still make ctx->sq_data->thread == current be false, so we can safely
> > ignore the data race.
> >
> > Reported-by: syzbot+3c750be01dab672c513d@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: Li Zetao <lizetao1@xxxxxxxxxx>
> > Signed-off-by: Bui Quang Minh <minhquangbui99@xxxxxxxxx>
> > ---
> >   io_uring/io_uring.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> > index ff691f37462c..b1a116620ae1 100644
> > --- a/io_uring/io_uring.c
> > +++ b/io_uring/io_uring.c
> > @@ -3094,9 +3094,18 @@ static __cold bool 
> > io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
> >           ret |= (cret != IO_WQ_CANCEL_NOTFOUND);
> >       }
> > -    /* SQPOLL thread does its own polling */
> > +    /*
> > +     * SQPOLL thread does its own polling
> > +     *
> > +     * We expect ctx->sq_data->thread == current to be false when
> > +     * this function is called on a task other than the sq thread.
> > +     * In that case, the sq_data->thread read can race with the
> > +     * NULL write in the sq thread termination. However, the race
> > +     * will still make ctx->sq_data->thread == current be false,
> > +     * so we can safely ignore the data race here.
> > +     */
> >       if ((!(ctx->flags & IORING_SETUP_SQPOLL) && cancel_all) ||
> > -        (ctx->sq_data && ctx->sq_data->thread == current)) {
> > +        (ctx->sq_data && data_race(ctx->sq_data->thread) == current)) {
> >           while (!wq_list_empty(&ctx->iopoll_list)) {
> >               io_iopoll_try_reap_events(ctx);
> >               ret = true;
>
> data_race() is a hammer we don't want to use to just silence warnings,
> it can hide real problems. The fact that it needs 6 lines of comments
> to explain is also not a good sign.
>
> Instead, you can pass a flag, i.e. io_uring_cancel_generic() will have
> non zero sqd IFF it's the SQPOLL task.

At first, I think of using READ_ONCE here and WRITE_ONCE in the sq 
thread termination to avoid the data race. What do you think about this 
approach?

Your proposed approach sounds good too.

> > @@ -3142,7 +3151,7 @@ __cold void io_uring_cancel_generic(bool 
> > cancel_all, struct io_sq_data *sqd)
> >       s64 inflight;
> >       DEFINE_WAIT(wait);
> > -    WARN_ON_ONCE(sqd && sqd->thread != current);
>
> It's not racing if it's the same thread, if it's not it'll trigger
> the warning anyway, I don't think we care about this one.
>
> > +    WARN_ON_ONCE(sqd && data_race(sqd->thread) != current);
> >       if (!current->io_uring)
> >           return;

Oh, thanks. I will remove this.

Thanks,
Quang Minh.