diff mbox series

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

Message ID 20221122161240.137570-2-pskocik@gmail.com
State New
Headers
Series *** Fix kill(-1,s) returning 0 on 0 kills *** |

Commit Message

Petr Skocik Nov. 22, 2022, 4:12 p.m. UTC 
  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(-)

Comments

Oleg Nesterov Nov. 23, 2022, 10:30 a.m. UTC | #1 
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.
Oleg Nesterov Nov. 23, 2022, 11:20 a.m. UTC | #2 
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.
Petr Skocik Nov. 23, 2022, 11:27 a.m. UTC | #3 
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
Oleg Nesterov Nov. 23, 2022, 11:56 a.m. UTC | #4 
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.
diff mbox series

Patch

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);