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