* [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
@ 2020-12-20 7:45 Leesoo Ahn
2020-12-20 14:21 ` Oleg Nesterov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Leesoo Ahn @ 2020-12-20 7:45 UTC (permalink / raw)
To: linux-kernel
Cc: Leesoo Ahn, Christian Brauner, Eric W. Biederman, Oleg Nesterov,
Jens Axboe, Peter Collingbourne, Zhiqiang Liu
clear_siginfo() is responsible for clearing struct kernel_siginfo object.
It's obvious that manually initializing those fields is needless as
a commit[1] explains why the function introduced and its guarantee that
all bits in the struct are cleared after it.
[1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
---
kernel/signal.c | 21 ---------------------
1 file changed, 21 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index 5736c55aaa1a..8f49fa3ade33 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
*/
clear_siginfo(info);
info->si_signo = sig;
- info->si_errno = 0;
info->si_code = SI_USER;
- info->si_pid = 0;
- info->si_uid = 0;
}
}
@@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
case (unsigned long) SEND_SIG_NOINFO:
clear_siginfo(&q->info);
q->info.si_signo = sig;
- q->info.si_errno = 0;
q->info.si_code = SI_USER;
q->info.si_pid = task_tgid_nr_ns(current,
task_active_pid_ns(t));
@@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
case (unsigned long) SEND_SIG_PRIV:
clear_siginfo(&q->info);
q->info.si_signo = sig;
- q->info.si_errno = 0;
q->info.si_code = SI_KERNEL;
- q->info.si_pid = 0;
- q->info.si_uid = 0;
break;
default:
copy_siginfo(&q->info, info);
@@ -1623,10 +1616,7 @@ void force_sig(int sig)
clear_siginfo(&info);
info.si_signo = sig;
- info.si_errno = 0;
info.si_code = SI_KERNEL;
- info.si_pid = 0;
- info.si_uid = 0;
force_sig_info(&info);
}
EXPORT_SYMBOL(force_sig);
@@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
clear_siginfo(&info);
info.si_signo = sig;
- info.si_errno = 0;
info.si_code = code;
info.si_addr = addr;
#ifdef __ARCH_SI_TRAPNO
@@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
clear_siginfo(&info);
info.si_signo = sig;
- info.si_errno = 0;
info.si_code = code;
info.si_addr = addr;
#ifdef __ARCH_SI_TRAPNO
@@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
clear_siginfo(&info);
info.si_signo = SIGBUS;
- info.si_errno = 0;
info.si_code = code;
info.si_addr = addr;
info.si_addr_lsb = lsb;
@@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
clear_siginfo(&info);
info.si_signo = SIGBUS;
- info.si_errno = 0;
info.si_code = code;
info.si_addr = addr;
info.si_addr_lsb = lsb;
@@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
clear_siginfo(&info);
info.si_signo = SIGSEGV;
- info.si_errno = 0;
info.si_code = SEGV_BNDERR;
info.si_addr = addr;
info.si_lower = lower;
@@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
clear_siginfo(&info);
info.si_signo = SIGSEGV;
- info.si_errno = 0;
info.si_code = SEGV_PKUERR;
info.si_addr = addr;
info.si_pkey = pkey;
@@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
clear_siginfo(&info);
info.si_signo = sig;
- info.si_errno = 0;
/*
* We are under tasklist_lock here so our parent is tied to
* us and cannot change.
@@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
clear_siginfo(&info);
info.si_signo = SIGCHLD;
- info.si_errno = 0;
/*
* see comment in do_notify_parent() about the following 4 lines
*/
@@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
if (signr != info->si_signo) {
clear_siginfo(info);
info->si_signo = signr;
- info->si_errno = 0;
info->si_code = SI_USER;
rcu_read_lock();
info->si_pid = task_pid_vnr(current->parent);
@@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
{
clear_siginfo(info);
info->si_signo = sig;
- info->si_errno = 0;
info->si_code = SI_USER;
info->si_pid = task_tgid_vnr(current);
info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
@@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
clear_siginfo(&info);
info.si_signo = sig;
- info.si_errno = 0;
info.si_code = SI_TKILL;
info.si_pid = task_tgid_vnr(current);
info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 7:45 [PATCH] signal: Don't init struct kernel_siginfo fields to zero again Leesoo Ahn
@ 2020-12-20 14:21 ` Oleg Nesterov
2020-12-20 15:43 ` Rae Kim
2020-12-20 15:25 ` Christian Brauner
2020-12-21 18:36 ` Eric W. Biederman
2 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2020-12-20 14:21 UTC (permalink / raw)
To: Leesoo Ahn
Cc: linux-kernel, Leesoo Ahn, Christian Brauner, Eric W. Biederman,
Jens Axboe, Peter Collingbourne, Zhiqiang Liu
On 12/20, Leesoo Ahn wrote:
>
> clear_siginfo() is responsible for clearing struct kernel_siginfo object.
> It's obvious that manually initializing those fields is needless as
> a commit[1] explains why the function introduced and its guarantee that
> all bits in the struct are cleared after it.
>
> [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
>
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
Acked-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/signal.c | 21 ---------------------
> 1 file changed, 21 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5736c55aaa1a..8f49fa3ade33 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> */
> clear_siginfo(info);
> info->si_signo = sig;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> - info->si_pid = 0;
> - info->si_uid = 0;
> }
> }
>
> @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> case (unsigned long) SEND_SIG_NOINFO:
> clear_siginfo(&q->info);
> q->info.si_signo = sig;
> - q->info.si_errno = 0;
> q->info.si_code = SI_USER;
> q->info.si_pid = task_tgid_nr_ns(current,
> task_active_pid_ns(t));
> @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> case (unsigned long) SEND_SIG_PRIV:
> clear_siginfo(&q->info);
> q->info.si_signo = sig;
> - q->info.si_errno = 0;
> q->info.si_code = SI_KERNEL;
> - q->info.si_pid = 0;
> - q->info.si_uid = 0;
> break;
> default:
> copy_siginfo(&q->info, info);
> @@ -1623,10 +1616,7 @@ void force_sig(int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = SI_KERNEL;
> - info.si_pid = 0;
> - info.si_uid = 0;
> force_sig_info(&info);
> }
> EXPORT_SYMBOL(force_sig);
> @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> #ifdef __ARCH_SI_TRAPNO
> @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> #ifdef __ARCH_SI_TRAPNO
> @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> clear_siginfo(&info);
> info.si_signo = SIGBUS;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> info.si_addr_lsb = lsb;
> @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> clear_siginfo(&info);
> info.si_signo = SIGBUS;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> info.si_addr_lsb = lsb;
> @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
>
> clear_siginfo(&info);
> info.si_signo = SIGSEGV;
> - info.si_errno = 0;
> info.si_code = SEGV_BNDERR;
> info.si_addr = addr;
> info.si_lower = lower;
> @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
>
> clear_siginfo(&info);
> info.si_signo = SIGSEGV;
> - info.si_errno = 0;
> info.si_code = SEGV_PKUERR;
> info.si_addr = addr;
> info.si_pkey = pkey;
> @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> /*
> * We are under tasklist_lock here so our parent is tied to
> * us and cannot change.
> @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>
> clear_siginfo(&info);
> info.si_signo = SIGCHLD;
> - info.si_errno = 0;
> /*
> * see comment in do_notify_parent() about the following 4 lines
> */
> @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
> if (signr != info->si_signo) {
> clear_siginfo(info);
> info->si_signo = signr;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> rcu_read_lock();
> info->si_pid = task_pid_vnr(current->parent);
> @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> {
> clear_siginfo(info);
> info->si_signo = sig;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> info->si_pid = task_tgid_vnr(current);
> info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = SI_TKILL;
> info.si_pid = task_tgid_vnr(current);
> info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 14:21 ` Oleg Nesterov
@ 2020-12-20 15:43 ` Rae Kim
2020-12-20 16:03 ` Christian Brauner
2020-12-21 11:38 ` Leesoo Ahn
0 siblings, 2 replies; 7+ messages in thread
From: Rae Kim @ 2020-12-20 15:43 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Leesoo Ahn, linux-kernel, Leesoo Ahn, Christian Brauner,
Eric W. Biederman, Jens Axboe, Peter Collingbourne, Zhiqiang Liu
It looks like compiler optimization is smart enough to know that
assigning zero is unnecessary after clear_siginfo() which is memset()
under the hood. At least in my x86_64 machine, w/ or w/o this patch,
there is no difference in final compiled machine code. (I've compared
"objdump -d" results for "__send_signal()", "do_tkill()", and
"collect_signal()")
Wouldn't it be nicer to have more information for both human and
compiler since it doesn't generate extra machine code?
On Sun, Dec 20, 2020 at 03:21:35PM +0100, Oleg Nesterov wrote:
> On 12/20, Leesoo Ahn wrote:
> >
> > clear_siginfo() is responsible for clearing struct kernel_siginfo object.
> > It's obvious that manually initializing those fields is needless as
> > a commit[1] explains why the function introduced and its guarantee that
> > all bits in the struct are cleared after it.
> >
> > [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
> >
> > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
>
>
> > ---
> > kernel/signal.c | 21 ---------------------
> > 1 file changed, 21 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 5736c55aaa1a..8f49fa3ade33 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> > */
> > clear_siginfo(info);
> > info->si_signo = sig;
> > - info->si_errno = 0;
> > info->si_code = SI_USER;
> > - info->si_pid = 0;
> > - info->si_uid = 0;
> > }
> > }
> >
> > @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> > case (unsigned long) SEND_SIG_NOINFO:
> > clear_siginfo(&q->info);
> > q->info.si_signo = sig;
> > - q->info.si_errno = 0;
> > q->info.si_code = SI_USER;
> > q->info.si_pid = task_tgid_nr_ns(current,
> > task_active_pid_ns(t));
> > @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> > case (unsigned long) SEND_SIG_PRIV:
> > clear_siginfo(&q->info);
> > q->info.si_signo = sig;
> > - q->info.si_errno = 0;
> > q->info.si_code = SI_KERNEL;
> > - q->info.si_pid = 0;
> > - q->info.si_uid = 0;
> > break;
> > default:
> > copy_siginfo(&q->info, info);
> > @@ -1623,10 +1616,7 @@ void force_sig(int sig)
> >
> > clear_siginfo(&info);
> > info.si_signo = sig;
> > - info.si_errno = 0;
> > info.si_code = SI_KERNEL;
> > - info.si_pid = 0;
> > - info.si_uid = 0;
> > force_sig_info(&info);
> > }
> > EXPORT_SYMBOL(force_sig);
> > @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
> >
> > clear_siginfo(&info);
> > info.si_signo = sig;
> > - info.si_errno = 0;
> > info.si_code = code;
> > info.si_addr = addr;
> > #ifdef __ARCH_SI_TRAPNO
> > @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
> >
> > clear_siginfo(&info);
> > info.si_signo = sig;
> > - info.si_errno = 0;
> > info.si_code = code;
> > info.si_addr = addr;
> > #ifdef __ARCH_SI_TRAPNO
> > @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> > clear_siginfo(&info);
> > info.si_signo = SIGBUS;
> > - info.si_errno = 0;
> > info.si_code = code;
> > info.si_addr = addr;
> > info.si_addr_lsb = lsb;
> > @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
> > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> > clear_siginfo(&info);
> > info.si_signo = SIGBUS;
> > - info.si_errno = 0;
> > info.si_code = code;
> > info.si_addr = addr;
> > info.si_addr_lsb = lsb;
> > @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
> >
> > clear_siginfo(&info);
> > info.si_signo = SIGSEGV;
> > - info.si_errno = 0;
> > info.si_code = SEGV_BNDERR;
> > info.si_addr = addr;
> > info.si_lower = lower;
> > @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
> >
> > clear_siginfo(&info);
> > info.si_signo = SIGSEGV;
> > - info.si_errno = 0;
> > info.si_code = SEGV_PKUERR;
> > info.si_addr = addr;
> > info.si_pkey = pkey;
> > @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> >
> > clear_siginfo(&info);
> > info.si_signo = sig;
> > - info.si_errno = 0;
> > /*
> > * We are under tasklist_lock here so our parent is tied to
> > * us and cannot change.
> > @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> >
> > clear_siginfo(&info);
> > info.si_signo = SIGCHLD;
> > - info.si_errno = 0;
> > /*
> > * see comment in do_notify_parent() about the following 4 lines
> > */
> > @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
> > if (signr != info->si_signo) {
> > clear_siginfo(info);
> > info->si_signo = signr;
> > - info->si_errno = 0;
> > info->si_code = SI_USER;
> > rcu_read_lock();
> > info->si_pid = task_pid_vnr(current->parent);
> > @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> > {
> > clear_siginfo(info);
> > info->si_signo = sig;
> > - info->si_errno = 0;
> > info->si_code = SI_USER;
> > info->si_pid = task_tgid_vnr(current);
> > info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
> >
> > clear_siginfo(&info);
> > info.si_signo = sig;
> > - info.si_errno = 0;
> > info.si_code = SI_TKILL;
> > info.si_pid = task_tgid_vnr(current);
> > info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > --
> > 2.26.2
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 15:43 ` Rae Kim
@ 2020-12-20 16:03 ` Christian Brauner
2020-12-21 11:38 ` Leesoo Ahn
1 sibling, 0 replies; 7+ messages in thread
From: Christian Brauner @ 2020-12-20 16:03 UTC (permalink / raw)
To: Rae Kim
Cc: Oleg Nesterov, Leesoo Ahn, linux-kernel, Leesoo Ahn,
Eric W. Biederman, Jens Axboe, Peter Collingbourne, Zhiqiang Liu
On Mon, Dec 21, 2020 at 12:43:05AM +0900, Rae Kim wrote:
>
> It looks like compiler optimization is smart enough to know that
> assigning zero is unnecessary after clear_siginfo() which is memset()
> under the hood. At least in my x86_64 machine, w/ or w/o this patch,
> there is no difference in final compiled machine code. (I've compared
> "objdump -d" results for "__send_signal()", "do_tkill()", and
> "collect_signal()")
>
> Wouldn't it be nicer to have more information for both human and
> compiler since it doesn't generate extra machine code?
I don't have a strong preference. But the name clear_siginfo() is pretty
obvious imho. Say a new field "foo" were added to siginfo. We would
almost certainly reject a patch that would add an extra info->foo = 0
into all places where siginfo is initialized as being unnecessary unless
there was severe potential for confusion which I don't think is the case
here when removing this in favor of just relying on clear_siginfo(). But
as I said I don't have a strong opinion. I've picked this up but I'm
happy to drop it if other maintainers agree with you.
Christian
>
> On Sun, Dec 20, 2020 at 03:21:35PM +0100, Oleg Nesterov wrote:
> > On 12/20, Leesoo Ahn wrote:
> > >
> > > clear_siginfo() is responsible for clearing struct kernel_siginfo object.
> > > It's obvious that manually initializing those fields is needless as
> > > a commit[1] explains why the function introduced and its guarantee that
> > > all bits in the struct are cleared after it.
> > >
> > > [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
> > >
> > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> >
> >
> > > ---
> > > kernel/signal.c | 21 ---------------------
> > > 1 file changed, 21 deletions(-)
> > >
> > > diff --git a/kernel/signal.c b/kernel/signal.c
> > > index 5736c55aaa1a..8f49fa3ade33 100644
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> > > */
> > > clear_siginfo(info);
> > > info->si_signo = sig;
> > > - info->si_errno = 0;
> > > info->si_code = SI_USER;
> > > - info->si_pid = 0;
> > > - info->si_uid = 0;
> > > }
> > > }
> > >
> > > @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> > > case (unsigned long) SEND_SIG_NOINFO:
> > > clear_siginfo(&q->info);
> > > q->info.si_signo = sig;
> > > - q->info.si_errno = 0;
> > > q->info.si_code = SI_USER;
> > > q->info.si_pid = task_tgid_nr_ns(current,
> > > task_active_pid_ns(t));
> > > @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> > > case (unsigned long) SEND_SIG_PRIV:
> > > clear_siginfo(&q->info);
> > > q->info.si_signo = sig;
> > > - q->info.si_errno = 0;
> > > q->info.si_code = SI_KERNEL;
> > > - q->info.si_pid = 0;
> > > - q->info.si_uid = 0;
> > > break;
> > > default:
> > > copy_siginfo(&q->info, info);
> > > @@ -1623,10 +1616,7 @@ void force_sig(int sig)
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = sig;
> > > - info.si_errno = 0;
> > > info.si_code = SI_KERNEL;
> > > - info.si_pid = 0;
> > > - info.si_uid = 0;
> > > force_sig_info(&info);
> > > }
> > > EXPORT_SYMBOL(force_sig);
> > > @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = sig;
> > > - info.si_errno = 0;
> > > info.si_code = code;
> > > info.si_addr = addr;
> > > #ifdef __ARCH_SI_TRAPNO
> > > @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = sig;
> > > - info.si_errno = 0;
> > > info.si_code = code;
> > > info.si_addr = addr;
> > > #ifdef __ARCH_SI_TRAPNO
> > > @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> > > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> > > clear_siginfo(&info);
> > > info.si_signo = SIGBUS;
> > > - info.si_errno = 0;
> > > info.si_code = code;
> > > info.si_addr = addr;
> > > info.si_addr_lsb = lsb;
> > > @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
> > > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> > > clear_siginfo(&info);
> > > info.si_signo = SIGBUS;
> > > - info.si_errno = 0;
> > > info.si_code = code;
> > > info.si_addr = addr;
> > > info.si_addr_lsb = lsb;
> > > @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = SIGSEGV;
> > > - info.si_errno = 0;
> > > info.si_code = SEGV_BNDERR;
> > > info.si_addr = addr;
> > > info.si_lower = lower;
> > > @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = SIGSEGV;
> > > - info.si_errno = 0;
> > > info.si_code = SEGV_PKUERR;
> > > info.si_addr = addr;
> > > info.si_pkey = pkey;
> > > @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = sig;
> > > - info.si_errno = 0;
> > > /*
> > > * We are under tasklist_lock here so our parent is tied to
> > > * us and cannot change.
> > > @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = SIGCHLD;
> > > - info.si_errno = 0;
> > > /*
> > > * see comment in do_notify_parent() about the following 4 lines
> > > */
> > > @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
> > > if (signr != info->si_signo) {
> > > clear_siginfo(info);
> > > info->si_signo = signr;
> > > - info->si_errno = 0;
> > > info->si_code = SI_USER;
> > > rcu_read_lock();
> > > info->si_pid = task_pid_vnr(current->parent);
> > > @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> > > {
> > > clear_siginfo(info);
> > > info->si_signo = sig;
> > > - info->si_errno = 0;
> > > info->si_code = SI_USER;
> > > info->si_pid = task_tgid_vnr(current);
> > > info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > > @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
> > >
> > > clear_siginfo(&info);
> > > info.si_signo = sig;
> > > - info.si_errno = 0;
> > > info.si_code = SI_TKILL;
> > > info.si_pid = task_tgid_vnr(current);
> > > info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > > --
> > > 2.26.2
> > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 15:43 ` Rae Kim
2020-12-20 16:03 ` Christian Brauner
@ 2020-12-21 11:38 ` Leesoo Ahn
1 sibling, 0 replies; 7+ messages in thread
From: Leesoo Ahn @ 2020-12-21 11:38 UTC (permalink / raw)
To: Rae Kim, Oleg Nesterov
Cc: Leesoo Ahn, linux-kernel, Christian Brauner, Eric W. Biederman,
Jens Axboe, Peter Collingbourne, Zhiqiang Liu
20. 12. 21. 오전 12:43에 Rae Kim 이(가) 쓴 글:
>
> It looks like compiler optimization is smart enough to know that
> assigning zero is unnecessary after clear_siginfo() which is memset()
> under the hood. At least in my x86_64 machine, w/ or w/o this patch,
> there is no difference in final compiled machine code. (I've compared
> "objdump -d" results for "__send_signal()", "do_tkill()", and
> "collect_signal()")
>
> Wouldn't it be nicer to have more information for both human and
> compiler since it doesn't generate extra machine code?
I think the patch is still worth in human perspective even if there is no
difference on machine code. Because someone could get confused by
initializing them twice at the point, for example, why still try to init
even though clear_siginfo() takes all the responsibility, so they could
waste more time to figure out the code.
Leesoo
>
> On Sun, Dec 20, 2020 at 03:21:35PM +0100, Oleg Nesterov wrote:
>> On 12/20, Leesoo Ahn wrote:
>>>
>>> clear_siginfo() is responsible for clearing struct kernel_siginfo object.
>>> It's obvious that manually initializing those fields is needless as
>>> a commit[1] explains why the function introduced and its guarantee that
>>> all bits in the struct are cleared after it.
>>>
>>> [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
>>>
>>> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
>>
>> Acked-by: Oleg Nesterov <oleg@redhat.com>
>>
>>
>>> ---
>>> kernel/signal.c | 21 ---------------------
>>> 1 file changed, 21 deletions(-)
>>>
>>> diff --git a/kernel/signal.c b/kernel/signal.c
>>> index 5736c55aaa1a..8f49fa3ade33 100644
>>> --- a/kernel/signal.c
>>> +++ b/kernel/signal.c
>>> @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
>>> */
>>> clear_siginfo(info);
>>> info->si_signo = sig;
>>> - info->si_errno = 0;
>>> info->si_code = SI_USER;
>>> - info->si_pid = 0;
>>> - info->si_uid = 0;
>>> }
>>> }
>>>
>>> @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
>>> case (unsigned long) SEND_SIG_NOINFO:
>>> clear_siginfo(&q->info);
>>> q->info.si_signo = sig;
>>> - q->info.si_errno = 0;
>>> q->info.si_code = SI_USER;
>>> q->info.si_pid = task_tgid_nr_ns(current,
>>> task_active_pid_ns(t));
>>> @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
>>> case (unsigned long) SEND_SIG_PRIV:
>>> clear_siginfo(&q->info);
>>> q->info.si_signo = sig;
>>> - q->info.si_errno = 0;
>>> q->info.si_code = SI_KERNEL;
>>> - q->info.si_pid = 0;
>>> - q->info.si_uid = 0;
>>> break;
>>> default:
>>> copy_siginfo(&q->info, info);
>>> @@ -1623,10 +1616,7 @@ void force_sig(int sig)
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = sig;
>>> - info.si_errno = 0;
>>> info.si_code = SI_KERNEL;
>>> - info.si_pid = 0;
>>> - info.si_uid = 0;
>>> force_sig_info(&info);
>>> }
>>> EXPORT_SYMBOL(force_sig);
>>> @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = sig;
>>> - info.si_errno = 0;
>>> info.si_code = code;
>>> info.si_addr = addr;
>>> #ifdef __ARCH_SI_TRAPNO
>>> @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = sig;
>>> - info.si_errno = 0;
>>> info.si_code = code;
>>> info.si_addr = addr;
>>> #ifdef __ARCH_SI_TRAPNO
>>> @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
>>> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
>>> clear_siginfo(&info);
>>> info.si_signo = SIGBUS;
>>> - info.si_errno = 0;
>>> info.si_code = code;
>>> info.si_addr = addr;
>>> info.si_addr_lsb = lsb;
>>> @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
>>> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
>>> clear_siginfo(&info);
>>> info.si_signo = SIGBUS;
>>> - info.si_errno = 0;
>>> info.si_code = code;
>>> info.si_addr = addr;
>>> info.si_addr_lsb = lsb;
>>> @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = SIGSEGV;
>>> - info.si_errno = 0;
>>> info.si_code = SEGV_BNDERR;
>>> info.si_addr = addr;
>>> info.si_lower = lower;
>>> @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = SIGSEGV;
>>> - info.si_errno = 0;
>>> info.si_code = SEGV_PKUERR;
>>> info.si_addr = addr;
>>> info.si_pkey = pkey;
>>> @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = sig;
>>> - info.si_errno = 0;
>>> /*
>>> * We are under tasklist_lock here so our parent is tied to
>>> * us and cannot change.
>>> @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = SIGCHLD;
>>> - info.si_errno = 0;
>>> /*
>>> * see comment in do_notify_parent() about the following 4 lines
>>> */
>>> @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
>>> if (signr != info->si_signo) {
>>> clear_siginfo(info);
>>> info->si_signo = signr;
>>> - info->si_errno = 0;
>>> info->si_code = SI_USER;
>>> rcu_read_lock();
>>> info->si_pid = task_pid_vnr(current->parent);
>>> @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
>>> {
>>> clear_siginfo(info);
>>> info->si_signo = sig;
>>> - info->si_errno = 0;
>>> info->si_code = SI_USER;
>>> info->si_pid = task_tgid_vnr(current);
>>> info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
>>> @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
>>>
>>> clear_siginfo(&info);
>>> info.si_signo = sig;
>>> - info.si_errno = 0;
>>> info.si_code = SI_TKILL;
>>> info.si_pid = task_tgid_vnr(current);
>>> info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
>>> --
>>> 2.26.2
>>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 7:45 [PATCH] signal: Don't init struct kernel_siginfo fields to zero again Leesoo Ahn
2020-12-20 14:21 ` Oleg Nesterov
@ 2020-12-20 15:25 ` Christian Brauner
2020-12-21 18:36 ` Eric W. Biederman
2 siblings, 0 replies; 7+ messages in thread
From: Christian Brauner @ 2020-12-20 15:25 UTC (permalink / raw)
To: Leesoo Ahn
Cc: linux-kernel, Leesoo Ahn, Eric W. Biederman, Oleg Nesterov,
Jens Axboe, Peter Collingbourne, Zhiqiang Liu
On Sun, Dec 20, 2020 at 04:45:54PM +0900, Leesoo Ahn wrote:
> clear_siginfo() is responsible for clearing struct kernel_siginfo object.
> It's obvious that manually initializing those fields is needless as
> a commit[1] explains why the function introduced and its guarantee that
> all bits in the struct are cleared after it.
>
> [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
>
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> ---
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
I have a __user annotation fix in my tree from Jann that I plan to send
soon so I'll just stick this on top of it if noone minds.
Christian
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] signal: Don't init struct kernel_siginfo fields to zero again
2020-12-20 7:45 [PATCH] signal: Don't init struct kernel_siginfo fields to zero again Leesoo Ahn
2020-12-20 14:21 ` Oleg Nesterov
2020-12-20 15:25 ` Christian Brauner
@ 2020-12-21 18:36 ` Eric W. Biederman
2 siblings, 0 replies; 7+ messages in thread
From: Eric W. Biederman @ 2020-12-21 18:36 UTC (permalink / raw)
To: Leesoo Ahn
Cc: linux-kernel, Leesoo Ahn, Christian Brauner, Oleg Nesterov,
Jens Axboe, Peter Collingbourne, Zhiqiang Liu
Leesoo Ahn <dev@ooseel.net> writes:
> clear_siginfo() is responsible for clearing struct kernel_siginfo object.
> It's obvious that manually initializing those fields is needless as
> a commit[1] explains why the function introduced and its guarantee that
> all bits in the struct are cleared after it.
The fields that are explicitly set to zero are fields that must be zero.
Not fields whose value should default to zero.
Given how difficult it is to keep track of which fields are relevant
in struct siginfo. I prefer the explicit approach that currently
exists. Especially as the compiler appears smart enough to optimize
through this.
Mostly I am being touchy and conservative because getting all of these
fields set properly took me several kernel development cycles, and
before that there were bugs in setting those fields that persisted for
decades.
I think the current form allows someone to glance at the entry and see
that a field like si_errno is present and set to 0. With you your
suggested change someone needs to walk through the definition of the
union and see which union member is being invoked to see which fields
are present, to see if si_errno even exists, before it is possible
to see that si_errno is 0.
So unless there is a reason more compelling than a few less lines of
code let's keep it this way for now.
Eric
> [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo")
>
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> ---
> kernel/signal.c | 21 ---------------------
> 1 file changed, 21 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5736c55aaa1a..8f49fa3ade33 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> */
> clear_siginfo(info);
> info->si_signo = sig;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> - info->si_pid = 0;
> - info->si_uid = 0;
> }
> }
>
> @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> case (unsigned long) SEND_SIG_NOINFO:
> clear_siginfo(&q->info);
> q->info.si_signo = sig;
> - q->info.si_errno = 0;
> q->info.si_code = SI_USER;
> q->info.si_pid = task_tgid_nr_ns(current,
> task_active_pid_ns(t));
> @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
> case (unsigned long) SEND_SIG_PRIV:
> clear_siginfo(&q->info);
> q->info.si_signo = sig;
> - q->info.si_errno = 0;
> q->info.si_code = SI_KERNEL;
> - q->info.si_pid = 0;
> - q->info.si_uid = 0;
> break;
> default:
> copy_siginfo(&q->info, info);
> @@ -1623,10 +1616,7 @@ void force_sig(int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = SI_KERNEL;
> - info.si_pid = 0;
> - info.si_uid = 0;
> force_sig_info(&info);
> }
> EXPORT_SYMBOL(force_sig);
> @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> #ifdef __ARCH_SI_TRAPNO
> @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> #ifdef __ARCH_SI_TRAPNO
> @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> clear_siginfo(&info);
> info.si_signo = SIGBUS;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> info.si_addr_lsb = lsb;
> @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
> WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> clear_siginfo(&info);
> info.si_signo = SIGBUS;
> - info.si_errno = 0;
> info.si_code = code;
> info.si_addr = addr;
> info.si_addr_lsb = lsb;
> @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
>
> clear_siginfo(&info);
> info.si_signo = SIGSEGV;
> - info.si_errno = 0;
> info.si_code = SEGV_BNDERR;
> info.si_addr = addr;
> info.si_lower = lower;
> @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
>
> clear_siginfo(&info);
> info.si_signo = SIGSEGV;
> - info.si_errno = 0;
> info.si_code = SEGV_PKUERR;
> info.si_addr = addr;
> info.si_pkey = pkey;
> @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> /*
> * We are under tasklist_lock here so our parent is tied to
> * us and cannot change.
> @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>
> clear_siginfo(&info);
> info.si_signo = SIGCHLD;
> - info.si_errno = 0;
> /*
> * see comment in do_notify_parent() about the following 4 lines
> */
> @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
> if (signr != info->si_signo) {
> clear_siginfo(info);
> info->si_signo = signr;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> rcu_read_lock();
> info->si_pid = task_pid_vnr(current->parent);
> @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> {
> clear_siginfo(info);
> info->si_signo = sig;
> - info->si_errno = 0;
> info->si_code = SI_USER;
> info->si_pid = task_tgid_vnr(current);
> info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
>
> clear_siginfo(&info);
> info.si_signo = sig;
> - info.si_errno = 0;
> info.si_code = SI_TKILL;
> info.si_pid = task_tgid_vnr(current);
> info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-21 18:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-20 7:45 [PATCH] signal: Don't init struct kernel_siginfo fields to zero again Leesoo Ahn
2020-12-20 14:21 ` Oleg Nesterov
2020-12-20 15:43 ` Rae Kim
2020-12-20 16:03 ` Christian Brauner
2020-12-21 11:38 ` Leesoo Ahn
2020-12-20 15:25 ` Christian Brauner
2020-12-21 18:36 ` Eric W. Biederman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).