Make kill(-1,s) return -ESRCH when it has nothing to kill.
It's the sensible thing to do, it's what FreeBSD does, and
it also seems to be the unrealized intention of the original code.
Signed-off-by: Petr Skocik <[email protected]>
---
kernel/signal.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index d140672185a4..02e7c85c7152 100644
--- 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*/
}
}
- ret = count ? retval : -ESRCH;
}
read_unlock(&tasklist_lock);
--
2.25.1
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.
On 11/23, 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..
OTOH... I think we do not really care, but there is another problem with
or without your patch. Suppose that group_send_sig_info() returns -EAGAIN,
then succeeds. So perhaps something like
struct task_struct *p;
int esrch = -ESRCH;
ret = 0;
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 == 0)
esrch = 0;
else if (err != -EPERM)
ret = err;
}
}
ret = ret ?: esrch;
if we really want to make this code "100% correct". But again, I am not sure
this makes sense.
Oleg.
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
On 11/23, Petr Skocik wrote:
>
> On 11/23/22 11:30, Oleg Nesterov wrote:
> >
> >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.
Ah, now I see what did you mean, thanks.
Well, you are probably right, __send_signal_locked() won't fail even if
__sigqueue_alloc() fails, because si_code = SI_USER.
Not sure we should rely on this, but I won't argue.
Oleg.