| Message ID | 1cbbe72cf32c45a8fee96026463024cd8564a7d7.1736541357.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] io_uring: don't touch sqd->thread off tw add | expand |
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; > } >
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,
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; }
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(-)