public inbox for [email protected]
 help / color / mirror / Atom feed
From: [email protected] (Eric W. Biederman)
To: Linus Torvalds <[email protected]>
Cc: Jens Axboe <[email protected]>, io-uring <[email protected]>,
	Linux Kernel Mailing List <[email protected]>,
	Oleg Nesterov <[email protected]>,
	[email protected]
Subject: Re: [PATCHSET 0/2] PF_IO_WORKER signal tweaks
Date: Sat, 20 Mar 2021 17:08:02 -0500	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHk-=wh3DCgezr5RKQ4Mqffoj-F4i47rp85Q4MSFRNhrr8tg3w@mail.gmail.com> (Linus Torvalds's message of "Sat, 20 Mar 2021 12:18:15 -0700")


Added criu because I just realized that io_uring (which can open files
from an io worker thread) looks to require some special handling for
stopping and freezing processes.  If not in the SIGSTOP case in the
related cgroup freezer case.

Linus Torvalds <[email protected]> writes:

> On Sat, Mar 20, 2021 at 10:51 AM Linus Torvalds
> <[email protected]> wrote:
>>
>> Alternatively, make it not use
>> CLONE_SIGHAND|CLONE_THREAD at all, but that would make it
>> unnecessarily allocate its own signal state, so that's "cleaner" but
>> not great either.
>
> Thinking some more about that, it would be problematic for things like
> the resource counters too. They'd be much better shared.
>
> Not adding it to the thread list etc might be clever, but feels a bit too scary.
>
> So on the whole I think Jens' minor patches to just not have IO helper
> threads accept signals are probably the right thing to do.

The way I see it we have two options:

1) Don't ask PF_IO_WORKERs to stop do_signal_stop and in
   task_join_group_stop.

   The easiest comprehensive implementation looks like just
   updating task_set_jobctl_pending to treat PF_IO_WORKER
   as it treats PF_EXITING.

2) Have the main loop of the kernel thread test for JOBCTL_STOP_PENDING
   and call into do_signal_stop.

It is a wee bit trickier to modify the io_workers to stop, but it does
not look prohibitively difficult.

All of the work performed by the io worker is work scheduled via
io_uring by the process being stopped.

- Is the amount of work performed by the io worker thread sufficiently
  negligible that we don't care?

- Or is the amount of work performed by the io worker so great that it
  becomes a way for an errant process to escape SIGSTOP?

As the code is all intermingled with the cgroup_freezer.  I am also
wondering creating checkpoints needs additional stopping guarantees.


To solve the issue that SIGSTOP is simply broken right now I am totally
fine with something like:

diff --git a/kernel/signal.c b/kernel/signal.c
index ba4d1ef39a9e..cb9acdfb32fa 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -288,7 +288,8 @@ bool task_set_jobctl_pending(struct task_struct *task, unsigned long mask)
 			JOBCTL_STOP_SIGMASK | JOBCTL_TRAPPING));
 	BUG_ON((mask & JOBCTL_TRAPPING) && !(mask & JOBCTL_PENDING_MASK));
 
-	if (unlikely(fatal_signal_pending(task) || (task->flags & PF_EXITING)))
+	if (unlikely(fatal_signal_pending(task) ||
+		     (task->flags & (PF_EXITING | PF_IO_WORKER))))
 		return false;
 
 	if (mask & JOBCTL_STOP_SIGMASK)



Which just keeps from creating unstoppable processes today.  I am just
not convinced that is what we want as a long term solution.

Eric

  reply	other threads:[~2021-03-20 22:10 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-20 15:38 [PATCHSET 0/2] PF_IO_WORKER signal tweaks Jens Axboe
2021-03-20 15:38 ` [PATCH 1/2] signal: don't allow sending any signals to PF_IO_WORKER threads Jens Axboe
2021-03-20 16:18   ` Eric W. Biederman
2021-03-20 17:56     ` Linus Torvalds
2021-03-20 21:38       ` Eric W. Biederman
2021-03-20 22:42         ` Jens Axboe
2021-03-21 14:54           ` Eric W. Biederman
2021-03-21 15:40             ` Jens Axboe
2021-03-20 15:38 ` [PATCH 2/2] signal: don't allow STOP on " Jens Axboe
2021-03-20 16:21   ` Eric W. Biederman
2021-03-22 16:18     ` Oleg Nesterov
2021-03-22 16:15   ` Oleg Nesterov
2021-03-20 16:26 ` [PATCHSET 0/2] PF_IO_WORKER signal tweaks Eric W. Biederman
2021-03-20 17:51   ` Linus Torvalds
2021-03-20 19:18     ` Linus Torvalds
2021-03-20 22:08       ` Eric W. Biederman [this message]
2021-03-20 22:53         ` Jens Axboe
2021-03-21 15:18           ` Eric W. Biederman
2021-03-21 15:42             ` Jens Axboe
2021-03-20 22:56       ` Jens Axboe
2021-03-20 17:05 ` kernel test robot
2021-03-20 17:05 ` kernel test robot
2021-03-20 19:10 ` kernel test robot
2021-03-22 16:05 ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox