get_signal() uses signr or ksig->info.si_signo in a chaotic way, this
looks confusing. Change it to always use signr.
This allows us to simplify the SIGNAL_GROUP_EXIT/group_exec_task branch,
it no longer needs to touch ksig at all.
Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/signal.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index c9c57d053ce4..690982e6595e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2727,12 +2727,15 @@ bool get_signal(struct ksignal *ksig)
/* Has this task already been marked for death? */
if ((signal->flags & SIGNAL_GROUP_EXIT) ||
signal->group_exec_task) {
- clear_siginfo(&ksig->info);
- ksig->info.si_signo = signr = SIGKILL;
+ signr = SIGKILL;
sigdelset(¤t->pending.signal, SIGKILL);
trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
- &sighand->action[SIGKILL - 1]);
+ &sighand->action[SIGKILL - 1]);
recalc_sigpending();
+ /*
+ * implies do_group_exit(), no need to initialize
+ * ksig->info
+ */
goto fatal;
}
@@ -2842,7 +2845,7 @@ bool get_signal(struct ksignal *ksig)
spin_lock_irq(&sighand->siglock);
}
- if (likely(do_signal_stop(ksig->info.si_signo))) {
+ if (likely(do_signal_stop(signr))) {
/* It released the siglock. */
goto relock;
}
@@ -2866,7 +2869,7 @@ bool get_signal(struct ksignal *ksig)
if (sig_kernel_coredump(signr)) {
if (print_fatal_signals)
- print_fatal_signal(ksig->info.si_signo);
+ print_fatal_signal(signr);
proc_coredump_connector(current);
/*
* If it was able to dump core, this kills all
@@ -2890,7 +2893,7 @@ bool get_signal(struct ksignal *ksig)
/*
* Death signals, no core dump.
*/
- do_group_exit(ksig->info.si_signo);
+ do_group_exit(signr);
/* NOTREACHED */
}
spin_unlock_irq(&sighand->siglock);
--
2.25.1.362.g51ebf55
On 2024/2/23 18:50, Oleg Nesterov wrote:
> get_signal() uses signr or ksig->info.si_signo in a chaotic way, this
> looks confusing. Change it to always use signr.
>
> This allows us to simplify the SIGNAL_GROUP_EXIT/group_exec_task branch,
> it no longer needs to touch ksig at all.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---
> kernel/signal.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c9c57d053ce4..690982e6595e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2727,12 +2727,15 @@ bool get_signal(struct ksignal *ksig)
> /* Has this task already been marked for death? */
> if ((signal->flags & SIGNAL_GROUP_EXIT) ||
> signal->group_exec_task) {
> - clear_siginfo(&ksig->info);
> - ksig->info.si_signo = signr = SIGKILL;
> + signr = SIGKILL;
> sigdelset(¤t->pending.signal, SIGKILL);
> trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> - &sighand->action[SIGKILL - 1]);
> + &sighand->action[SIGKILL - 1]);
> recalc_sigpending();
> + /*
> + * implies do_group_exit(), no need to initialize
> + * ksig->info
> + */
> goto fatal;
There is little question:
If the this conditions is met:
current->flags & PF_USER_WORKER
It may execute “goto out“ instead of do_group_exit().
--
Best wishes,
Wen
> }
>
> @@ -2842,7 +2845,7 @@ bool get_signal(struct ksignal *ksig)
> spin_lock_irq(&sighand->siglock);
> }
>
> - if (likely(do_signal_stop(ksig->info.si_signo))) {
> + if (likely(do_signal_stop(signr))) {
> /* It released the siglock. */
> goto relock;
> }
> @@ -2866,7 +2869,7 @@ bool get_signal(struct ksignal *ksig)
>
> if (sig_kernel_coredump(signr)) {
> if (print_fatal_signals)
> - print_fatal_signal(ksig->info.si_signo);
> + print_fatal_signal(signr);
> proc_coredump_connector(current);
> /*
> * If it was able to dump core, this kills all
> @@ -2890,7 +2893,7 @@ bool get_signal(struct ksignal *ksig)
> /*
> * Death signals, no core dump.
> */
> - do_group_exit(ksig->info.si_signo);
> + do_group_exit(signr);
> /* NOTREACHED */
> }
> spin_unlock_irq(&sighand->siglock);
On 02/23, Wen Yang wrote:
>
>
> On 2024/2/23 18:50, Oleg Nesterov wrote:
> >get_signal() uses signr or ksig->info.si_signo in a chaotic way, this
> >looks confusing. Change it to always use signr.
> >
> >This allows us to simplify the SIGNAL_GROUP_EXIT/group_exec_task branch,
> >it no longer needs to touch ksig at all.
> >
> >Signed-off-by: Oleg Nesterov <[email protected]>
> >---
> > kernel/signal.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> >diff --git a/kernel/signal.c b/kernel/signal.c
> >index c9c57d053ce4..690982e6595e 100644
> >--- a/kernel/signal.c
> >+++ b/kernel/signal.c
> >@@ -2727,12 +2727,15 @@ bool get_signal(struct ksignal *ksig)
> > /* Has this task already been marked for death? */
> > if ((signal->flags & SIGNAL_GROUP_EXIT) ||
> > signal->group_exec_task) {
> >- clear_siginfo(&ksig->info);
> >- ksig->info.si_signo = signr = SIGKILL;
> >+ signr = SIGKILL;
> > sigdelset(¤t->pending.signal, SIGKILL);
> > trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> >- &sighand->action[SIGKILL - 1]);
> >+ &sighand->action[SIGKILL - 1]);
> > recalc_sigpending();
> >+ /*
> >+ * implies do_group_exit(), no need to initialize
> >+ * ksig->info
> >+ */
> > goto fatal;
>
> There is little question:
> If the this conditions is met:
> current->flags & PF_USER_WORKER
> It may execute “goto out“ instead of do_group_exit().
Yes, but vhost/io workers do not use ksig at all.
Oleg.
On 02/23, Oleg Nesterov wrote:
>
> On 02/23, Wen Yang wrote:
> >
> >
> > On 2024/2/23 18:50, Oleg Nesterov wrote:
> > >+ /*
> > >+ * implies do_group_exit(), no need to initialize
> > >+ * ksig->info
> > >+ */
> > > goto fatal;
> >
> > There is little question:
> > If the this conditions is met:
> > current->flags & PF_USER_WORKER
> > It may execute “goto out“ instead of do_group_exit().
>
> Yes, but vhost/io workers do not use ksig at all.
Ah, wait...
I forgot about the ->sa_flags check before hide_si_addr_tag_bits().
OK, thanks... I'll send V2.
Oleg.
On 02/23, Oleg Nesterov wrote:
>
> On 02/23, Oleg Nesterov wrote:
> >
> > On 02/23, Wen Yang wrote:
> > >
> > >
> > > On 2024/2/23 18:50, Oleg Nesterov wrote:
> > > >+ /*
> > > >+ * implies do_group_exit(), no need to initialize
> > > >+ * ksig->info
> > > >+ */
> > > > goto fatal;
> > >
> > > There is little question:
> > > If the this conditions is met:
> > > current->flags & PF_USER_WORKER
> > > It may execute “goto out“ instead of do_group_exit().
> >
> > Yes, but vhost/io workers do not use ksig at all.
>
> Ah, wait...
>
> I forgot about the ->sa_flags check before hide_si_addr_tag_bits().
>
> OK, thanks... I'll send V2.
Tomorrow.
I'll recheck, but I think we need the patch below as 1/2 regadless of
this change
Oleg.
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2897,13 +2897,13 @@ bool get_signal(struct ksignal *ksig)
/* NOTREACHED */
}
spin_unlock_irq(&sighand->siglock);
-out:
+
ksig->sig = signr;
if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
hide_si_addr_tag_bits(ksig);
-
- return ksig->sig > 0;
+out:
+ return signr > 0;
}
/**
OK, lets forget about "no need to initialize ksig->info" part for now.
Yes, PF_USER_WORKER's never dereference ksig, but I agree this doesn't
look clean without other cleanups. I'll probably return to this later
when other pending problems with PF_USER_WORKER are fixed.
Wen, thanks again for your review.
Oleg.
On 02/23, Oleg Nesterov wrote:
>
> On 02/23, Oleg Nesterov wrote:
> >
> > On 02/23, Oleg Nesterov wrote:
> > >
> > > On 02/23, Wen Yang wrote:
> > > >
> > > >
> > > > On 2024/2/23 18:50, Oleg Nesterov wrote:
> > > > >+ /*
> > > > >+ * implies do_group_exit(), no need to initialize
> > > > >+ * ksig->info
> > > > >+ */
> > > > > goto fatal;
> > > >
> > > > There is little question:
> > > > If the this conditions is met:
> > > > current->flags & PF_USER_WORKER
> > > > It may execute “goto out“ instead of do_group_exit().
> > >
> > > Yes, but vhost/io workers do not use ksig at all.
> >
> > Ah, wait...
> >
> > I forgot about the ->sa_flags check before hide_si_addr_tag_bits().
> >
> > OK, thanks... I'll send V2.
>
> Tomorrow.
>
> I'll recheck, but I think we need the patch below as 1/2 regadless of
> this change
>
> Oleg.
> ---
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2897,13 +2897,13 @@ bool get_signal(struct ksignal *ksig)
> /* NOTREACHED */
> }
> spin_unlock_irq(&sighand->siglock);
> -out:
> +
> ksig->sig = signr;
>
> if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
> hide_si_addr_tag_bits(ksig);
> -
> - return ksig->sig > 0;
> +out:
> + return signr > 0;
> }
>
> /**
On 2024/2/26 17:52, Oleg Nesterov wrote:
> OK, lets forget about "no need to initialize ksig->info" part for now.
>
> Yes, PF_USER_WORKER's never dereference ksig, but I agree this doesn't
> look clean without other cleanups. I'll probably return to this later
> when other pending problems with PF_USER_WORKER are fixed.
>
> Wen, thanks again for your review.
>
> Oleg.
>
Thank you very much, Oleg,
We greatly appreciate your comments and the discussion has given us a
precious opportunity to understand the linux signal.
--
Best wishes,
Wen
> On 02/23, Oleg Nesterov wrote:
>>
>> On 02/23, Oleg Nesterov wrote:
>>>
>>> On 02/23, Oleg Nesterov wrote:
>>>>
>>>> On 02/23, Wen Yang wrote:
>>>>>
>>>>>
>>>>> On 2024/2/23 18:50, Oleg Nesterov wrote:
>>>>>> + /*
>>>>>> + * implies do_group_exit(), no need to initialize
>>>>>> + * ksig->info
>>>>>> + */
>>>>>> goto fatal;
>>>>>
>>>>> There is little question:
>>>>> If the this conditions is met:
>>>>> current->flags & PF_USER_WORKER
>>>>> It may execute “goto out“ instead of do_group_exit().
>>>>
>>>> Yes, but vhost/io workers do not use ksig at all.
>>>
>>> Ah, wait...
>>>
>>> I forgot about the ->sa_flags check before hide_si_addr_tag_bits().
>>>
>>> OK, thanks... I'll send V2.
>>
>> Tomorrow.
>>
>> I'll recheck, but I think we need the patch below as 1/2 regadless of
>> this change
>>
>> Oleg.
>> ---
>>
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2897,13 +2897,13 @@ bool get_signal(struct ksignal *ksig)
>> /* NOTREACHED */
>> }
>> spin_unlock_irq(&sighand->siglock);
>> -out:
>> +
>> ksig->sig = signr;
>>
>> if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
>> hide_si_addr_tag_bits(ksig);
>> -
>> - return ksig->sig > 0;
>> +out:
>> + return signr > 0;
>> }
>>
>> /**
>