lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/1] Fix kill(-1,s) returning 0 on 0 kills
From
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

\
 
 \ /
  Last update: 2022-11-23 12:30    [W:0.074 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site