Messages in this thread | | | Date | Tue, 29 Nov 2016 14:23:53 +0000 | From | Jamie Iles <> | Subject | Re: [PATCH] signal: protect SIGNAL_UNKILLABLE from unintentional clearing. |
| |
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
| |