[GIT PULL] io_uring fixes for 6.10-rc4

[GIT PULL] io_uring fixes for 6.10-rc4

[Jens Axboe]

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(-)

-- 
Jens Axboe

Re: [GIT PULL] io_uring fixes for 6.10-rc4

[Linus Torvalds]

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

Re: [GIT PULL] io_uring fixes for 6.10-rc4

[pr-tracker-bot]

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!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] io_uring fixes for 6.10-rc4

[Jens Axboe]

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.

-- 
Jens Axboe

Re: [GIT PULL] io_uring fixes for 6.10-rc4

[Jens Axboe]

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.

-- 
Jens Axboe