On 1/8/25 23:24, lizetao wrote:
Hi,
-----Original Message-----
From: Bui Quang Minh <minhquangbui99@xxxxxxxxx>
Sent: Wednesday, January 8, 2025 11:11 PM
To: linux-kernel@xxxxxxxxxxxxxxx
Cc: Bui Quang Minh <minhquangbui99@xxxxxxxxx>; Jens Axboe
<axboe@xxxxxxxxx>; Pavel Begunkov <asml.silence@xxxxxxxxx>; io-
uring@xxxxxxxxxxxxxxx;
syzbot+5988142e8a69a67b1418@xxxxxxxxxxxxxxxxxxxxxxxxx
Subject: [PATCH] io_uring/sqpoll: annotate data race for access in debug check
sqd->thread must only be access while holding sqd->lock. In
io_sq_thread_stop, the sqd->thread access to wake up the sq thread is placed
while holding sqd->lock, but the access in debug check is not. As this access if
for debug check only, we can safely ignore the data race here. So we annotate
this access with data_race to silence KCSAN.
Reported-by: syzbot+5988142e8a69a67b1418@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Bui Quang Minh <minhquangbui99@xxxxxxxxx>
---
io_uring/sqpoll.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c index
9e5bd79fd2b5..2088c56dbaa0 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -57,7 +57,7 @@ void io_sq_thread_park(struct io_sq_data *sqd)
void io_sq_thread_stop(struct io_sq_data *sqd) {
- WARN_ON_ONCE(sqd->thread == current);
+ WARN_ON_ONCE(data_race(sqd->thread) == current);
WARN_ON_ONCE(test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd-
state));
set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
--
2.43.0
The modification of this patch itself is fine, but there are two other things I need to confirm.
1、Does the io_uring_cancel_generic() require the same modification?
I think yes, there is another syzbot's bug report on data race on the
io_uring_cancel_generic I'm currently looking at. Here is the link:
https://syzkaller.appspot.com/bug?extid=3c750be01dab672c513d
2、It is not holding sqd->lock in io_req_normal_work_add(), is it safe?
This is a valid point, I think we should add lock here too. I will try
to write a proof-of-concept to validate this.
Thanks,
Quang Minh.
