Date: Thu, 1 Dec 2016 18:45:24 +0100
From: Oleg Nesterov <oleg@redhat.com>
To: Jamie Iles <jamie.iles@oracle.com>
Cc: linux-kernel@vger.kernel.org, Alexander Viro <viro@zeniv.linux.org.uk>,
        Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] signal: protect SIGNAL_UNKILLABLE from unintentional
        clearing.
Message-ID: <20161201174524.GA27845@redhat.com>
References: <20161116125759.4727-1-jamie.iles@oracle.com> <20161129140600.GB9654@redhat.com> <20161129142353.ew2l4yn66ehdfupd@cedar>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161129142353.ew2l4yn66ehdfupd@cedar>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2558
Lines: 61

On 11/29, Jamie Iles wrote:
>
> On Tue, Nov 29, 2016 at 03:06:00PM +0100, Oleg Nesterov wrote:
> >
> > > +static inline void signal_clear_flags(struct signal_struct *sig,
> > > +				      unsigned int flags)
> > > +{
> > > +	sig->flags &= (~flags | SIGNAL_PROTECTED_MASK);
> > > +}
> >
> > But this one looks pointless.
>
> Well the intent was to have set/clear helpers and *always* use those so
> that it's clear when SIGNAL_UNKILLABLE is being intentionally cleared.

But you can't clear UNKILLABLE intentionally if you do "flags &= WHATEVER".
Unless WHATEVER included UNKILLABLE of course, but in this case we do want
to clear this bit, so I do not understand the purpose.

And in any case this helper should not deny SIGNAL_PROTECTED_MASK silently,
this just adds the confusion.

> > So I think it would be better to start with the minimal change which fixes
> > task_participate_group_stop() and prepare_signal() only. And while I won't
> > insist, perhaps we should add
> >
> > 	#define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | SIGNAL_STOP_CONTINUED)
> >
> > 	signal_set_stop_flags(signal, flags)
> > 	{
> > 		signal->flags = (signal->flags & ~SIGNAL_STOP_MASK) | flags;
> > 	}
> >
> > instead of signal_set_flags(). This way we can, say, add
> > WARN_ON(signal->flags & (SIGNAL_GROUP_EXIT|SIGNAL_GROUP_COREDUMP)) into this helper.
> > Then later we can add signal_set_exit_flags() which manipulates
> > GROUP_EXIT/COREDUMP/UNKILLABLE.
> >
> > Not sure, up to you.
>
> Right, so part of the challenge was figuring out where SIGNAL_UNKILLABLE
> should be cleared, and where it shouldn't.  So is making it an explicit
> boolean in a bitfield a better approach?

Well, I don't think so. Contrary, I think we should turn ->is_child_subreaper
and ->is_child_subreaper into SIGNAL_ flags. And btw these bitfields were
added exactly because we can't add the new SIGNAL_ flags until we cleanup
the current code, to ensure they can't be cleared unintentionally just like
UNKILLABLE. And we can have more flags, so this is not only about UNKILLABLE.

And that is why I suggest to add SIGNAL_STOP_MASK/signal_set_stop_flags
instead of SIGNAL_PROTECTED_MASK/signal_set_flags.

If we add set_signal_exit_flags() later, they should use the different
"preserve" masks. Say, signal_set_stop_flags() must never clear
SIGNAL_GROUP_EXIT (actually it must never see this flag set). On the
other hand, set_signal_exit_flags(GROUP_EXIT) must clear GROUP_STOPPED/etc
but may be preserve UNKILLABLE (later). So bitfields will only complicate
this all.

Oleg.