Re: [PATCHSET 0/2] PF_IO_WORKER signal tweaks

[Linus Torvalds]

On Sat, Mar 20, 2021 at 9:27 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>
> That makes me uneasy.  Because especially the SIGSTOP changes feels like
> it is the wrong thing semantically.  The group_send_sig_info change
> simply feels like it is unnecessary.

SIGSTOP handling is fundamentally done at signal handling time, and
signal handling is fundamentally done at "return to user space" time.

End result: you cannot send kernel threads any signals at all, unless
it _explicitly_ handles them manually.  SIGSTOP isn't different from
user space delivery of an "actual" signal handler in this respect.

And practically speaking, the only signal a kernel thread generally
can handle is SIGKILL (and exit on it).

Now, to make matters actually more confusing for SIGSTOP, it's a
two-phase operation - initiated from that usual "about to return to
user space with signals pending" logic (which doesn't happen for
kernel threads, including IO threads), _and_ then it has that magic
accounting for when to notify the parent about stoppage (which has
some across-thread handling).

I really think IO threads need to not participate, because they simply
cannot handle signals in any sane manner.

You should think of the IO threads as fully kernel threads that just
share VM and fs with the user thing.

In fact, it might be a good idea to disassociate them from the thread
group entirely when they are created, so that none of
"for_each_thread()" or "next_thread()" logic ever finds them.

Maybe the right thing to do is to add a new case to that whole thread
group initialization code in copy_process() - something like this fake
and intentionally whitespace-damaged pseudo-patch:

  diff --git a/kernel/fork.c b/kernel/fork.c
  index 54cc905e5fe0..b87abe3a9ac6 100644
  --- a/kernel/fork.c
  +++ b/kernel/fork.c
  @@ -2296,7 +2296,7 @@ static __latent_entropy struct task_struct
*copy_process(
                attach_pid(p, PIDTYPE_PGID);
                attach_pid(p, PIDTYPE_SID);
                __this_cpu_inc(process_counts);
  -     } else {
  +     } else if (!IOTHREAD) {
                current->signal->nr_threads++;
                atomic_inc(&current->signal->live);
                refcount_inc(&current->signal->sigcnt);

so that the IO thread isn't considered "really" part of the existing
signal state. 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.

Hmm.

            Linus