In the fixes commit, removing SIGKILL from each thread signal mask
and executing "goto fatal" directly will skip the call to
"trace_signal_deliver". At this point, the delivery tracking of the SIGKILL
signal will be inaccurate.
Therefore, we need to add trace_signal_deliver before "goto fatal"
after executing sigdelset.
Note: SEND_SIG_NOINFO matches the fact that SIGKILL doesn't have any info.
Acked-by: Christian Brauner <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
Cc: <[email protected]>
Fixes: cf43a757fd4944 ("signal: Restore the stop PTRACE_EVENT_EXIT")
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Zhenliang Wei <[email protected]>
---
kernel/signal.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/signal.c b/kernel/signal.c
index 227ba170298e..3edf526db7c6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2441,6 +2441,8 @@ bool get_signal(struct ksignal *ksig)
if (signal_group_exit(signal)) {
ksig->info.si_signo = signr = SIGKILL;
sigdelset(¤t->pending.signal, SIGKILL);
+ trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
+ &sighand->action[signr - 1]);
recalc_sigpending();
goto fatal;
}
--
2.14.1.windows.1
On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
> In the fixes commit, removing SIGKILL from each thread signal mask
> and executing "goto fatal" directly will skip the call to
> "trace_signal_deliver". At this point, the delivery tracking of the SIGKILL
> signal will be inaccurate.
>
> Therefore, we need to add trace_signal_deliver before "goto fatal"
> after executing sigdelset.
>
> Note: SEND_SIG_NOINFO matches the fact that SIGKILL doesn't have any info.
>
> Acked-by: Christian Brauner <[email protected]>
I think we're supposed to use more Reviewed-bys so feel free (or Andrew)
to change this to:
Reviewed-by: Christian Brauner <[email protected]>
> Reviewed-by: Oleg Nesterov <[email protected]>
> Cc: <[email protected]>
> Fixes: cf43a757fd4944 ("signal: Restore the stop PTRACE_EVENT_EXIT")
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Zhenliang Wei <[email protected]>
> ---
> kernel/signal.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 227ba170298e..3edf526db7c6 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2441,6 +2441,8 @@ bool get_signal(struct ksignal *ksig)
> if (signal_group_exit(signal)) {
> ksig->info.si_signo = signr = SIGKILL;
> sigdelset(¤t->pending.signal, SIGKILL);
> + trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> + &sighand->action[signr - 1]);
Hm, sorry for being the really nitpicky person here. Just for the sake
of consistency how about we do either:
+ trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
+ &sighand->action[SIGKILL - 1]);
or
+ trace_signal_deliver(signr, SEND_SIG_NOINFO,
+ &sighand->action[signr - 1]);
I'm not going to argue about this though. Can just also leave it as is.
Christian
On 04/24, Christian Brauner wrote:
>On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>>
>> Acked-by: Christian Brauner <[email protected]>
>
>I think we're supposed to use more Reviewed-bys so feel free (or Andrew) to change this to:
>
>Reviewed-by: Christian Brauner <[email protected]>
Ok, I will change this in patch v5.
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2441,6 +2441,8 @@ bool get_signal(struct ksignal *ksig)
>> if (signal_group_exit(signal)) {
>> ksig->info.si_signo = signr = SIGKILL;
>> sigdelset(¤t->pending.signal, SIGKILL);
>> + trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
>> + &sighand->action[signr - 1]);
>
>Hm, sorry for being the really nitpicky person here. Just for the sake of consistency how about we do either:
>
>+ trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
>+ &sighand->action[SIGKILL - 1]);
>
>or
>
>+ trace_signal_deliver(signr, SEND_SIG_NOINFO,
>+ &sighand->action[signr - 1]);
>
>I'm not going to argue about this though. Can just also leave it as is.
Thank you for your comments and learn from rigorous people! I will take:
+ trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
+ &sighand->action[SIGKILL - 1]);
Any other suggestions about the patch?
Wei
On 04/24, weizhenliang wrote:
>
> On 04/24, Oleg wrote:
> >On 04/24, Christian Brauner wrote:
> >>
> >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
> >>
> >> > Reviewed-by: Oleg Nesterov <[email protected]>
> >
> >Yes, but ...
> >
> >> > Reported-by: kbuild test robot <[email protected]>
> >
> >Hmm, really?
>
> Yes, the kbuild test robot says that if I fix the problem with the third parameter type,
> I should add this tag. What is wrong or missing?
But this patch does not fix the problem reported by robot, your patch
(which introduced that problem) was dropped, the problem has gone.
With this "Reported-by: kbuild test robot <[email protected]>" tag it looks
as if test-robot has found the problem you are trying to fix: the lack of
trace_signal_deliver(SIGKILL).
Oleg.
On Wed, Apr 24, 2019 at 6:16 PM Oleg Nesterov <[email protected]> wrote:
>
> On 04/24, weizhenliang wrote:
> >
> > On 04/24, Oleg wrote:
> > >On 04/24, Christian Brauner wrote:
> > >>
> > >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
> > >>
> > >> > Reviewed-by: Oleg Nesterov <[email protected]>
> > >
> > >Yes, but ...
> > >
> > >> > Reported-by: kbuild test robot <[email protected]>
> > >
> > >Hmm, really?
> >
> > Yes, the kbuild test robot says that if I fix the problem with the third parameter type,
> > I should add this tag. What is wrong or missing?
>
> But this patch does not fix the problem reported by robot, your patch
> (which introduced that problem) was dropped, the problem has gone.
>
> With this "Reported-by: kbuild test robot <[email protected]>" tag it looks
> as if test-robot has found the problem you are trying to fix: the lack of
> trace_signal_deliver(SIGKILL).
Yeah, Oleg's absolutely right. That tag should just go.
The Fixes line is all that we want, I think.
Christian
On 04/24, Oleg wrote:
>On 04/24, Christian Brauner wrote:
>>
>> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>>
>> > Reviewed-by: Oleg Nesterov <[email protected]>
>
>Yes, but ...
>
>> > Reported-by: kbuild test robot <[email protected]>
>
>Hmm, really?
Yes, the kbuild test robot says that if I fix the problem with the third parameter type,
I should add this tag. What is wrong or missing?
Wei.
On 04/24, Christian Brauner wrote:
>
> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>
> > Reviewed-by: Oleg Nesterov <[email protected]>
Yes, but ...
> > Reported-by: kbuild test robot <[email protected]>
Hmm, really?
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2441,6 +2441,8 @@ bool get_signal(struct ksignal *ksig)
> > if (signal_group_exit(signal)) {
> > ksig->info.si_signo = signr = SIGKILL;
> > sigdelset(¤t->pending.signal, SIGKILL);
> > + trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> > + &sighand->action[signr - 1]);
>
> Hm, sorry for being the really nitpicky person here. Just for the sake
> of consistency how about we do either:
>
> + trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> + &sighand->action[SIGKILL - 1]);
>
> or
>
> + trace_signal_deliver(signr, SEND_SIG_NOINFO,
> + &sighand->action[signr - 1]);
Agreed!
Oleg.
On 04/25 Christian wrote:
>On Wed, Apr 24, 2019 at 6:16 PM Oleg Nesterov <[email protected]> wrote:
>>
>> On 04/24, weizhenliang wrote:
>> >
>> > On 04/24, Oleg wrote:
>> > >On 04/24, Christian Brauner wrote:
>> > >>
>> > >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>> > >>
>> > >> > Reviewed-by: Oleg Nesterov <[email protected]>
>> > >
>> > >Yes, but ...
>> > >
>> > >> > Reported-by: kbuild test robot <[email protected]>
>> > >
>> > >Hmm, really?
>> >
>> > Yes, the kbuild test robot says that if I fix the problem with the
>> > third parameter type, I should add this tag. What is wrong or missing?
>>
>> But this patch does not fix the problem reported by robot, your patch
>> (which introduced that problem) was dropped, the problem has gone.
>>
>> With this "Reported-by: kbuild test robot <[email protected]>" tag it
>> looks as if test-robot has found the problem you are trying to fix:
>> the lack of trace_signal_deliver(SIGKILL).
>
>Yeah, Oleg's absolutely right. That tag should just go.
>The Fixes line is all that we want, I think.
Got it ~
Thank you (Oleg and Christian) for the kind guidance
And I will update the patch as soon as possible
Wei.