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

[Oleg Nesterov <oleg@redhat.com>]

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.