Re: [PATCH v3 4/7] linux/signal.h: Ignore SIGINFO by default in new tasks

[Christian Brauner <christian.brauner@ubuntu.com>]

On Thu, Apr 30, 2020 at 05:37:28PM +1000, Aleksa Sarai wrote:
> On 2020-04-30, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > On Thu, Apr 30, 2020 at 08:53:56AM +0200, Jiri Slaby wrote:
> > > On 30. 04. 20, 8:42, Arseny Maslennikov wrote:
> > > > This matches the behaviour of other Unix-like systems that have SIGINFO
> > > > and causes less harm to processes that do not install handlers for this
> > > > signal, making the keyboard status character non-fatal for them.
> > > > 
> > > > This is implemented with the assumption that SIGINFO is defined
> > > > to be equivalent to SIGPWR; still, there is no reason for PWR to
> > > > result in termination of the signal recipient anyway — it does not
> > > > indicate there is a fatal problem with the recipient's execution
> > > > context (like e.g. FPE/ILL do), and we have TERM/KILL for explicit
> > > > termination requests.
> > > > 
> > > > To put it another way:
> > > > The only scenario where system behaviour actually changes is when the
> > > > signal recipient has default disposition for SIGPWR. If a process
> > > > chose to interpret a SIGPWR as an incentive to cleanly terminate, it
> > > > would supply its own handler — and this commit does not affect processes
> > > > with non-default handlers.
> > > > 
> > > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > > ---
> > > >  include/linux/signal.h | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/linux/signal.h b/include/linux/signal.h
> > > > index 05bacd2ab..dc31da8fc 100644
> > > > --- a/include/linux/signal.h
> > > > +++ b/include/linux/signal.h
> > > > @@ -369,7 +369,7 @@ extern bool unhandled_signal(struct task_struct *tsk, int sig);
> > > >   *	|  SIGSYS/SIGUNUSED  |	coredump 	|
> > > >   *	|  SIGSTKFLT         |	terminate	|
> > > >   *	|  SIGWINCH          |	ignore   	|
> > > > - *	|  SIGPWR            |	terminate	|
> > > > + *	|  SIGPWR            |	ignore   	|
> > > 
> > > You need to update signal.7 too:
> > > https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man7/signal.7#n285
> > 
> > (I fail this whole thread via b4 and it appears that a bunch of messages
> > are missing on lore. Might just be delay though.)
> > 
> > How this is this not going to break userspace? Just for a start,
> > SIGPWR (for better or worse) was used for a long time by some
> > sandboxing/container runtimes to shutdown a process and still is.
> 
> To play Devil's advocate -- pid1 has also always had a default-ignore
> signal mask (which included SIGPWR), so any pid1 that obeyed SIGPWR
> already had a non-default signal mask (and thus wouldn't be affected by
> this patch).

Sure, my point wasn't specifically about init systems but rather generic
processes. The reason that SIGPWR was originally used was because
older init systems - apart from systemd - could be shutdown with it
while other programs left it set to SIG_DFL and they would terminate
too. I'm not saying this is a great idea. But changing SIGPWR - if I
read this right - to go from "terminate" to "ignore" will mean that any
program that left SIGPWR unhandled/SIG_DFL will potentially see altered
behavior. Just looking at gvfsd and in debian.codesearch shows that they
explicitly set signal(SIGPWR, SIG_DFL) and I'm sure there are more.

(You also need to keep in mind that the default ignore mask applies to
signals sent from _within_ the pid namespace to pid 1. They'll be
delivered just fine from an ancestor pid namespace. Otherwise I'd be
interested to know how you've ever shutdown one of your containers. ;))

> 
> But I do agree that this seems like a strange change to make (SIGPWR
> seems like a signal you don't want to ignore by default). Unfortunately
> the fact that it appears to always be equal to SIGINFO means that while
> SIGINFO (to me at least) seems like it should be a no-op, the necessary
> SIGPWR change makes it harder to justify IMHO.