[PATCH] io_uring/sqpoll: annotate data race for access in debug check

[PATCH] io_uring/sqpoll: annotate data race for access in debug check

[Bui Quang Minh]

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@syzkaller.appspotmail.com
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 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

RE: [PATCH] io_uring/sqpoll: annotate data race for access in debug check

[lizetao]

Hi,

> -----Original Message-----
> From: Bui Quang Minh <minhquangbui99@gmail.com>
> Sent: Wednesday, January 8, 2025 11:11 PM
> To: linux-kernel@vger.kernel.org
> Cc: Bui Quang Minh <minhquangbui99@gmail.com>; Jens Axboe
> <axboe@kernel.dk>; Pavel Begunkov <asml.silence@gmail.com>; io-
> uring@vger.kernel.org;
> syzbot+5988142e8a69a67b1418@syzkaller.appspotmail.com
> 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@syzkaller.appspotmail.com
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  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?
2、It is not holding sqd->lock in io_req_normal_work_add(), is it safe?

Thanks.
Reviewed-by: Li Zetao<lizetao1@huawei.com>

---
Li Zetao

Re: [PATCH] io_uring/sqpoll: annotate data race for access in debug check

[Bui Quang Minh]

On 1/8/25 23:24, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Bui Quang Minh <minhquangbui99@gmail.com>
>> Sent: Wednesday, January 8, 2025 11:11 PM
>> To: linux-kernel@vger.kernel.org
>> Cc: Bui Quang Minh <minhquangbui99@gmail.com>; Jens Axboe
>> <axboe@kernel.dk>; Pavel Begunkov <asml.silence@gmail.com>; io-
>> uring@vger.kernel.org;
>> syzbot+5988142e8a69a67b1418@syzkaller.appspotmail.com
>> 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@syzkaller.appspotmail.com
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   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.