Re: [PATCH] io_uring: annotate sqd->thread access with data race in cancel path
From: Pavel Begunkov
Date: Sun Jan 12 2025 - 06:34:07 EST
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@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?
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