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

[Thomas Gleixner <tglx@kernel.org>]

On Tue, Aug 25 2026 at 20:53, Oleg Nesterov wrote:
> On 08/25, Thomas Gleixner wrote:
>> > Is there something to prevent the timer from firing on another CPU,
>> > racing with this tiny window and queue the signal to the old leader? After
>> > all exchange_tids() is just some RCU pointers changed but there is nothing
>> > to synchronize the readers before the flush_sigqueue(). So pid_task() may
>> > still return the old leader after it?
>>
>> You beat me to it.
>>
>> That's what I initialy thought when I added that exiting check into
>> posixtimer_send_queue(), but then the trivial variant lured me away. :)
>
> I'm afraid I am wrong again... but if change posixtimer_send_sigqueue()
> to check !PF_EXITING, de_thread() still can do flush_sigqueue() after
> exchange_tids() outside of tasklist_lock?
>
> 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.

But I think we all looked at it way too narrowly focussed on that
specific non-leader exec() scenario. Let's take a step back and look at
the larger picture.

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.

So why worrying about the non-leader exec() oddity?

begin_new_exex()
{
        ...

	bprm->point_of_no_return = true;

	scoped_guard(spinlock_irq, &me->sighand->siglock)
		me->signal->flags |= SIGNAL_EXEC;

        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
        ...
	scoped_guard(spinlock_irq, &me->sighand->siglock)
		me->signal->flags &= ~SIGNAL_EXEC;
        // SUCCESS
        return 0;

and in posixtimer_send_sigqueue()

	if (!likely(lock_task_sighand(t, &flags)))
		return;

	if (unlikely(t->signal->flags & (SIGNAL_EXEC)))
		goto unlock;

and as we need that check anyway we can just make it:

	if (unlikely(t->signal->flags & (SIGNAL_EXEC | SIGNAL_GROUP_EXIT)))
		goto unlock;

because there is no point either to queue posix timer signals when
SIGNAL_GROUP_EXIT is set, right?

Something like the untested below. At least I'm sure that I got the
scoped_guard() types right this time.

FWIW, I briefly pondered to hide the first part in de_thread(), but
that just made my tired brain fail to reason about it.

Thanks,

        tglx
---
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1148,6 +1148,9 @@ int begin_new_exec(struct linux_binprm *
 	 */
 	bprm->point_of_no_return = true;
 
+	scoped_guard(spinlock_irq, &me->sighand->siglock)
+		me->signal->flags |= SIGNAL_EXEC;
+
 	/* Make this the only thread in the thread group */
 	retval = de_thread(me);
 	if (retval)
@@ -1324,6 +1327,10 @@ int begin_new_exec(struct linux_binprm *
 		}
 		bprm->execfd = retval;
 	}
+
+	scoped_guard(spinlock_irq, &me->sighand->siglock)
+		me->signal->flags &= ~SIGNAL_EXEC;
+
 	return 0;
 
 out_unlock:
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -261,6 +261,8 @@ struct signal_struct {
 #define SIGNAL_STOP_STOPPED	0x00000001 /* job control stop in effect */
 #define SIGNAL_STOP_CONTINUED	0x00000002 /* SIGCONT since WCONTINUED reap */
 #define SIGNAL_GROUP_EXIT	0x00000004 /* group exit in progress */
+#define SIGNAL_EXEC		0x00000008 /* exec in progress */
+
 /*
  * Pending notifications to parent.
  */
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1991,6 +1991,20 @@ void posixtimer_send_sigqueue(struct k_i
 		return;
 
 	/*
+	 * If the process is in the middle of exec(), don't queue signals as the
+	 * posix timers of this process are not longer accessible and about to
+	 * be removed. This prevents a race between queueing the signal on a
+	 * exiting former thread group leader in case of an non-leader exec.
+	 * Aside of that it makes no sense to queue anything now when it has to
+	 * be flushed a split second later anyway.
+	 *
+	 * As this conditional is required just use the opportunity and check
+	 * for a group exit too, where queueing signals is equally pointless.
+	 */
+	if (unlikely(t->signal->flags & (SIGNAL_EXEC | SIGNAL_GROUP_EXIT)))
+		goto unlock;
+
+	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().
 	 */
@@ -2081,6 +2095,7 @@ void posixtimer_send_sigqueue(struct k_i
 	result = TRACE_SIGNAL_DELIVERED;
 out:
 	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
+unlock:
 	unlock_task_sighand(t, &flags);
 }