Message-ID: <336ae9be-c66c-d87f-61fe-b916e9f04ffc@gmail.com>
Date:   Thu, 10 Aug 2023 23:30:44 +0200
From:   Petr Skocik <pskocik@...il.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Kees Cook <keescook@...omium.org>, Oleg Nesterov <oleg@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Marco Elver <elver@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] *** Fix kill(-1,s) returning 0 on 0 kills ***

On 8/10/23 18:16, Eric W. Biederman wrote:
> Petr Skocik <pskocik@...il.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

Powered by blists - more mailing lists