[PATCH 04/17] signal: In get_signal call do_exit when it is unnecessary to shoot down threads

Eric W. Biederman posted 17 patches 1 year, 7 months ago
[PATCH 04/17] signal: In get_signal call do_exit when it is unnecessary to shoot down threads
Posted by Eric W. Biederman 1 year, 7 months ago

In get_signal if other threads of the current process do not need to
be shoot down calling do_group_exit is equivalent to calling do_exit.
The code in get_signal is only responsible for shooting down threads
when it dequeues a signal and decides the signal is fatal.

To remove special cases and make the code easier to read, call do_exit
instead of do_group_exit when no other threads need to be shoot down.

With do_group_exit no longer being called when exec is terminating
threads in de_thread remove the special case in do_group_exit for
handling exec.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c   | 4 ----
 kernel/signal.c | 7 ++++++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index f95a2c1338a8..08de33740b9c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1001,8 +1001,6 @@ do_group_exit(int exit_code)
 
 	if (sig->flags & SIGNAL_GROUP_EXIT)
 		exit_code = sig->group_exit_code;
-	else if (sig->group_exec_task)
-		exit_code = 0;
 	else {
 		struct sighand_struct *const sighand = current->sighand;
 
@@ -1010,8 +1008,6 @@ do_group_exit(int exit_code)
 		if (sig->flags & SIGNAL_GROUP_EXIT)
 			/* Another thread got here before we took the lock.  */
 			exit_code = sig->group_exit_code;
-		else if (sig->group_exec_task)
-			exit_code = 0;
 		else {
 			sig->group_exit_code = exit_code;
 			sig->flags = SIGNAL_GROUP_EXIT;
diff --git a/kernel/signal.c b/kernel/signal.c
index 392d802dbf61..caeaff81a197 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2736,6 +2736,7 @@ bool get_signal(struct ksignal *ksig)
 	}
 
 	for (;;) {
+		bool group_exit_needed = false;
 		struct k_sigaction *ka;
 		enum pid_type type;
 		int exit_code;
@@ -2881,6 +2882,7 @@ bool get_signal(struct ksignal *ksig)
 		 * Anything else is fatal, maybe with a core dump.
 		 */
 		exit_code = signr;
+		group_exit_needed = true;
 	fatal:
 		spin_unlock_irq(&sighand->siglock);
 		if (unlikely(cgroup_task_frozen(current)))
@@ -2916,7 +2918,10 @@ bool get_signal(struct ksignal *ksig)
 		/*
 		 * Death signals, no core dump.
 		 */
-		do_group_exit(exit_code);
+		if (group_exit_needed)
+			do_group_exit(exit_code);
+		else
+			do_exit(exit_code);
 		/* NOTREACHED */
 	}
 	spin_unlock_irq(&sighand->siglock);
-- 
2.41.0
Re: [PATCH 04/17] signal: In get_signal call do_exit when it is unnecessary to shoot down threads
Posted by Oleg Nesterov 1 year, 7 months ago
On 06/18, Eric W. Biederman wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1001,8 +1001,6 @@ do_group_exit(int exit_code)
>
>  	if (sig->flags & SIGNAL_GROUP_EXIT)
>  		exit_code = sig->group_exit_code;
> -	else if (sig->group_exec_task)
> -		exit_code = 0;

OK...

> @@ -1010,8 +1008,6 @@ do_group_exit(int exit_code)
>  		if (sig->flags & SIGNAL_GROUP_EXIT)
>  			/* Another thread got here before we took the lock.  */
>  			exit_code = sig->group_exit_code;
> -		else if (sig->group_exec_task)
> -			exit_code = 0;

Well... So with this change do_group_exit() always "wins" the race if it is
called when another thread has already started de_thread().

But de_thread() won't necessarily notice SIGKILL. Sure, the execing thread
can't return to user-space, but it can do a lot of things after de_thread().

Just for example, can it reach trace_sched_process_exec() ? If yes, then it
will look as if it exits with the exit_code provided by do_group_exit()
_after_ exec.

This differs from case when the execing thread is killed after de_thread(),
in this case exit_code = SIGKILL...

I do not see anything really wrong, just trying to understand the impact of
this change, it looks a bit subtle...

Oleg.