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

[Bui Quang Minh <minhquangbui99@gmail.com>]

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@syzkaller.appspotmail.com
>> Reported-by: Li Zetao <lizetao1@huawei.com>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   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.