Re: [PATCH 1/1] Fix kill(-1,s) returning 0 on 0 kills

[Petr Skocik <pskocik@gmail.com>]

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