[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.

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.

...

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

I think we should fix (and simplify) next_thread() instead. How about

	--- a/include/linux/sched/signal.h
	+++ b/include/linux/sched/signal.h
	@@ -764,7 +764,7 @@ static inline struct task_struct *__next_thread(struct task_struct *p)
	 
	 static inline struct task_struct *next_thread(struct task_struct *p)
	 {
	-	return __next_thread(p) ?: p->group_leader;
	+	return list_next_entry_circular(p, &p->thread_node, thread_node);
	 }

?

Oleg.

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

[Nickolai Zeldovich]

On Wed, Oct 28, 2026 at 10:29 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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.
>
> ...
>
> > --- 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);
>
> I think we should fix (and simplify) next_thread() instead. How about
>
>         --- a/include/linux/sched/signal.h
>         +++ b/include/linux/sched/signal.h
>         @@ -764,7 +764,7 @@ static inline struct task_struct *__next_thread(struct task_struct *p)
>
>          static inline struct task_struct *next_thread(struct task_struct *p)
>          {
>         -       return __next_thread(p) ?: p->group_leader;
>         +       return list_next_entry_circular(p, &p->thread_node, thread_node);
>          }
>
> ?

This seems plausible (presumably with &p->signal->thread_head as the
head), though I think it ends up being a more involved fix.

One thing that would break is cgroup_attach_task() and
cgroup_migrate(): they call while_each_thread() while holding
css_set_lock but not tasklist_lock or siglock.  If cgroup.procs is
written with the pid of a zombie leader that its parent meanwhile
reaps, the walk starts at a task that is already off thread_head,
which is now empty, and next_thread() returns the list head as a task.
(Confirmed as an oops in qemu.)

One option might be to have next_thread do:

  return __next_thread(p) ?:
    list_first_or_null_rcu(&p->signal->thread_head,
      struct task_struct, thread_node) ?:
      p->group_leader;

or convert the cgroup walks to for_each_thread().

Also, since check_unsafe_exec() walks under rcu_read_lock() alone,
should next_thread() keep a single READ_ONCE() of ->next, as
__next_thread() does, instead of list_next_entry_circular()'s two
plain loads?

Happy to send a revised patch along any of these lines, or also feel
free to take it over if that'd be easier for you.

Nickolai.