Messages in this thread | | | Date | Wed, 23 Nov 2022 12:27:48 +0100 | Subject | Re: [PATCH 1/1] Fix kill(-1,s) returning 0 on 0 kills | From | Petr Skocik <> |
| |
On 11/23/22 11:30, Oleg Nesterov wrote: > On 11/22, Petr Skocik wrote: >> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -1600,20 +1600,18 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid) >> ret = __kill_pgrp_info(sig, info, >> pid ? find_vpid(-pid) : task_pgrp(current)); >> } else { >> - int retval = 0, count = 0; >> struct task_struct * p; >> >> + ret = -ESRCH; >> for_each_process(p) { >> if (task_pid_vnr(p) > 1 && >> !same_thread_group(p, current)) { >> int err = group_send_sig_info(sig, info, p, >> PIDTYPE_MAX); >> - ++count; >> if (err != -EPERM) >> - retval = err; >> + ret = err; /*either all 0 or all -EINVAL*/ > The patch looks good to me, and it also simplifies the code. > > But I fail to understand the /*either all 0 or all -EINVAL*/ comment above.. > > Oleg. >
Thanks. The comment is explained in my reply to Kees Cook: https://lkml.org/lkml/2022/11/22/1327. I felt like making it because without it to me it suspiciously looks like the `if ( err != -EPERM) ret = err;` (or `if ( err != -EPERM) retval = err;` in the original) could be masking a non-EPERM failure with a later success, but it isn't because in this context, all the non-EPERM return vals should either ALL be 0 or ALL be -EINVAL. The original code seems to make this assumption too, although implicitly. Perhaps this should be asserted somehow (WARN_ON?).
If it couldn't be assumed, I'd imagine you'd want to guard against such masking
int retval = 0, count = 0; struct task_struct * p;
for_each_process(p) { if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) { int err = group_send_sig_info(sig, info, p, PIDTYPE_MAX); if (err != -EPERM){ ++count; if ( err < 0 ) /*retval is 0 to start with and set to negatives only*/ retval = err; } } } ret = count ? retval : -ESRCH;
Regards, Petr Skocik
| |