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

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

[Petr Skocik]

Hi. I've never sent a kernel patch before but this one seemed trivial,
so I thought I'd give it a shot.

My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
to kill.

The code sample below demonstrates the problem, which gets fixed by the
patch:

    #define _GNU_SOURCE
    #include <assert.h>
    #include <errno.h>
    #include <signal.h>
    #include <stdio.h>
    #include <sys/wait.h>
    #include <unistd.h>
    #define VICTIM_UID 4200 //check these are safe to use on your system!
    #define UNUSED_UID 4300
    int main(){
        uid_t r,e,s;
        if(geteuid()) return 1; //requires root privileges

        //pipe to let the parent know when the child has changed ids
        int fds[2]; if(0>pipe(fds)) return 1;
        pid_t pid;
        if(0>(pid=fork())) return 1;
        else if(0==pid){
            setreuid(VICTIM_UID,VICTIM_UID);
            getresuid(&r,&e,&s); printf("child: %u %u %u\n", r,e,s);
            close(fds[0]); close(fds[1]); //let the parent continue
            for(;;) pause();
        }
        close(fds[1]);
        read(fds[0],&(char){0},1); //wait for uid change in the child

        #if 1
        setreuid(VICTIM_UID,(uid_t)-1); seteuid(VICTIM_UID);
        #else
        setresuid(UNUSED_UID,VICTIM_UID,0);
        #endif
        getresuid(&r,&e,&s); printf("parent: %u %u %u\n", r,e,s); //4200 4200 0

        int err = kill(-1,-111); (void)err; //test -EINVAL
        assert(err < 0 &&  errno == EINVAL);

        int rc = kill(-1,SIGTERM); //test 0
        if(rc>=0) wait(0);
        int rc2 = kill(-1,SIGTERM); //test -ESCHR
        printf("1st kill ok==%d; 2nd kill ESRCH==%d\n", rc==0, rc2<0&& errno==ESRCH);
    }

Thank you for considering the patch.

Best regards,
Petr S.


Petr Skocik (1):
  Fix kill(-1,s) returning 0 on 0 kills

 kernel/signal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.25.1

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

[Petr Skocik]

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 <pskocik@gmail.com>
---
 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

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

[Kees Cook]

On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
> Hi. I've never sent a kernel patch before but this one seemed trivial,
> so I thought I'd give it a shot.
> 
> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
> to kill.

It looks like LTP already tests for this, and gets -ESRCH?
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c

Does it still pass with your change?

-- 
Kees Cook

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

[Petr Skocik]

On 11/22/22 18:15, Kees Cook wrote:
> On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
>> Hi. I've never sent a kernel patch before but this one seemed trivial,
>> so I thought I'd give it a shot.
>>
>> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
>> to kill.
> It looks like LTP already tests for this, and gets -ESRCH?
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c
>
> Does it still pass with your change?
>
I went ahead and ran it and it does pass with the change.

But it should be obvious from the code alone too. It's only a few
(and fewer after the patch) simple lines of code.
The original:

         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);
                 ++count;
                 if (err != -EPERM)
                     retval = err;
             }
         }
         ret = count ? retval : -ESRCH;

counts kills made after the `task_pid_vnr(p) > 1 && 
!same_thread_group(p, current)` check.
Some, and possibly all, of those kills fail with -EPERM, but the the 
final line only sets -ESRCH
if the count is zero (i.e., the initial check fails). It should be 
counting only kill attempts that
have _not_ returned -EPERM (if all returned -EPERM, then no suitable 
target was found and
a -ESRCH is would be in order -- but it won't be set with the original 
code!).

So the change can be as minor as

     diff --git a/kernel/signal.c b/kernel/signal.c
     index d140672185a4..e42076e2332b 100644
     --- a/kernel/signal.c
     +++ b/kernel/signal.c
     @@ -1608,9 +1608,10 @@ static int kill_something_info(int sig, 
struct kernel_siginfo *info, pid_t pid)
                         !same_thread_group(p, current)) {
                     int err = group_send_sig_info(sig, info, p,
                                       PIDTYPE_MAX);
     -                ++count;
     -                if (err != -EPERM)
     +                if (err != -EPERM){
     +                    ++count;
                         retval = err;
     +                }
                 }
             }
             ret = count ? retval : -ESRCH;

But since the count variable isn't used other than for the zeroness 
check,  I simplified it further
into
     -        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;


adding a comment explaining the apparent implicit assumption of the 
original code that
the non-EPERM returns from group_send_sig_info in this context must be 
either all  -EINVAL
(bad signal number) or all 0, i.e., there can't be a signal allocation 
failure
(that would be susceptible to being overshadowed by a 0 returned from a 
later kill)
because none of  those kills in this context (kill not sigqueue) should 
require any memory allocation.

It's a tiny patch.

Cheers,
Petr Skocik

P.S.: Apologies if the code formatting is off. Sent this one with 
Thunderbird. Need to work on my
CLI mailsending skills.

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

[Oleg Nesterov]

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.

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

[Oleg Nesterov]

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.

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

[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

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

[Oleg Nesterov]

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.

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

[Petr Skocik]

Hi.

Is there anything else I can do to help get this (or some other 
equivalent change that results in kill(-1,s) returning -ESRCH when it 
has nothing to kill (like it does on the BSDs),
as opposed to the current return value of 0 in that case) incorporated 
into mainline Linux?

It would rather help some of the user software I'm developing, and the 
slightly new semantics are IMO definitely reasonable (BSDs have them).

Basically, the current code:
         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);
                 ++count;
                 if (err != -EPERM)
                     retval = err;
             }
         }
         ret = count ? retval : -ESRCH;

counts kill attempts at non-1, other-process pids  and sets hardcoded 
-ESRCH only if no such attempts are made, which will almost never happen

for a nonroot EUID, because there will typically be non-pid-1 processes 
unkillable by the nonroot EUID, but the code will still count those kill 
attempts, and thus not return the hardcoded -ESRCH even if ALL of those 
kill attemtpts return -EPERM, in which case -ESRCH would be in order 
too, because there were no processes that the current EUID had 
permission to kill (BDSs indeed return ESRCH in such a case).

(The kernel shouldn't need to concern itself with possible racy creation 
of new EUID-killable processes during the kill(-1,s) walk. Either the 
system can be known not to have running superuser code that could racily 
create such EUID-killable processes and then such a kill-returned -ESRCH 
would be useful, or it cannot be known not to have such running 
superuser code, in which case the -ESRCH is transient and should be 
droped by the user).

The current code also implicitly assumes either all non-EPERM kill 
attempts return -EINVAL (invalid signal) or they
all return 0 (success). This assumption should be valid because either 
the signal number is invalid and stays invalid, or it is valid and
the only possible error is -EPERM (this isn't sigqueue so the kill 
shouldn't ever fail with -ENOMEM). If the assumption were not valid,
then the current code could overshadow a previous failed attempt with a 
later succesful one, returning success even if there were some non-EPERM 
failures.

My change proposes:

         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);
                 if (err != -EPERM)
                     ret = err; /*either all 0 or all -EINVAL*/
             }
         }

i.e., start with -ESRCH (nothing to kill) and any non-EPERM kill 
attempts change it to the last return value
--either all 0 or all -EINVAL as per the implicit assumption of the 
original code.

It passes the tests put forth by Kees Cook.

More defensively, the implicit assumption of the original code could be 
made explicit:


         struct task_struct * p;
         int has_last_err = 0;

         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);
                 if (err != -EPERM){
                     if (has_last_err)
                         BUG_ON(ret != err); /*either all 0 or all -EINVAL*/
                     has_last_err = 1;
                     ret = err;
                 }
             }
         }

or dropped;

         struct task_struct * p;
         int has_last_err = 0;

         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);
                 if (err != -EPERM){
                     if (has_last_err){
                         if (err >= 0)
                             continue; /*don't mask previous failure 
with later success*/
                     }
                     has_last_err = 1;
                     ret = err;
                 }
             }
         }

Thanks again for consideration. Criticism welcome.

Regards,
Petr Skocik


On 11/22/22 18:15, Kees Cook wrote:
> On Tue, Nov 22, 2022 at 05:12:40PM +0100, Petr Skocik wrote:
>> Hi. I've never sent a kernel patch before but this one seemed trivial,
>> so I thought I'd give it a shot.
>>
>> My issue: kill(-1,s) on Linux doesn't return -ESCHR when it has nothing
>> to kill.
> It looks like LTP already tests for this, and gets -ESRCH?
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/containers/pidns/pidns10.c
>
> Does it still pass with your change?
>

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

[Eric W. Biederman]

Petr Skocik <pskocik@gmail.com> writes:

> Hi.
>
> Is there anything else I can do to help get this (or some other equivalent
> change that results in kill(-1,s) returning -ESRCH when it has nothing to kill
> (like it does on the BSDs),
> as opposed to the current return value of 0 in that case) incorporated into
> mainline Linux?

I think you are talking about a rare enough case that we can safely
change the error handling behavior  without real risk of trouble.

I think there is room for cleanup here.

I don't think we can change the set of processes signaled.  The linux
man page should be updated to note that we skip sending a signal
to ourselves in the event of -1.

Reading the code the error handling logic is dubious.

POSIX provides some guidance it says:

If pid is -1, sig shall be sent to all processes (excluding an
unspecified set of system processes) for which the process has
permission to send that signal.

[EINVAL]
    The value of the sig argument is an invalid or unsupported signal number.
[EPERM]
    The process does not have permission to send the signal to any receiving process.
[ESRCH]
    No process or process group can be found corresponding to that specified by pid. 

>                 if (err != -EPERM)
>                     ret = err; /*either all 0 or all -EINVAL*/

The comment in your proposed patch is wrong:
  -EAGAIN an be returned in the case of real time signals.
  -ENOMEM can be returned due to linux audit.
  -EINVAL can be returned, but arguably it should be caught
          before we even go into the loop.

Given that the comment is wrong I don't like what you have done with the
error handling logic.  It just adds confusion.

My question: What would a good and carefully implemented version
of kill(2) return?

Today for -pgrp we return 0 if any signal delivery succeeds and
the error from the last process in the signal group otherwise.

For -1 we return -EINVAL if the signal is invalid.
For -1 we return -ESRCH only if we are the init process and
there are no other processes in the system, aka never except
when we are the init process in a pid namespace.
For -1 otherwise we return the return value of the last
process signaled.

I would argue that what needs to happen for -1 is:
- Return 0 if the signal was sent to any process successfully.
- Return -EINVAL for invalid signals.
- When everything errors return some error value and not 0.

What error value should we return when everything errors?
Especially what error value should we return when everything
says -EPERM?

Should we follow BSD and return ESRCH?
Should we follow Posix and return EPERM?
Should we follow SYSV unix?

Looking at FreeBSD aka:
https://cgit.freebsd.org/src/tree/sys/kern/kern_sig.c?id=9e283cf9a2fe2b3aa6e4a47a012fd43b4e49ebec
kill(2) aka killpg1 only returns 0 or ESRCH when sending a signal
to multiple processes (after passing the EINVAL) check.

The man pages for AIX and Solaris suggest that -EPERM is returned when
things don't work.

So what should Linux do?

Historically Linux signaling is very SysV unix with a some compatibility
flags for BSD where it matters.  So I am not convinced that return
ESRCH in this case is the right answer.

Basing the logic off of __kill_pgrp_info I would do:

diff --git a/kernel/signal.c b/kernel/signal.c
index b5370fe5c198..369499ebb8e2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1602,7 +1602,8 @@ 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;
+		bool found = false, success = false;
+		int retval = 0;
 		struct task_struct * p;
 
 		for_each_process(p) {
@@ -1610,12 +1611,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
 					!same_thread_group(p, current)) {
 				int err = group_send_sig_info(sig, info, p,
 							      PIDTYPE_MAX);
-				++count;
-				if (err != -EPERM)
-					retval = err;
+				found = true;
+				success |= !err;
+				retval = err;
 			}
 		}
-		ret = count ? retval : -ESRCH;
+		ret = success ? 0 : (found ? retval : -ESRCH);
 	}
 	read_unlock(&tasklist_lock);
 
I think that fits in better with Linux, and doesn't have any surprising
behavior.

> It would rather help some of the user software I'm developing, and the slightly
> new semantics are IMO definitely reasonable (BSDs have them).

Would my proposal above work for the software you are developing?

The behavior your patch was implementing was:
	ret = success ? 0 : ((retval == -EINVAL)? -EINVAL : -ESRCH);

Which gives less information.  So I am not thrilled by it.

Eric

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

[Petr Skocik]

On 8/10/23 18:16, Eric W. Biederman wrote:
> Petr Skocik <pskocik@gmail.com> writes:
>
>> Hi.
>>
>> Is there anything else I can do to help get this (or some other equivalent
>> change that results in kill(-1,s) returning -ESRCH when it has nothing to kill
>> (like it does on the BSDs),
>> as opposed to the current return value of 0 in that case) incorporated into
>> mainline Linux?
> I think you are talking about a rare enough case that we can safely
> change the error handling behavior  without real risk of trouble.
>
> I think there is room for cleanup here.
>
> I don't think we can change the set of processes signaled.  The linux
> man page should be updated to note that we skip sending a signal
> to ourselves in the event of -1.
>
> Reading the code the error handling logic is dubious.
>
> POSIX provides some guidance it says:
>
> If pid is -1, sig shall be sent to all processes (excluding an
> unspecified set of system processes) for which the process has
> permission to send that signal.
>
> [EINVAL]
>      The value of the sig argument is an invalid or unsupported signal number.
> [EPERM]
>      The process does not have permission to send the signal to any receiving process.
> [ESRCH]
>      No process or process group can be found corresponding to that specified by pid.
>
>>                  if (err != -EPERM)
>>                      ret = err; /*either all 0 or all -EINVAL*/
> The comment in your proposed patch is wrong:
>    -EAGAIN an be returned in the case of real time signals.
>    -ENOMEM can be returned due to linux audit.
>    -EINVAL can be returned, but arguably it should be caught
>            before we even go into the loop.
>
> Given that the comment is wrong I don't like what you have done with the
> error handling logic.  It just adds confusion.
>
> My question: What would a good and carefully implemented version
> of kill(2) return?
>
> Today for -pgrp we return 0 if any signal delivery succeeds and
> the error from the last process in the signal group otherwise.
>
> For -1 we return -EINVAL if the signal is invalid.
> For -1 we return -ESRCH only if we are the init process and
> there are no other processes in the system, aka never except
> when we are the init process in a pid namespace.
> For -1 otherwise we return the return value of the last
> process signaled.
>
> I would argue that what needs to happen for -1 is:
> - Return 0 if the signal was sent to any process successfully.
> - Return -EINVAL for invalid signals.
> - When everything errors return some error value and not 0.
>
> What error value should we return when everything errors?
> Especially what error value should we return when everything
> says -EPERM?
>
> Should we follow BSD and return ESRCH?
> Should we follow Posix and return EPERM?
> Should we follow SYSV unix?
>
> Looking at FreeBSD aka:
> https://cgit.freebsd.org/src/tree/sys/kern/kern_sig.c?id=9e283cf9a2fe2b3aa6e4a47a012fd43b4e49ebec
> kill(2) aka killpg1 only returns 0 or ESRCH when sending a signal
> to multiple processes (after passing the EINVAL) check.
>
> The man pages for AIX and Solaris suggest that -EPERM is returned when
> things don't work.
>
> So what should Linux do?
>
> Historically Linux signaling is very SysV unix with a some compatibility
> flags for BSD where it matters.  So I am not convinced that return
> ESRCH in this case is the right answer.
>
> Basing the logic off of __kill_pgrp_info I would do:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index b5370fe5c198..369499ebb8e2 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1602,7 +1602,8 @@ 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;
> +		bool found = false, success = false;
> +		int retval = 0;
>   		struct task_struct * p;
>   
>   		for_each_process(p) {
> @@ -1610,12 +1611,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
>   					!same_thread_group(p, current)) {
>   				int err = group_send_sig_info(sig, info, p,
>   							      PIDTYPE_MAX);
> -				++count;
> -				if (err != -EPERM)
> -					retval = err;
> +				found = true;
> +				success |= !err;
> +				retval = err;
>   			}
>   		}
> -		ret = count ? retval : -ESRCH;
> +		ret = success ? 0 : (found ? retval : -ESRCH);
>   	}
>   	read_unlock(&tasklist_lock);
>   
> I think that fits in better with Linux, and doesn't have any surprising
> behavior.
>
>> It would rather help some of the user software I'm developing, and the slightly
>> new semantics are IMO definitely reasonable (BSDs have them).
> Would my proposal above work for the software you are developing?
>
> The behavior your patch was implementing was:
> 	ret = success ? 0 : ((retval == -EINVAL)? -EINVAL : -ESRCH);
>
> Which gives less information.  So I am not thrilled by it.
>
> Eric
>
>
>
Thanks for the detailed analysis, Eric W. Biederman.

All my software really cares about is that I get some indication that a 
kill(-1,s) run from a non-root pid no longer had anything left to kill, 
which on Linux is currently being masked by a return value of 0 whereas 
BDSs nicely provide an ESRCH. -EPERM would work too (and would still be 
more useful to me than the current behavior), but I will still object to 
it because I'm convinced you're misreading POSIX here and ESRCH, not 
EPERM, is the error that should be returned here.

You see, while the POSIX guidance for EPERM-returns from kill

     [EPERM] The process does not have permission to send the signal to 
any receiving process.

does indeed seem to suggest that EPERM might be right here, the issue is 
that the receiving processes that returned -EPERM in the loop were 
formally NOT the receiving processes of  kill(-1,s) at all

     If pid is -1, sig shall be sent to all processes (excluding an
     unspecified set of system processes) for which the process has
     permission to send that signal.

The -EPERM-returning internal kills (group_send_sig_info), according to 
the POSIX rules for kill(-1,s), *never* should have happened. It's 
harmless that they did happen as part of the implementation, given that 
those attempts aren't externally observable anyway, but this 
implementation detail should not leak out. Since all targets that for 
kill(-1,s) internally returned -EPERM formally shouldn't have been tried 
in the first place, then if all tried processes returned -EPERM, there 
was no process to try to kill and an -ESRCH is in order. No need to 
diverge from the BSDs here.

That is why the original code had a branch to disregard internal EPERM 
returns and why this branch should be preserved in any patches so that
kill(-1,s) should continue to NEVER return -EPERM. Returning it would 
contradict the spec (kill(-1,s) kills all it has permission to kill so 
it's nonsensical for it to report that it
lacks that permission).

As I said in previous messages, especially my latest one, I don't object 
to dropping the apparent implicit assumption of the current code that 
there can be no masking of a previous
non-EPERM error by a later success, or to making it explicit with some 
assert/BUG_ON statement. Please see the code examples for both of these 
other alternatives  in my previous message.

However, any other implementation (including the one you suggested) is 
welcome and thank you very much for your analysis and willingness to 
pick this.

Best regards,
Petr Skocik

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

[Eric W. Biederman]

Petr Skocik <pskocik@gmail.com> writes:

> Thanks for the detailed analysis, Eric W. Biederman.
>
> All my software really cares about is that I get some indication that a
> kill(-1,s) run from a non-root pid no longer had anything left to kill, 
> which on Linux is currently being masked by a return value of 0 whereas BDSs
> nicely provide an ESRCH. -EPERM would work too (and would still be more useful
> to me than the current behavior), but I will still object to it because I'm
> convinced you're misreading POSIX here and ESRCH, not EPERM, is the error that
> should be returned here.

Thank you for saying any error return is good enough for your
application.  It is definitely a bug that Linux reports success when no
signal has been delivered.

I dug into this a little bit more and found that Illumos and it's
ancestor OpenSolaris can return EPERM, even when sending to all
processes, by reading the Illumos source code.

Reading through the rational of kill it says that it is sometimes
desirable to hide the existence of one process from another so that the
existence of a process will not be an information leak.  To accommodate
that POSIX allows ESRCH instead of EPERM as an error code.

If you want you can read it for yourself here:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html


To sum up.

The function kill(2) should always report success when it has delivered
a signal and not otherwise.

The Linux version of kill(2) is buggy because it reports success when it
has not delivered a signal.

Different implementations of kill(2) do different things in this
situation and POSIX appears to allow the variation, so there is no
strong argument for any specific behavior (other than returning an
error) from a compatibility standpoint.

From my perspective making the implementation of sending a signal to all
processes (-1) handle errors the same as sending a signal to a process
group (-pgrp) seems like the most sensible way to fix this bug in Linux.

I can see an argument for hiding the existence of processes and
returning ESRCH but if/when we go down that road I would just ask that
we be consistent and update all of the signal sending functions at the
same time.

I will see about writing a commit message and posting a final patch in
just a little bit.

Eric

[PATCH] signal: Fix the error return of kill -1

[Eric W. Biederman]

I dug through posix[1], the FreeBSD version of kill(2), and the Illumos
version of kill(2).  Common sense, the documentation and the other
implemnetations of kill(2) agree that an error should be returned if no
signal is delivered.

What is up in the air is which error code should be returned.  FreeBSD
uses ESRCH for all errors.  Illumos will return EPERM for some errors,
and ESRCH for others.  According to the rationale POSIX allows both.

The current Linux behavior of reporting success even when no signal
was delivered dates back to Linux 0.1 with the introduction of
returning ESRCH when there were no processes being added in Linux 1.0.

Since the current behavior is buggy and user-space cares[2][3] change
the behavior to match the behavior when Linux sends signals to process
groups.

Petr Skocik <pskocik@gmail.com> wrote:
> The code sample below demonstrates the problem, which gets fixed by the
> patch:
>
>     #define _GNU_SOURCE
>     #include <assert.h>
>     #include <errno.h>
>     #include <signal.h>
>     #include <stdio.h>
>     #include <sys/wait.h>
>     #include <unistd.h>
>     #define VICTIM_UID 4200 //check these are safe to use on your system!
>     #define UNUSED_UID 4300
>     int main(){
>         uid_t r,e,s;
>         if(geteuid()) return 1; //requires root privileges
>
>         //pipe to let the parent know when the child has changed ids
>         int fds[2]; if(0>pipe(fds)) return 1;
>         pid_t pid;
>         if(0>(pid=fork())) return 1;
>         else if(0==pid){
>             setreuid(VICTIM_UID,VICTIM_UID);
>             getresuid(&r,&e,&s); printf("child: %u %u %u\n", r,e,s);
>             close(fds[0]); close(fds[1]); //let the parent continue
>             for(;;) pause();
>         }
>         close(fds[1]);
>         read(fds[0],&(char){0},1); //wait for uid change in the child
>
>         #if 1
>         setreuid(VICTIM_UID,(uid_t)-1); seteuid(VICTIM_UID);
>         #else
>         setresuid(UNUSED_UID,VICTIM_UID,0);
>         #endif
>         getresuid(&r,&e,&s); printf("parent: %u %u %u\n", r,e,s); //4200 4200 0
>
>         int err = kill(-1,-111); (void)err; //test -EINVAL
>         assert(err < 0 &&  errno == EINVAL);
>
>         int rc = kill(-1,SIGTERM); //test 0
>         if(rc>=0) wait(0);
>         int rc2 = kill(-1,SIGTERM); //test -ESCHR
>         printf("1st kill ok==%d; 2nd kill ESRCH==%d\n", rc==0, rc2<0&& errno==ESRCH);
>     }

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html
[2] https://lkml.kernel.org/r/336ae9be-c66c-d87f-61fe-b916e9f04ffc@gmail.com
[3] https://lkml.kernel.org/r/20221122161240.137570-1-pskocik@gmail.com
Reported-by: Petr Skocik <pskocik@gmail.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index b5370fe5c198..731c6e3b351d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1582,8 +1582,9 @@ EXPORT_SYMBOL_GPL(kill_pid_usb_asyncio);
 /*
  * kill_something_info() interprets pid in interesting ways just like kill(2).
  *
- * POSIX specifies that kill(-1,sig) is unspecified, but what we have
- * is probably wrong.  Should make it like BSD or SYSV.
+ * POSIX allows the error codes EPERM and ESRCH when kill(-1,sig) does
+ * not deliver a signal to any process.  For consistency use the same
+ * logic in kill_something_info and __kill_pgrp_info.
  */
 
 static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
@@ -1602,7 +1603,8 @@ 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;
+		bool found = false, success = false;
+		int retval = 0;
 		struct task_struct * p;
 
 		for_each_process(p) {
@@ -1610,12 +1612,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
 					!same_thread_group(p, current)) {
 				int err = group_send_sig_info(sig, info, p,
 							      PIDTYPE_MAX);
-				++count;
-				if (err != -EPERM)
-					retval = err;
+				found = true;
+				success |= !err;
+				retval = err;
 			}
 		}
-		ret = count ? retval : -ESRCH;
+		ret = success ? 0 : (found ? retval : -ESRCH);
 	}
 	read_unlock(&tasklist_lock);
 
-- 
2.35.3

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

[Petr Skocik]

Thanks. I appreciate your patch and your researching of this.

I still think returning -EPERM for kill(-1,s) (unlike for kill(-pgrp,s), 
where it *can* make sense) is nonsensical because of how POSIX specifies 
kill(-1,sig) specifically ("sig shall be sent to all processes 
(excluding an unspecified set of system processes) for which the process 
has permission to send that signal"). But as I said, any error will do 
for me, so I am still grateful for your patch.

(The way I see it, the POSIX-mentioned possible hiding of processes via 
ESRCH is a completely different matter. In kill(-1,sig) specifically, 
targets that would return -EPERM are excluded/hidden by virtue of the 
definition of kill(-1,sig), which makes it different from other types of 
kills for which there's no generic need to hide EPERMs (only optional 
specific need, hence the paragraph in the POSIX spec on processes with a 
different security label)).

Best regards, Petr Skocik

Re: [PATCH] signal: Fix the error return of kill -1

[Oleg Nesterov]

Hi Eric,

This change LGTM, but ...

On 08/11, Eric W. Biederman wrote:
>
> @@ -1602,7 +1603,8 @@ 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;
> +		bool found = false, success = false;
> +		int retval = 0;
>  		struct task_struct * p;
>  
>  		for_each_process(p) {
> @@ -1610,12 +1612,12 @@ static int kill_something_info(int sig, struct kernel_siginfo *info, pid_t pid)
>  					!same_thread_group(p, current)) {
>  				int err = group_send_sig_info(sig, info, p,
>  							      PIDTYPE_MAX);
> -				++count;
> -				if (err != -EPERM)
> -					retval = err;
> +				found = true;
> +				success |= !err;
> +				retval = err;
>  			}
>  		}
> -		ret = count ? retval : -ESRCH;
> +		ret = success ? 0 : (found ? retval : -ESRCH);

Why do we need the "bool found" variable ? Afacis

	} else {
		bool success = false;
		int retval = -ESRCH;
		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);
				success |= !err;
				retval = err;
			}
		}
		ret = success ? 0 : retval;
	}

does the same?

Oleg.

Re: [PATCH] signal: Fix the error return of kill -1

[Oleg Nesterov]

On 08/14, Oleg Nesterov wrote:
>
> Why do we need the "bool found" variable ? Afacis
>
> 	} else {
> 		bool success = false;
> 		int retval = -ESRCH;
> 		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);
> 				success |= !err;
> 				retval = err;
> 			}
> 		}
> 		ret = success ? 0 : retval;
> 	}
>
> does the same?

Even simpler

	} else {
		struct task_struct * p;
		bool success = false;
		int err = -ESRCH;

		for_each_process(p) {
			if (task_pid_vnr(p) > 1 &&
					!same_thread_group(p, current)) {
				err = group_send_sig_info(sig, info, p,
							  PIDTYPE_MAX);
				success |= !err;
			}
		}
		ret = success ? 0 : err;
	}

unless I missed something...

Oleg.

RE: [PATCH] signal: Fix the error return of kill -1

[David Laight]

From: Oleg Nesterov
> Sent: 14 August 2023 16:44
...
> Even simpler
> 
> 	} else {
> 		struct task_struct * p;
> 		bool success = false;
> 		int err = -ESRCH;

		int err;
		ret = -ESRCH;

> 
> 		for_each_process(p) {
> 			if (task_pid_vnr(p) > 1 &&
> 					!same_thread_group(p, current)) {
> 				err = group_send_sig_info(sig, info, p,
> 							  PIDTYPE_MAX);
> 				success |= !err;
> 			}
> 		}
> 		ret = success ? 0 : err;
> 	}

or maybe even:
	} else {
		struct task_struct * p;
		int err;
		ret = -ESRCH;

		for_each_process(p) {
			if (task_pid_vnr(p) > 1 &&
					!same_thread_group(p, current)) {
				err = group_send_sig_info(sig, info, p,
							  PIDTYPE_MAX);
				if (ret)
					ret = err; 
			}
		}
	}

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] signal: Fix the error return of kill -1

[Oleg Nesterov]

On 08/15, David Laight wrote:
>
> or maybe even:
> 	} else {
> 		struct task_struct * p;
> 		int err;
> 		ret = -ESRCH;
>
> 		for_each_process(p) {
> 			if (task_pid_vnr(p) > 1 &&
> 					!same_thread_group(p, current)) {
> 				err = group_send_sig_info(sig, info, p,
> 							  PIDTYPE_MAX);
> 				if (ret)
> 					ret = err;

Hmm, indeed ;)

and "err" can be declared inside the loop.

Oleg.

Re: [PATCH] signal: Fix the error return of kill -1

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/15, David Laight wrote:
>>
>> or maybe even:
>> 	} else {
>> 		struct task_struct * p;
>> 		int err;
>> 		ret = -ESRCH;
>>
>> 		for_each_process(p) {
>> 			if (task_pid_vnr(p) > 1 &&
>> 					!same_thread_group(p, current)) {
>> 				err = group_send_sig_info(sig, info, p,
>> 							  PIDTYPE_MAX);
>> 				if (ret)
>> 					ret = err;
>
> Hmm, indeed ;)
>
> and "err" can be declared inside the loop.

We can't remove the success case, from my posted patch.

A signal is considered as successfully delivered if at least
one process receives it.

That is something the current code for kill -1 actually gets
wrong (but hides because it ignores -EPERM).

Otherwise yes I expect we can simplify the use of variables as
suggested.

Eric

Re: [PATCH] signal: Fix the error return of kill -1

[Oleg Nesterov]

On 08/16, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 08/15, David Laight wrote:
> >>
> >> or maybe even:
> >> 	} else {
> >> 		struct task_struct * p;
> >> 		int err;
> >> 		ret = -ESRCH;
> >>
> >> 		for_each_process(p) {
> >> 			if (task_pid_vnr(p) > 1 &&
> >> 					!same_thread_group(p, current)) {
> >> 				err = group_send_sig_info(sig, info, p,
> >> 							  PIDTYPE_MAX);
> >> 				if (ret)
> >> 					ret = err;
> >
> > Hmm, indeed ;)
> >
> > and "err" can be declared inside the loop.
>
> We can't remove the success case, from my posted patch.
>
> A signal is considered as successfully delivered if at least
> one process receives it.

Yes.

Initially ret = -ESRCH.

Once group_send_sig_info() succeeds at least once (returns zero)
ret becomes 0.

After that

	if (ret)
		ret = err;

has no effect.

So if a signal is successfully delivered at least once the code
above returns zero.

Oleg.

Re: [PATCH] signal: Fix the error return of kill -1

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/16, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > On 08/15, David Laight wrote:
>> >>
>> >> or maybe even:
>> >> 	} else {
>> >> 		struct task_struct * p;
>> >> 		int err;
>> >> 		ret = -ESRCH;
>> >>
>> >> 		for_each_process(p) {
>> >> 			if (task_pid_vnr(p) > 1 &&
>> >> 					!same_thread_group(p, current)) {
>> >> 				err = group_send_sig_info(sig, info, p,
>> >> 							  PIDTYPE_MAX);
>> >> 				if (ret)
>> >> 					ret = err;
>> >
>> > Hmm, indeed ;)
>> >
>> > and "err" can be declared inside the loop.
>>
>> We can't remove the success case, from my posted patch.
>>
>> A signal is considered as successfully delivered if at least
>> one process receives it.
>
> Yes.
>
> Initially ret = -ESRCH.
>
> Once group_send_sig_info() succeeds at least once (returns zero)
> ret becomes 0.
>
> After that
>
> 	if (ret)
> 		ret = err;
>
> has no effect.
>
> So if a signal is successfully delivered at least once the code
> above returns zero.

Point.

We should be consistent and ensure  __kill_pgrp_info uses
the same code pattern, otherwise it will be difficult to
see they use the same logic.

Does "if (ret) ret = err;" generate better code than "success |= !err"?


I think for both patterns the reader of the code is going to have to
stop and think about what is going on to understand the logic.

We should probably do something like:

	/* 0 for success or the last error */
	if (ret)
        	ret = err;

I am somewhat partial to keeping the variable "success" simply because
while it's computation is clever it's use in generating the result is
not, so it should be more comprehensible code.  Plus the variable
success seems not to need a comment just a minute to stare at
the code and confirm it works.

Eric

Re: [PATCH] signal: Fix the error return of kill -1

[Eric W. Biederman]

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Oleg Nesterov <oleg@redhat.com> writes:
>
>> On 08/16, Eric W. Biederman wrote:
>>>
>>> Oleg Nesterov <oleg@redhat.com> writes:
>>>
>>> > On 08/15, David Laight wrote:
>>> >>
>>> >> or maybe even:
>>> >> 	} else {
>>> >> 		struct task_struct * p;
>>> >> 		int err;
>>> >> 		ret = -ESRCH;
>>> >>
>>> >> 		for_each_process(p) {
>>> >> 			if (task_pid_vnr(p) > 1 &&
>>> >> 					!same_thread_group(p, current)) {
>>> >> 				err = group_send_sig_info(sig, info, p,
>>> >> 							  PIDTYPE_MAX);
>>> >> 				if (ret)
>>> >> 					ret = err;
>>> >
>>> > Hmm, indeed ;)
>>> >
>>> > and "err" can be declared inside the loop.
>>>
>>> We can't remove the success case, from my posted patch.
>>>
>>> A signal is considered as successfully delivered if at least
>>> one process receives it.
>>
>> Yes.
>>
>> Initially ret = -ESRCH.
>>
>> Once group_send_sig_info() succeeds at least once (returns zero)
>> ret becomes 0.
>>
>> After that
>>
>> 	if (ret)
>> 		ret = err;
>>
>> has no effect.
>>
>> So if a signal is successfully delivered at least once the code
>> above returns zero.
>
> Point.
>
> We should be consistent and ensure  __kill_pgrp_info uses
> the same code pattern, otherwise it will be difficult to
> see they use the same logic.
>
> Does "if (ret) ret = err;" generate better code than "success |= !err"?
>

I just looked at the assembly output and at least on x86 with cmov
"if (ret) ret = err;" generates the better assembly even in
the inner loop.

> I think for both patterns the reader of the code is going to have to
> stop and think about what is going on to understand the logic.
>
> We should probably do something like:
>
> 	/* 0 for success or the last error */
> 	if (ret)
>         	ret = err;
>

Even with that comment it feels awkward to me.

Does anyone have any idea how to make that idiom more obvious
what is happening?

Eric

[PATCH] __kill_pgrp_info: simplify the calculation of return value

[Oleg Nesterov]

On 08/16, Eric W. Biederman wrote:
>
> > We should be consistent and ensure  __kill_pgrp_info uses
> > the same code pattern, otherwise it will be difficult to
> > see they use the same logic.

Hmm, agreed.

Then I think we should change __kill_pgrp_info() first, then "copy"
this pattern into kill_something_info() in a separate patch.

> > I think for both patterns the reader of the code is going to have to
> > stop and think about what is going on to understand the logic.

Yes, although to me the current code looks less clear but this is subjective.

But I agree this needs a comment. How about the patch below?


From 753d4edd1f2f21f9f9181b9ff7394ed098d58ff6 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Thu, 17 Aug 2023 17:38:55 +0200
Subject: [PATCH] __kill_pgrp_info: simplify the calculation of return value

No need to calculate/check the "success" variable, we can kill it and update
retval in the main loop unless it is zero.

Suggested-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 128e9bb3d1a2..c0acdfd4c81b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1460,16 +1460,21 @@ int group_send_sig_info(int sig, struct kernel_siginfo *info,
 int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
 {
 	struct task_struct *p = NULL;
-	int retval, success;
+	int ret = -ESRCH;
 
-	success = 0;
-	retval = -ESRCH;
 	do_each_pid_task(pgrp, PIDTYPE_PGID, p) {
 		int err = group_send_sig_info(sig, info, p, PIDTYPE_PGID);
-		success |= !err;
-		retval = err;
+		/*
+		 * If group_send_sig_info() succeeds at least once ret
+		 * becomes 0 and after that the code below has no effect.
+		 * Otherwise we return the last err or -ESRCH if this
+		 * process group is empty.
+		 */
+		if (ret)
+			ret = err;
 	} while_each_pid_task(pgrp, PIDTYPE_PGID, p);
-	return success ? 0 : retval;
+
+	return ret;
 }
 
 int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
-- 
2.25.1.362.g51ebf55