* [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