From: Wen Yang <[email protected]>
By explicitly initializing ksig->info.si_code and uniformly using ksig->info,
get_signal() function could be slightly optimized, as folowes:
clear_siginfo(&ksig->info);
ksig->info.si_signo = signr = SIGKILL; --> missed si_code
sigdelset(¤t->pending.signal, SIGKILL);
trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, --> unnecessary SEND_SIG_NOINFO
&sighand->action[SIGKILL - 1]);
recalc_sigpending();
goto fatal;
..
fatal:
..
if (sig_kernel_coredump(signr)) {
..
do_coredump(&ksig->info); --> contains si_code
}
No functional change intended.
Signed-off-by: Wen Yang <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Vincent Whitchurch <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
---
kernel/signal.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index 9b40109f0c56..8cab55bbec2f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2732,8 +2732,9 @@ bool get_signal(struct ksignal *ksig)
signal->group_exec_task) {
clear_siginfo(&ksig->info);
ksig->info.si_signo = signr = SIGKILL;
+ ksig->info.si_code = SI_USER;
sigdelset(¤t->pending.signal, SIGKILL);
- trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
+ trace_signal_deliver(SIGKILL, &ksig->info,
&sighand->action[SIGKILL - 1]);
recalc_sigpending();
goto fatal;
--
2.25.1
On 02/23, [email protected] wrote:
>
> From: Wen Yang <[email protected]>
>
> By explicitly initializing ksig->info.si_code and uniformly using ksig->info,
> get_signal() function could be slightly optimized, as folowes:
I don't understand. Why do you think it will be optimized? in what sense?
> clear_siginfo(&ksig->info);
> ksig->info.si_signo = signr = SIGKILL; --> missed si_code
because we do not need to set .si_code in this case?
> sigdelset(¤t->pending.signal, SIGKILL);
> trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, --> unnecessary SEND_SIG_NOINFO
Why do you think the usage of SEND_SIG_NOINFO is "unnecessary" or bad?
To me this looks good.
> @@ -2732,8 +2732,9 @@ bool get_signal(struct ksignal *ksig)
> signal->group_exec_task) {
> clear_siginfo(&ksig->info);
> ksig->info.si_signo = signr = SIGKILL;
> + ksig->info.si_code = SI_USER;
> sigdelset(¤t->pending.signal, SIGKILL);
> - trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
> + trace_signal_deliver(SIGKILL, &ksig->info,
Well. to me this look like the minor but unnecessary pessimization.
AFAICS, we do not need to initialize .si_code. The usage if ksig->info
instead of ksig->info means that TP_STORE_SIGINFO() will actually read
the memory.
Sorry, I don't understand the point at all :/
and it seems that we can simply kill clear_siginfo(), but this is
another story.
Oleg.
On 2024/2/23 03:05, Oleg Nesterov wrote:
> On 02/23, [email protected] wrote:
>>
>> From: Wen Yang <[email protected]>
>>
>> By explicitly initializing ksig->info.si_code and uniformly using ksig->info,
>> get_signal() function could be slightly optimized, as folowes:
>
> I don't understand. Why do you think it will be optimized? in what sense?
>
>> clear_siginfo(&ksig->info);
>> ksig->info.si_signo = signr = SIGKILL; --> missed si_code
>
> because we do not need to set .si_code in this case?
>
>> sigdelset(¤t->pending.signal, SIGKILL);
>> trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, --> unnecessary SEND_SIG_NOINFO
>
> Why do you think the usage of SEND_SIG_NOINFO is "unnecessary" or bad?
> To me this looks good.
>
Since it is called "SEND_SIG_NOINFO", but here it is neither SEND_SIG
nor NOINFO.
It is get_signal() here, and ksig->info has also been partially
initialized before calling trace_signal_deliver(). Below "goto fatal",
do_coredump() also use the initialized ksig->info.
>> @@ -2732,8 +2732,9 @@ bool get_signal(struct ksignal *ksig)
>> signal->group_exec_task) {
>> clear_siginfo(&ksig->info);
>> ksig->info.si_signo = signr = SIGKILL;
>> + ksig->info.si_code = SI_USER;
>> sigdelset(¤t->pending.signal, SIGKILL);
>> - trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
>> + trace_signal_deliver(SIGKILL, &ksig->info,
>
> Well. to me this look like the minor but unnecessary pessimization.
>
> AFAICS, we do not need to initialize .si_code. The usage if ksig->info
> instead of means that TP_STORE_SIGINFO() will actually read
> the memory.
>
> Sorry, I don't understand the point at all :/
>
> and it seems that we can simply kill clear_siginfo(), but this is
> another story.
>
This is not right.
ksig->info will be passed to user space through do_coredump(), and the
clear_siginfo() cannot be killed.
bool get_signal(struct ksignal *ksig)
{
..
if ((signal->flags & SIGNAL_GROUP_EXIT) ||
signal->group_exec_task) {
clear_siginfo(&ksig->info);
ksig->info.si_signo = signr = SIGKILL;
..
goto fatal;
}
fatal:
..
do_coredump(&ksig->info);
}
void do_coredump(const kernel_siginfo_t *siginfo)
{
..
struct coredump_params cprm = {
.siginfo = siginfo,
..
};
..
sub_info = call_usermodehelper_setup(..., &cprm);
..
call_usermodehelper_exec(sub_info,...);
> Oleg.
>
On 02/23, Wen Yang wrote:
>
> On 2024/2/23 03:05, Oleg Nesterov wrote:
> >On 02/23, [email protected] wrote:
> >>
> >>From: Wen Yang <[email protected]>
> >>
> >>By explicitly initializing ksig->info.si_code and uniformly using ksig->info,
> >>get_signal() function could be slightly optimized, as folowes:
> >
> >I don't understand. Why do you think it will be optimized? in what sense?
> >
> >> clear_siginfo(&ksig->info);
> >> ksig->info.si_signo = signr = SIGKILL; --> missed si_code
> >
> >because we do not need to set .si_code in this case?
> >
> >> sigdelset(¤t->pending.signal, SIGKILL);
> >> trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, --> unnecessary SEND_SIG_NOINFO
> >
> >Why do you think the usage of SEND_SIG_NOINFO is "unnecessary" or bad?
> >To me this looks good.
> >
>
> Since it is called "SEND_SIG_NOINFO", but here it is neither SEND_SIG
> nor NOINFO.
I don't really understand what does this mean. But I can say that
SEND_SIG_NOINFO is exactly what we should use, this signal has no
info.
In fact, SIGKILL can never have the info, see the sig == SIGKILL
check in __send_signal_locked() but this is offtopic.
> It is get_signal() here, and ksig->info has also been partially
> initialized before calling trace_signal_deliver(). Below "goto fatal",
> do_coredump() also use the initialized ksig->info.
IIRC, do_coredump() paths use only siginfo->si_signo, but this doesn't
matter.
do_coredump() can't be called, sig_kernel_coredump(SIGKILL) is false.
> >and it seems that we can simply kill clear_siginfo(), but this is
> >another story.
>
> This is not right.
>
> ksig->info will be passed to user space through do_coredump(), and the
> clear_siginfo() cannot be killed.
See above.
Oleg.