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
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
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.
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?
>
Petr Skocik <[email protected]> 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
On 8/10/23 18:16, Eric W. Biederman wrote:
> Petr Skocik <[email protected]> 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
Petr Skocik <[email protected]> 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
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 <[email protected]> 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/[email protected]
[3] https://lkml.kernel.org/r/[email protected]
Reported-by: Petr Skocik <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
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
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
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.
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.
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)
"Eric W. Biederman" <[email protected]> writes:
> Oleg Nesterov <[email protected]> writes:
>
>> On 08/16, Eric W. Biederman wrote:
>>>
>>> Oleg Nesterov <[email protected]> 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
Oleg Nesterov <[email protected]> writes:
> On 08/16, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <[email protected]> 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