lkml.org 
[lkml]   [2016]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2016-11-29 15:26    [W:6.068 / U:1.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site