Re: [PATCH] signal: protect SIGNAL_UNKILLABLE from unintentional clearing.

[Jamie Iles]

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