[PATCH 1/1] io_uring: don't touch sqd->thread off tw add

[PATCH 1/1] io_uring: don't touch sqd->thread off tw add

[Pavel Begunkov]

With IORING_SETUP_SQPOLL all requests are created by the SQPOLL task,
which means that req->task should always match sqd->thread. Since
accesses to sqd->thread should be separately protected, use req->task
in io_req_normal_work_add() instead.

Note, in the eyes of io_req_normal_work_add(), the SQPOLL task struct
is always pinned and alive, and sqd->thread can either be the task or
NULL. It's only problematic if the compiler decides to reload the value
after the null check, which is not so likely.

Cc: stable@vger.kernel.org
Cc: Bui Quang Minh <minhquangbui99@gmail.com>
Reported-by: lizetao <lizetao1@huawei.com>
Fixes: 78f9b61bd8e54 ("io_uring: wake SQPOLL task when task_work is added to an empty queue")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index db198bd435b5..9b83b875d2e4 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1225,10 +1225,7 @@ static void io_req_normal_work_add(struct io_kiocb *req)
 
 	/* SQPOLL doesn't need the task_work added, it'll run it itself */
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
-		struct io_sq_data *sqd = ctx->sq_data;
-
-		if (sqd->thread)
-			__set_notify_signal(sqd->thread);
+		__set_notify_signal(tctx->task);
 		return;
 	}
 
-- 
2.47.1

Re: [PATCH 1/1] io_uring: don't touch sqd->thread off tw add

[Pavel Begunkov]

On 1/10/25 20:36, Pavel Begunkov wrote:
> With IORING_SETUP_SQPOLL all requests are created by the SQPOLL task,
> which means that req->task should always match sqd->thread. Since
> accesses to sqd->thread should be separately protected, use req->task
> in io_req_normal_work_add() instead.
> 
> Note, in the eyes of io_req_normal_work_add(), the SQPOLL task struct
> is always pinned and alive, and sqd->thread can either be the task or
> NULL. It's only problematic if the compiler decides to reload the value
> after the null check, which is not so likely.

We don't have much time to drag it on, let's fix it up
so it hopefully gets into 6.13


> Cc: stable@vger.kernel.org
> Cc: Bui Quang Minh <minhquangbui99@gmail.com>
> Reported-by: lizetao <lizetao1@huawei.com>
> Fixes: 78f9b61bd8e54 ("io_uring: wake SQPOLL task when task_work is added to an empty queue")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   io_uring/io_uring.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index db198bd435b5..9b83b875d2e4 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -1225,10 +1225,7 @@ static void io_req_normal_work_add(struct io_kiocb *req)
>   
>   	/* SQPOLL doesn't need the task_work added, it'll run it itself */
>   	if (ctx->flags & IORING_SETUP_SQPOLL) {
> -		struct io_sq_data *sqd = ctx->sq_data;
> -
> -		if (sqd->thread)
> -			__set_notify_signal(sqd->thread);
> +		__set_notify_signal(tctx->task);
>   		return;
>   	}
>   

-- 
Pavel Begunkov

Re: [PATCH 1/1] io_uring: don't touch sqd->thread off tw add

[Jens Axboe]

On Fri, 10 Jan 2025 20:36:45 +0000, Pavel Begunkov wrote:
> With IORING_SETUP_SQPOLL all requests are created by the SQPOLL task,
> which means that req->task should always match sqd->thread. Since
> accesses to sqd->thread should be separately protected, use req->task
> in io_req_normal_work_add() instead.
> 
> Note, in the eyes of io_req_normal_work_add(), the SQPOLL task struct
> is always pinned and alive, and sqd->thread can either be the task or
> NULL. It's only problematic if the compiler decides to reload the value
> after the null check, which is not so likely.
> 
> [...]

Applied, thanks!

[1/1] io_uring: don't touch sqd->thread off tw add
      commit: bd2703b42decebdcddf76e277ba76b4c4a142d73

Best regards,
-- 
Jens Axboe