Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue()

[Oleg Nesterov <oleg@redhat.com>]

On 08/25, Thomas Gleixner wrote:
>
> On Tue, Aug 25 2026 at 20:53, Oleg Nesterov wrote:
> >
> > And I'd suggest to check t->exit_state instead of PF_EXITING,
> > posixtimer_send_sigqueue() can't miss it if it is called after
> > scoped_guard(spinlock_irq, lock).
>
> It neither can miss PF_EXITING which is also set under sighand lock.

Yes, I didn't mean that the PF_EXITING check is wrong... nevermind.

> Once begin_new_exec() sets bprm->point_of_no_return = true there is
> _ZERO_ reason to queue any posix timer signal anymore. Any failure after
> that point will be fatal and shut the whole process down.
>
> begin_new_exex()
> {
>         ...
>
> 	bprm->point_of_no_return = true;
>
> 	scoped_guard(spinlock_irq, &me->sighand->siglock)
> 		me->signal->flags |= SIGNAL_EXEC;

We already have me->signal->group_exec_task.

In mt-exec case it is always set under ->siglock, and cleared after
the last thread passes __exit_signal() which takes the same lock.

>         de_thread(me)
>         ...
>
>         // FIXME: This sequence should be cleaned up with a
>         //        posix_timer_exec() function with a proper stub
> 	//        for CONFIG_POSIX_TIMERS=n.
>
> #ifdef CONFIG_POSIX_TIMERS
> 	spin_lock_irq(&me->sighand->siglock);
> 	posix_cpu_timers_exit(me);
> 	spin_unlock_irq(&me->sighand->siglock);
> 	exit_itimers(me);
> 	flush_itimer_signals();
> #endif

OK... unfortunately we can't do this CONFIG_POSIX_TIMERS sequence before
de_thread()... Another not-yet-exited sub-thread can create a timer
with it_pid = current->pid. Right?

> and in posixtimer_send_sigqueue()
>
> 	if (!likely(lock_task_sighand(t, &flags)))
> 		return;
>
> 	if (unlikely(t->signal->flags & (SIGNAL_EXEC)))
> 		goto unlock;

See above, I think it can check t->signal->group_exec_task. Perhaps along
with SIGNAL_GROUP_EXIT.

So. With this change release_task()->flush_sigqueue(&old_leader->pending)
can still race with posixtimer_send_sigqueue(), but it will do nothing.

But it also does "nothing" if tmr->sigq is already pending (!list_empty)
so I am starting to think about the change below again...

Oleg.

diff --git a/kernel/signal.c b/kernel/signal.c
index bbc0fd4cc4d7..4d12ebba33f9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -477,14 +477,14 @@ static void __sigqueue_free(struct sigqueue *q)
 
 void flush_sigqueue(struct sigpending *queue)
 {
-	struct sigqueue *q;
+	struct sigqueue *q, *n;
 
 	sigemptyset(&queue->signal);
-	while (!list_empty(&queue->list)) {
-		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+
+	list_for_each_entry_safe(q, n, &queue->list, list)
 		__sigqueue_free(q);
-	}
+
+	INIT_LIST_HEAD(&queue->list);
 }
 
 /*