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.