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