| Message ID | ae3d160d-6886-47a3-9179-de6becf0af38@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] io_uring fixes for 6.10-rc4 | expand |
On Fri, 14 Jun 2024 at 09:06, Jens Axboe <axboe@kernel.dk> wrote: > > - Ensure that the task state is correct before attempting to grab a > mutex This code is horrid. That code *also* does schedule(); __set_current_state(TASK_RUNNING); which makes no sense at all. If you just returned from schedule(), you *will* be running. The reason you need that __set_current_state(TASK_RUNNING); in the *other* place is the very fact that you didn't call schedule at all after doing a prepare_to_wait(&ctx->rsrc_quiesce_wq, &we, TASK_INTERRUPTIBLE); So the bug was that the code had the __set_current_state() in exactly the wrong place. But the fix didn't remove the bogus one, so it all looks entirely like voodoo. Linus
The pull request you sent on Fri, 14 Jun 2024 10:06:19 -0600:
> git://git.kernel.dk/linux.git tags/io_uring-6.10-20240614
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ac3cb72aea010510eaa1e19ab001a0d28c6eb4ab
Thank you!
On 6/14/24 12:29 PM, Linus Torvalds wrote: > On Fri, 14 Jun 2024 at 09:06, Jens Axboe <axboe@kernel.dk> wrote: >> >> - Ensure that the task state is correct before attempting to grab a >> mutex > > This code is horrid. > > That code *also* does > > schedule(); > __set_current_state(TASK_RUNNING); > > which makes no sense at all. If you just returned from schedule(), you > *will* be running. Yeah agree, not sure why that __set_current_state() is after schedule(), that's obviously not needed. > The reason you need that > > __set_current_state(TASK_RUNNING); > > in the *other* place is the very fact that you didn't call schedule at > all after doing a > > prepare_to_wait(&ctx->rsrc_quiesce_wq, &we, TASK_INTERRUPTIBLE); > > So the bug was that the code had the __set_current_state() in exactly > the wrong place. > > But the fix didn't remove the bogus one, so it all looks entirely like > voodoo. I'll kill that other redundant one.
On 6/14/24 6:22 PM, Jens Axboe wrote: >> The reason you need that >> >> __set_current_state(TASK_RUNNING); >> >> in the *other* place is the very fact that you didn't call schedule at >> all after doing a >> >> prepare_to_wait(&ctx->rsrc_quiesce_wq, &we, TASK_INTERRUPTIBLE); >> >> So the bug was that the code had the __set_current_state() in exactly >> the wrong place. >> >> But the fix didn't remove the bogus one, so it all looks entirely like >> voodoo. > > I'll kill that other redundant one. Honestly I think it's cleaner to kill both of them, and just call finish_wait() when we now we're going to break anyway. Yes that'll be an extra check after the break, but that doesn't matter. That's more readable than random __set_current_state() calls. Will do a separate patch once I reshuffle for -rc4 anyway.
Hi Linus, Two fixes from Pavel headed to stable: - Ensure that the task state is correct before attempting to grab a mutex - Split cancel sequence flag into a separate variable, as it can get set by someone not owning the request (but holding the ctx lock) Please pull! The following changes since commit 73254a297c2dd094abec7c9efee32455ae875bdf: io_uring: fix possible deadlock in io_register_iowq_max_workers() (2024-06-04 07:39:17 -0600) are available in the Git repository at: git://git.kernel.dk/linux.git tags/io_uring-6.10-20240614 for you to fetch changes up to f4a1254f2a076afb0edd473589bf40f9b4d36b41: io_uring: fix cancellation overwriting req->flags (2024-06-13 19:25:28 -0600) ---------------------------------------------------------------- io_uring-6.10-20240614 ---------------------------------------------------------------- Pavel Begunkov (2): io_uring/rsrc: don't lock while !TASK_RUNNING io_uring: fix cancellation overwriting req->flags include/linux/io_uring_types.h | 3 ++- io_uring/cancel.h | 4 ++-- io_uring/io_uring.c | 1 + io_uring/rsrc.c | 1 + 4 files changed, 6 insertions(+), 3 deletions(-)