[PATCH v1] signal: fix endless loop in complete_signal() during a non-leader exec

[PATCH v1] signal: fix endless loop in complete_signal() during a non-leader exec

[Nickolai Zeldovich]

kill(2) of a process while a non-leader thread of it is in execve() can
lock up the machine: complete_signal() loops forever with siglock held
and interrupts off, and de_thread() then spins on siglock in
release_task() with tasklist_lock held.

The curr_target walk uses next_thread(), which wraps from the last
thread to ->group_leader on the assumption that the leader is first on
signal->thread_head. de_thread() makes the exec'ing thread, the last
one, the leader before release_task() unlinks the old leader, so a walk
that starts at the old leader stays on the new one if that thread
blocks the signal, and never gets back to curr_target.

Wrap to the first entry of signal->thread_head instead; siglock keeps
the list stable.

Reproduced on riscv64 and x86_64 under QEMU: the unpatched kernel locks
up within seconds, the patched one ran for 5 minutes without a hang.

Fixes: d639cf4abb4d ("change next_thread() to use __next_thread() ?: group_leader")
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---

Notes:
    This patch was prepared with the help of an AI tool.
    
    The sender's pid_task() lookup must see the old leader, that is, run
    before de_thread()'s exchange_tids(); after the swap kill() finds the
    new leader and thread_group_empty() returns early.
    
    Reproducer (SMP, no privileges; apart from the RCU stall report the hang
    is silent):
    
      gcc -O2 -pthread -o exec-kill exec-kill.c && ./exec-kill
    
    Before (mainline d135ab453a74, defconfig, QEMU -smp 6, booted with
    rcupdate.rcu_cpu_stall_timeout=10): no "rounds" line, and on x86_64,
    16 s after boot:
    
      rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
      NMI backtrace for cpu 5
      RIP: 0010:complete_signal+0xe0/0x2c0
       __send_signal_locked
       do_send_sig_info
       kill_pid_info_type
      NMI backtrace for cpu 4
      RIP: 0010:queued_spin_lock_slowpath+0x17/0x270
       release_task
       begin_new_exec
    
    riscv64 stalls the same way 13 s after boot, with one CPU in
    complete_signal() and three in the spinlock slow path.
    After: "1000 rounds" up to "33000 rounds" in 300 s on x86_64 ("7000
    rounds" on riscv64), no report.
    
      /* kill(2) a process while its second thread calls execve(2); a hang is the bug. */
      #define _GNU_SOURCE
      #include <pthread.h>
      #include <signal.h>
      #include <stdio.h>
      #include <sys/wait.h>
      #include <unistd.h>
    
      static volatile pid_t target;
    
      static void *sender(void *arg)
      {
      	for (;;) {
      		pid_t pid = target;
    
      		if (pid)
      			kill(pid, SIGRTMIN);
      	}
      	return arg;
      }
    
      static void *execer(void *arg)
      {
      	execl("/proc/self/exe", "exec-kill", "exit", (char *)NULL);
      	return arg;
      }
    
      int main(int argc, char **argv)
      {
      	pthread_t t;
      	sigset_t set;
      	siginfo_t si;
    
      	if (argc > 1)
      		return 0;	/* the image the second thread execs */
      	sigemptyset(&set);
      	sigaddset(&set, SIGRTMIN);
      	sigprocmask(SIG_BLOCK, &set, NULL);	/* every thread blocks it */
      	for (int i = 0; i < 3; i++)
      		pthread_create(&t, NULL, sender, NULL);
      	for (long n = 1;; n++) {
      		pid_t pid = fork();
    
      		if (pid == 0) {
      			pthread_create(&t, NULL, execer, NULL);
      			for (;;)
      				pause();
      		}
      		target = pid;
      		waitid(P_PID, pid, &si, WEXITED | WNOWAIT);
      		target = 0;
      		usleep(1000);
      		waitpid(pid, NULL, 0);
      		if (n % 1000 == 0)
      			printf("%ld rounds\n", n);
      	}
      }

 kernel/signal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index cb6ec7054eee7..c0637ca7bdcfa 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -990,7 +990,10 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
 		 */
 		t = signal->curr_target;
 		while (!wants_signal(sig, t)) {
-			t = next_thread(t);
+			/* during de_thread() the leader may not be first */
+			t = __next_thread(t) ?:
+			    list_first_entry(&signal->thread_head,
+					     struct task_struct, thread_node);
 			if (t == signal->curr_target)
 				/*
 				 * No thread needs to be woken.

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.53.0

Re: [PATCH v1] signal: fix endless loop in complete_signal() during a non-leader exec

[Oleg Nesterov]

On 10/26, Nickolai Zeldovich wrote:
>
> The curr_target walk uses next_thread(), which wraps from the last
> thread to ->group_leader on the assumption that the leader is first on
> signal->thread_head. de_thread() makes the exec'ing thread, the last
> one, the leader before release_task() unlinks the old leader, so a walk
> that starts at the old leader stays on the new one if that thread
> blocks the signal, and never gets back to curr_target.

Oops... Nickolai, thanks, let me think about it. Can't work today, I'll
try to write another email tomorrow.

Oleg.