Hi Oleg,
I'm back looking at SIGNAL_UNKILLABLE and debugging child reapers again,
and the current issue is when running code in the target process,
SIGTRAP firing and that causing SIGNAL_UNKILLABLE protection to be
removed in force_sig_info():
if (action->sa.sa_handler == SIG_DFL)
t->signal->flags &= ~SIGNAL_UNKILLABLE;
Would relaxing that if the task is being traced with something like
diff --git i/kernel/signal.c w/kernel/signal.c
index 7e59ebc2c25e..f701f1889895 100644
--- i/kernel/signal.c
+++ w/kernel/signal.c
@@ -1185,7 +1185,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
recalc_sigpending_and_wake(t);
}
}
- if (action->sa.sa_handler == SIG_DFL)
+ if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
t->signal->flags &= ~SIGNAL_UNKILLABLE;
ret = specific_send_sig_info(sig, info, t);
spin_unlock_irqrestore(&t->sighand->siglock, flags);
make any sense? It does address the issue that I'm seeing, but are
there any downsides to doing so?
Thanks,
Jamie
Hi Jamie,
On 04/25, Jamie Iles wrote:
>
> Hi Oleg,
>
> I'm back looking at SIGNAL_UNKILLABLE and debugging child reapers again,
> and the current issue is when running code in the target process,
> SIGTRAP firing and that causing SIGNAL_UNKILLABLE protection to be
> removed in force_sig_info():
>
> if (action->sa.sa_handler == SIG_DFL)
> t->signal->flags &= ~SIGNAL_UNKILLABLE;
Yes, this is what I meant when I said force_sig_info() needs changes too.
I was going to fix it "tomorrow" but I was distracted and then forgot.
> @@ -1185,7 +1185,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
> recalc_sigpending_and_wake(t);
> }
> }
> - if (action->sa.sa_handler == SIG_DFL)
> + if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
> t->signal->flags &= ~SIGNAL_UNKILLABLE;
> ret = specific_send_sig_info(sig, info, t);
> spin_unlock_irqrestore(&t->sighand->siglock, flags);
Not sure, let me think a bit more... and this is not enough anyway.
perhaps we should start with this simple change, but the "real" fix
needs a lot of cleanups, although I am not sure if we will ever do this.
Oleg.
On Wed, Apr 26, 2017 at 05:18:58PM +0200, Oleg Nesterov wrote:
> Hi Jamie,
>
> On 04/25, Jamie Iles wrote:
> >
> > Hi Oleg,
> >
> > I'm back looking at SIGNAL_UNKILLABLE and debugging child reapers again,
> > and the current issue is when running code in the target process,
> > SIGTRAP firing and that causing SIGNAL_UNKILLABLE protection to be
> > removed in force_sig_info():
> >
> > if (action->sa.sa_handler == SIG_DFL)
> > t->signal->flags &= ~SIGNAL_UNKILLABLE;
>
> Yes, this is what I meant when I said force_sig_info() needs changes too.
> I was going to fix it "tomorrow" but I was distracted and then forgot.
>
> > @@ -1185,7 +1185,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
> > recalc_sigpending_and_wake(t);
> > }
> > }
> > - if (action->sa.sa_handler == SIG_DFL)
> > + if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
> > t->signal->flags &= ~SIGNAL_UNKILLABLE;
> > ret = specific_send_sig_info(sig, info, t);
> > spin_unlock_irqrestore(&t->sighand->siglock, flags);
>
> Not sure, let me think a bit more... and this is not enough anyway.
>
> perhaps we should start with this simple change, but the "real" fix
> needs a lot of cleanups, although I am not sure if we will ever do this.
Okay, sounds good. I'm happy to spend more time looking at this if you
have suggestions - in the context of namespaces and containers this
seems more relevant than when it was just the system init that we were
protecting.
Jamie
Hi Oleg,
On Thu, Apr 27, 2017 at 01:16:51PM +0100, Jamie Iles wrote:
> On Wed, Apr 26, 2017 at 05:18:58PM +0200, Oleg Nesterov wrote:
> > Hi Jamie,
> >
> > On 04/25, Jamie Iles wrote:
> > >
> > > Hi Oleg,
> > >
> > > I'm back looking at SIGNAL_UNKILLABLE and debugging child reapers again,
> > > and the current issue is when running code in the target process,
> > > SIGTRAP firing and that causing SIGNAL_UNKILLABLE protection to be
> > > removed in force_sig_info():
> > >
> > > if (action->sa.sa_handler == SIG_DFL)
> > > t->signal->flags &= ~SIGNAL_UNKILLABLE;
> >
> > Yes, this is what I meant when I said force_sig_info() needs changes too.
> > I was going to fix it "tomorrow" but I was distracted and then forgot.
> >
> > > @@ -1185,7 +1185,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
> > > recalc_sigpending_and_wake(t);
> > > }
> > > }
> > > - if (action->sa.sa_handler == SIG_DFL)
> > > + if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
> > > t->signal->flags &= ~SIGNAL_UNKILLABLE;
> > > ret = specific_send_sig_info(sig, info, t);
> > > spin_unlock_irqrestore(&t->sighand->siglock, flags);
> >
> > Not sure, let me think a bit more... and this is not enough anyway.
> >
> > perhaps we should start with this simple change, but the "real" fix
> > needs a lot of cleanups, although I am not sure if we will ever do this.
>
> Okay, sounds good. I'm happy to spend more time looking at this if you
> have suggestions - in the context of namespaces and containers this
> seems more relevant than when it was just the system init that we were
> protecting.
Any objections to moving ahead with this patch?
Thanks,
Jamie
Hi Jamie,
On 08/14, Jamie Iles wrote:
>
> > > Not sure, let me think a bit more... and this is not enough anyway.
> > >
> > > perhaps we should start with this simple change, but the "real" fix
> > > needs a lot of cleanups, although I am not sure if we will ever do this.
> >
> > Okay, sounds good. I'm happy to spend more time looking at this if you
> > have suggestions - in the context of namespaces and containers this
> > seems more relevant than when it was just the system init that we were
> > protecting.
>
> Any objections to moving ahead with this patch?
Oh, sorry.
OK, lets do this simple change then try to improve this logic further.
I'm afraid you need to re-send your patch, sorry.
Oleg.