Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dcfcb10b-10c9-eb37-b345-07735453f5b5@gmail.com>
Date:   Wed, 23 Nov 2022 12:27:48 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH 1/1] Fix kill(-1,s) returning 0 on 0 kills
Content-Language: en-US
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Marco Elver <elver@google.com>, linux-kernel@vger.kernel.org
References: <20221122161240.137570-1-pskocik@gmail.com>
 <20221122161240.137570-2-pskocik@gmail.com>
 <20221123103016.GA32207@redhat.com>
From:   Petr Skocik <pskocik@gmail.com>
In-Reply-To: <20221123103016.GA32207@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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