Date: Tue, 29 Nov 2016 14:23:53 +0000
From: Jamie Iles <jamie.iles@oracle.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Jamie Iles <jamie.iles@oracle.com>, 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: <20161129142353.ew2l4yn66ehdfupd@cedar>
References: <20161116125759.4727-1-jamie.iles@oracle.com>
 <20161129140600.GB9654@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161129140600.GB9654@redhat.com>
User-Agent: Mutt/1.6.2-neo (2016-08-21)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4182
Lines: 111

Hi Oleg,

On Tue, Nov 29, 2016 at 03:06:00PM +0100, Oleg Nesterov wrote:
> Jamie,
> 
> I am really sorry for the huge delay.

No problem!

> On 11/16, Jamie Iles wrote:
> >
> > Since 00cd5c37af (ptrace: permit ptracing of /sbin/init) we can now
> > trace init processes.  init is initially protected with
> > SIGNAL_UNKILLABLE which will prevent fatal signals such as SIGSTOP, but
> > there are a number of paths during tracing where SIGNAL_UNKILLABLE can
> > be implicitly cleared.
> 
> Yes, old problem which nobody bothered to fix ;) Thanks for doing this.
> To remind, there are more problems with SIGNAL_UNKILLABLE, but this
> patch looks like a good start to me.
> 
> However, I would like to ask you to make V2, see below.
> 
> > +static inline void signal_set_flags(struct signal_struct *sig,
> > +				    unsigned int flags)
> > +{
> > +	sig->flags = (sig->flags & SIGNAL_PROTECTED_MASK) | flags;
> > +}
> 
> OK, agreed, probably the helper makes sense, but I think it is overused
> in this patch, please see below. In short, I'd suggest to use it only in
> the jctl code, at least for now.
> 
> > +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.  
At the moment there are sites where it is cleared intentionally, and 
others as a consequence of direct assignment.

> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -922,7 +922,7 @@ do_group_exit(int exit_code)
> >  			exit_code = sig->group_exit_code;
> >  		else {
> >  			sig->group_exit_code = exit_code;
> > -			sig->flags = SIGNAL_GROUP_EXIT;
> > +			signal_set_flags(sig, SIGNAL_GROUP_EXIT);
> 
> Well. I am not sure about this change. SIGNAL_GROUP_EXIT is the terminal
> state, it is fine to clear UNKILLABLE.
> 
> Yes, perhaps this actually makes sense, and we want to make UNKILLABLE
> immutable, but then we need to change force_sig_info() too, at least.
> And btw it should be changed anyway, in particular UNKILLABLE can be
> wrongly cleared if the task is traced. But I'd prefer to do this later.
> 
> The same for the similar changes in zap_process(), coredump_finish(),
> and complete_signal().

Okay.

> > @@ -922,7 +922,7 @@ static void complete_signal(int sig, struct 
> > task_struct *p, int group)
> >  			 * running and doing things after a slower
> >  			 * thread has the fatal signal pending.
> >  			 */
> > -			signal->flags = SIGNAL_GROUP_EXIT;
> > +			signal_set_flags(signal, SIGNAL_GROUP_EXIT);
> 
> Again, I'd prefer to just set SIGNAL_GROUP_EXIT, like in do_group_exit().
> Note also that SIGNAL_UNKILLABLE can't be set in this case, see the check
> above, so this change has no effect. And at the same time this code needs
> the changes too, this SIGNAL_UNKILLABLE check is not 100% correct, but this
> is off-topic.
> 
> 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?  That way we can continue to 
manipulate flags as required and then explicitly clear SIGNAL_UNKILLABLE 
when it needs to be cleared?  SIGNAL_UKILLABLE feels like enough of a 
corner case that it has easy potential for regression in the future if 
signal_struct::flags is assigned to.

Thanks,

Jamie