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 <ls...@ooseel.net> > > > > Acked-by: Oleg Nesterov <o...@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 > > > > >