* [GIT PULL] io_uring followup fixes for 5.12-rc4
@ 2021-03-21 16:38 Jens Axboe
2021-03-21 19:57 ` Linus Torvalds
2021-03-21 19:59 ` pr-tracker-bot
0 siblings, 2 replies; 4+ messages in thread
From: Jens Axboe @ 2021-03-21 16:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Eric W. Biederman, io-uring
Hi Linus,
Was planning on holding these for -rc5, but I think we may as well
flush them out. In this pull request:
- The SIGSTOP change from Eric, so we properly ignore that for
PF_IO_WORKER threads.
- Disallow sending signals to PF_IO_WORKER threads in general, we're not
interested in having them funnel back to the io_uring owning task.
- Stable fix from Stefan, ensuring we properly break links for short
send/sendmsg recv/recvmsg if MSG_WAITALL is set.
- Catch and loop when needing to run task_work before a PF_IO_WORKER
threads goes to sleep.
Please pull!
The following changes since commit de75a3d3f5a14c9ab3c4883de3471d3c92a8ee78:
io_uring: don't leak creds on SQO attach error (2021-03-18 09:44:35 -0600)
are available in the Git repository at:
git://git.kernel.dk/linux-block.git tags/io_uring-5.12-2021-03-21
for you to fetch changes up to 0031275d119efe16711cd93519b595e6f9b4b330:
io_uring: call req_set_fail_links() on short send[msg]()/recv[msg]() with MSG_WAITALL (2021-03-21 09:41:14 -0600)
----------------------------------------------------------------
io_uring-5.12-2021-03-21
----------------------------------------------------------------
Eric W. Biederman (1):
signal: don't allow STOP on PF_IO_WORKER threads
Jens Axboe (2):
signal: don't allow sending any signals to PF_IO_WORKER threads
io-wq: ensure task is running before processing task_work
Stefan Metzmacher (1):
io_uring: call req_set_fail_links() on short send[msg]()/recv[msg]() with MSG_WAITALL
fs/io-wq.c | 8 ++++++--
fs/io_uring.c | 24 ++++++++++++++++++++----
kernel/signal.c | 6 +++++-
3 files changed, 31 insertions(+), 7 deletions(-)
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [GIT PULL] io_uring followup fixes for 5.12-rc4
2021-03-21 16:38 [GIT PULL] io_uring followup fixes for 5.12-rc4 Jens Axboe
@ 2021-03-21 19:57 ` Linus Torvalds
2021-03-21 20:15 ` Jens Axboe
2021-03-21 19:59 ` pr-tracker-bot
1 sibling, 1 reply; 4+ messages in thread
From: Linus Torvalds @ 2021-03-21 19:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: Eric W. Biederman, io-uring
On Sun, Mar 21, 2021 at 9:38 AM Jens Axboe <[email protected]> wrote:
>
> - Catch and loop when needing to run task_work before a PF_IO_WORKER
> threads goes to sleep.
Hmm. The patch looks fine, but it makes me wonder: why does that code
use test_tsk_thread_flag() and clear_tsk_thread_flag() on current?
It should just use test_thread_flag() and clear_thread_flag().
Now it looks up "current" - which goes through the thread info - and
then looks up the thread from that. It's all kinds of stupid.
It should just have used the thread_info from the beginning, which is
what test_thread_flag() and clear_thread_flag() do.
I see the same broken pattern in both fs/io-wq.c (which is where I
noticed it when looking at the patch) and in fs/io-uring.c.
Please don't do "*_tsk_thread_flag(current, x)", when just
"*_thread_flag(x)" is simpler, and more efficient.
In fact, you should avoid *_tsk_thread_flag() as much as possible in general.
Thread flags should be considered mostly private to that thread - the
exceptions are generally some very low-level system stuff, ie core
signal handling and things like that.
So please change things like
if (test_tsk_thread_flag(current, TIF_NOTIFY_SIGNAL))
to
if (test_thread_flag(TIF_NOTIFY_SIGNAL))
etc.
And yes, we have a design mistake in a closely related area:
"signal_pending()" should *not* take the task pointer either, and we
should have the "current thread" separate from "another thread".
Maybe the "signal_pending(current)" makes people think it's a good
idea to pass in "current" to the thread flag checkers. We would have
been better off with "{fatal_,}signal_pending(void)" for the current
task, and "tsk_(fatal_,}signal_pending(tsk)" for the (very few) cases
of checking another task.
Because it really is all kinds of stupid (yes, often historical -
going all the way back to when 'current' was the main model - but now
stupid) to look up "current" to then look up thread data, when these
days, when the basic pattern is
#define current get_current()
#define get_current() (current_thread_info()->task)
ioe, the *thread_info* is the primary and quick thing, and "current"
is the indirection, and so if you see code that basically does
"task_thread_info()" on "current", it is literally going back and
forth between the two.
And yes, on architectures that use "THREAD_INFO_IN_TASK" (which does
include x86), the back-and-forth ends up being a non-issue (because
it's just offsets into containing structs) and it doesn't really
matter. But conceptually, patterns like "test_tsk_thread_flag(current,
x)" really are wrong, and on some architectures it generates
potentially *much* worse code.
Linus
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [GIT PULL] io_uring followup fixes for 5.12-rc4
2021-03-21 16:38 [GIT PULL] io_uring followup fixes for 5.12-rc4 Jens Axboe
2021-03-21 19:57 ` Linus Torvalds
@ 2021-03-21 19:59 ` pr-tracker-bot
1 sibling, 0 replies; 4+ messages in thread
From: pr-tracker-bot @ 2021-03-21 19:59 UTC (permalink / raw)
To: Jens Axboe; +Cc: Linus Torvalds, Eric W. Biederman, io-uring
The pull request you sent on Sun, 21 Mar 2021 10:38:04 -0600:
> git://git.kernel.dk/linux-block.git tags/io_uring-5.12-2021-03-21
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/2c41fab1c60b02626c8153a1806a7a1e5d62aaf1
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [GIT PULL] io_uring followup fixes for 5.12-rc4
2021-03-21 19:57 ` Linus Torvalds
@ 2021-03-21 20:15 ` Jens Axboe
0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2021-03-21 20:15 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Eric W. Biederman, io-uring
On 3/21/21 1:57 PM, Linus Torvalds wrote:
> On Sun, Mar 21, 2021 at 9:38 AM Jens Axboe <[email protected]> wrote:
>>
>> - Catch and loop when needing to run task_work before a PF_IO_WORKER
>> threads goes to sleep.
>
> Hmm. The patch looks fine, but it makes me wonder: why does that code
> use test_tsk_thread_flag() and clear_tsk_thread_flag() on current?
>
> It should just use test_thread_flag() and clear_thread_flag().
>
> Now it looks up "current" - which goes through the thread info - and
> then looks up the thread from that. It's all kinds of stupid.
>
> It should just have used the thread_info from the beginning, which is
> what test_thread_flag() and clear_thread_flag() do.
>
> I see the same broken pattern in both fs/io-wq.c (which is where I
> noticed it when looking at the patch) and in fs/io-uring.c.
>
> Please don't do "*_tsk_thread_flag(current, x)", when just
> "*_thread_flag(x)" is simpler, and more efficient.
>
> In fact, you should avoid *_tsk_thread_flag() as much as possible in general.
>
> Thread flags should be considered mostly private to that thread - the
> exceptions are generally some very low-level system stuff, ie core
> signal handling and things like that.
>
> So please change things like
>
> if (test_tsk_thread_flag(current, TIF_NOTIFY_SIGNAL))
>
> to
>
> if (test_thread_flag(TIF_NOTIFY_SIGNAL))
>
> etc.
>
> And yes, we have a design mistake in a closely related area:
> "signal_pending()" should *not* take the task pointer either, and we
> should have the "current thread" separate from "another thread".
>
> Maybe the "signal_pending(current)" makes people think it's a good
> idea to pass in "current" to the thread flag checkers. We would have
> been better off with "{fatal_,}signal_pending(void)" for the current
> task, and "tsk_(fatal_,}signal_pending(tsk)" for the (very few) cases
> of checking another task.
>
> Because it really is all kinds of stupid (yes, often historical -
> going all the way back to when 'current' was the main model - but now
> stupid) to look up "current" to then look up thread data, when these
> days, when the basic pattern is
>
> #define current get_current()
> #define get_current() (current_thread_info()->task)
>
> ioe, the *thread_info* is the primary and quick thing, and "current"
> is the indirection, and so if you see code that basically does
> "task_thread_info()" on "current", it is literally going back and
> forth between the two.
>
> And yes, on architectures that use "THREAD_INFO_IN_TASK" (which does
> include x86), the back-and-forth ends up being a non-issue (because
> it's just offsets into containing structs) and it doesn't really
> matter. But conceptually, patterns like "test_tsk_thread_flag(current,
> x)" really are wrong, and on some architectures it generates
> potentially *much* worse code.
Thanks, that's useful information, I guess it just ended up being used
by chance and I didn't realize it made a difference for some archs. I'll
change these, and I also think that io-wq should be a bit nicer and use
tracehook_notify_signal() if TIF_NOTIFY_SIGNAL is set. Doesn't matter
now, but very well might in the future when TIF_NOTIFY_SIGNAL gets
used for more than just task_work notifications.
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-21 20:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-21 16:38 [GIT PULL] io_uring followup fixes for 5.12-rc4 Jens Axboe
2021-03-21 19:57 ` Linus Torvalds
2021-03-21 20:15 ` Jens Axboe
2021-03-21 19:59 ` pr-tracker-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox