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

[Pavel Begunkov <asml.silence@gmail.com>]

On 1/12/25 09:36, Bui Quang Minh wrote:
> On 1/12/25 08:21, Pavel Begunkov wrote:
>> On 1/11/25 13:57, Bui Quang Minh wrote:
>>> 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?
>>
>> Same thing, that'd be complicating synchronisation when there
>> shouldn't be any races in the first place. Having no races is
>> easier than wrapping them into READ_ONCE and keeping in mind
>> what that's even fine.
> 
> Okay, I'll send another patch with a new flag for the cancel path.
> 
>> Btw, the line you're changing doesn't even look right. SQPOLL
>> clears sqd->task right before starting with cancellations, so
>> sounds like it's mindlessly comparing NULL == current.
> 
> Hmm, I think it's correct but quite easy to get confused here. In the io_sq_thread, we explicitly call io_uring_cancel_generic before setting sqd->thread = NULL. The later io_uring_cancel_generic call in do_exit actually does nothing as we already set the task_struct->io_uring to NULL in the previous call.

Yeah, you're right, mixed it up with normal user task
cancellation, which happen in the exit path.

-- 
Pavel Begunkov