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

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

[Oleg Nesterov]

On 10/28, Nickolai Zeldovich wrote:
>
> On Wed, Oct 28, 2026 at 10:29 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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),

Yes sure, I obviously meant head == &p->signal->thread_head ;)

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

Yes, thanks...

> or convert the cgroup walks to for_each_thread().

(perhaps this makes sense regardless, but we should not delay your fix)

> 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?

IOW, we need list_next_entry_circular_rcu() we do not have yet.

Thanks Nickolai, I am starting to agree with your fix, we can do cleanups
later.


But, unless I missed something again, complete_signal() can safely use
list_next_entry_circular(). And to me

		while (!wants_signal(sig, t)) {
			t = list_next_entry_circular(t, ...);

certainly looks more simple/clean than __next_thread() + list_first_entry().

If you agree and see nothing wrong, can you send V2 please?

Oleg.

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

[Christian Brauner]

On Thu, Oct 29, 2026 at 01:28:41PM +0100, Oleg Nesterov wrote:
> If you agree and see nothing wrong, can you send V2 please?

Send a new version, pleaes. I'll pick it up.

[PATCH] signal: fix lockup in complete_signal() racing with mt exec

[Oleg Nesterov]

Nickolai Zeldovich says:

    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.

Change complete_signal() to use list_next_entry_circular() instead
of next_thread().

Other users of next_thread/while_each_thread/for_other_threads look safe,
so this patch only changes complete_signal() to make a simple fix for
-stable. However, in the longer term we should update next_thread()
or add another helper, but this needs more work; next_thread() should
work even when signal->thread_head is empty.

Fixes: d639cf4abb4d ("change next_thread() to use __next_thread() ?: group_leader")
Reported-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
Link: https://lore.kernel.org/all/CADGQE7EusFrrSoBnhXhdE20tZJrp1EDe=2623mS8vEMY9hn53A@mail.gmail.com/
Originally-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 2474507ac92b..bf5518be67a2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -987,7 +987,8 @@ 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);
+			t = list_next_entry_circular(t, &signal->thread_head,
+							thread_node);
 			if (t == signal->curr_target)
 				/*
 				 * No thread needs to be woken.
-- 
2.55.0

Re: [PATCH] signal: fix lockup in complete_signal() racing with mt exec

[Nickolai Zeldovich]

Thanks for taking this over, and for the better fix!

On Sat, Oct 31, 2026 at 9:47 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Nickolai Zeldovich says:
>
>     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.
>
> Change complete_signal() to use list_next_entry_circular() instead
> of next_thread().
>
> Other users of next_thread/while_each_thread/for_other_threads look safe,
> so this patch only changes complete_signal() to make a simple fix for
> -stable. However, in the longer term we should update next_thread()
> or add another helper, but this needs more work; next_thread() should
> work even when signal->thread_head is empty.
>
> Fixes: d639cf4abb4d ("change next_thread() to use __next_thread() ?: group_leader")
> Reported-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> Link: https://lore.kernel.org/all/CADGQE7EusFrrSoBnhXhdE20tZJrp1EDe=2623mS8vEMY9hn53A@mail.gmail.com/
> Originally-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 2474507ac92b..bf5518be67a2 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -987,7 +987,8 @@ 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);
> +                       t = list_next_entry_circular(t, &signal->thread_head,
> +                                                       thread_node);
>                         if (t == signal->curr_target)
>                                 /*
>                                  * No thread needs to be woken.
> --
> 2.55.0
>
>

Reviewed-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
Tested-by: Nickolai Zeldovich <nickolai@csail.mit.edu>

Nickolai.