* [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
@ 2025-01-11 10:59 Bui Quang Minh
2025-01-11 12:02 ` Pavel Begunkov
0 siblings, 1 reply; 6+ messages in thread
From: Bui Quang Minh @ 2025-01-11 10:59 UTC (permalink / raw)
To: linux-kernel
Cc: Bui Quang Minh, Jens Axboe, Pavel Begunkov, io-uring,
syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
Reported-by: Li Zetao <[email protected]>
Signed-off-by: Bui Quang Minh <[email protected]>
---
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;
@@ -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);
+ WARN_ON_ONCE(sqd && data_race(sqd->thread) != current);
if (!current->io_uring)
return;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
2025-01-11 10:59 [PATCH] io_uring: annotate sqd->thread access with data race in cancel path Bui Quang Minh
@ 2025-01-11 12:02 ` Pavel Begunkov
2025-01-11 13:57 ` Bui Quang Minh
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2025-01-11 12:02 UTC (permalink / raw)
To: Bui Quang Minh, linux-kernel
Cc: Jens Axboe, io-uring, syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
> Reported-by: Li Zetao <[email protected]>
> Signed-off-by: Bui Quang Minh <[email protected]>
> ---
> 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.
> @@ -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;
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
2025-01-11 12:02 ` Pavel Begunkov
@ 2025-01-11 13:57 ` Bui Quang Minh
2025-01-12 1:21 ` Pavel Begunkov
0 siblings, 1 reply; 6+ messages in thread
From: Bui Quang Minh @ 2025-01-11 13:57 UTC (permalink / raw)
To: Pavel Begunkov, linux-kernel
Cc: Jens Axboe, io-uring, syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
>> Reported-by: Li Zetao <[email protected]>
>> Signed-off-by: Bui Quang Minh <[email protected]>
>> ---
>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
2025-01-11 13:57 ` Bui Quang Minh
@ 2025-01-12 1:21 ` Pavel Begunkov
2025-01-12 9:36 ` Bui Quang Minh
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2025-01-12 1:21 UTC (permalink / raw)
To: Bui Quang Minh, linux-kernel
Cc: Jens Axboe, io-uring, syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
>>> Reported-by: Li Zetao <[email protected]>
>>> Signed-off-by: Bui Quang Minh <[email protected]>
>>> ---
>>> 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.
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.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
2025-01-12 1:21 ` Pavel Begunkov
@ 2025-01-12 9:36 ` Bui Quang Minh
2025-01-12 11:34 ` Pavel Begunkov
0 siblings, 1 reply; 6+ messages in thread
From: Bui Quang Minh @ 2025-01-12 9:36 UTC (permalink / raw)
To: Pavel Begunkov, linux-kernel
Cc: Jens Axboe, io-uring, syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
>>>> Reported-by: Li Zetao <[email protected]>
>>>> Signed-off-by: Bui Quang Minh <[email protected]>
>>>> ---
>>>> 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.
Thanks,
Quang Minh.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
2025-01-12 9:36 ` Bui Quang Minh
@ 2025-01-12 11:34 ` Pavel Begunkov
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2025-01-12 11:34 UTC (permalink / raw)
To: Bui Quang Minh, linux-kernel
Cc: Jens Axboe, io-uring, syzbot+3c750be01dab672c513d, Li Zetao
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: [email protected]
>>>>> Reported-by: Li Zetao <[email protected]>
>>>>> Signed-off-by: Bui Quang Minh <[email protected]>
>>>>> ---
>>>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-12 11:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-11 10:59 [PATCH] io_uring: annotate sqd->thread access with data race in cancel path Bui Quang Minh
2025-01-11 12:02 ` Pavel Begunkov
2025-01-11 13:57 ` Bui Quang Minh
2025-01-12 1:21 ` Pavel Begunkov
2025-01-12 9:36 ` Bui Quang Minh
2025-01-12 11:34 ` Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox