signal: Use list_del_init_careful() in flush_sigqueue()

That second half does not hold for the old group leader in a non-leader exec(). de_thread() calls exchange_tids() before release_task(leader), so the struct pid held by a SIGEV_THREAD_ID timer created against the leader's
tid now points to the thread which called execve(). pid_task() returns that
thread and lock_task_sighand() on it succeeds.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads ->next. list_del_init() is not
atomic and INIT_LIST_HEAD() stores ->next before ->prev, so the check can pass in between.

Use list_del_init_careful(), which stores ->next last. A list_empty() which
sees the entry unqueued is then guaranteed that the flush will not store into the entry any more.

Fixes fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")

Cc [email protected]

commit fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless") moved the ->pending flush from __exit_signal() to release_task(), where it runs without ->siglock. The justification was:

after the exiting task passes __exit_signal() lock_task_sighand() can't succeed and pid_task(tmr->it_pid) will return NULL

That second half does not hold for the old group leader in a non-leader exec(). de_thread() calls exchange_tids() before release_task(leader), so

Quoted 10 lines · Hyunwoo Kim

commit fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless") moved the ->pending flush from __exit_signal() to release_task(), where it runs without ->siglock. The justification was:

after the exiting task passes __exit_signal() lock_task_sighand() can't succeed and pid_task(tmr->it_pid) will return NULL

That second half does not hold for the old group leader in a non-leader exec(). de_thread() calls exchange_tids() before release_task(leader), so

Indeed... Thanks a lot!

I need some time to (try to ;) fully understand the problem and your fix... I'll read your patch again tomorrow with a clear head.

Now... I hope that the next paragraph

    This means that after __exit_signal(tsk) nobody can play with tsk->pending
    or (if group_dead) with tsk->signal->shared_pending,

from the changelog is still true, so the only problem is that it is not safe to play with q->list, right?

Quoted 4 lines

kernel/signal.c void flush_sigqueue(struct sigpending *queue) · line 490

Quoted 10 lines

kernel/signal.c +5 -1

@@ -482,7 +482,11 @@ void flush_sigqueue(struct sigpending *queue)
 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+		/*
+		 * Pairs with the list_empty() in posixtimer_send_sigqueue().
+		 * release_task() gets here without ->siglock.
+		 */
+		list_del_init_careful(&q->list);
 		__sigqueue_free(q);

Can't we avoid list_del_init() altogether? Can't flush_sigqueue() simply do

	list_for_each_entry(q, &pending->list, list)
		__sigqueue_free(q);

?

Quoted 3 lines · Oleg Nesterov

Can't we avoid list_del_init() altogether? Can't flush_sigqueue() simply do

Quoted 4 lines

	list_for_each_entry(q, &pending->list, list)
		__sigqueue_free(q);

?

__sigqueue_free() does kmem_cache_free() for anything which is not PREALLOC, so the iterator reads q->list.next after it is freed.

And flush_signals() and selinux_bprm_committed_creds() call it on live tasks, so the queue has to end up empty.

So,

	list_for_each_entry_safe(q, n, &queue->list, list)
		__sigqueue_free(q);
	INIT_LIST_HEAD(&queue->list);

If you are fine with it, could you submit this patch yourself? I am also attaching the reproducer and the mdelay diff. I hope they help.

kernel/signal.c void flush_sigqueue(struct sigpending *queue) · line 486

Quoted 11 lines · Hyunwoo Kim

kernel/signal.c +2 -1

index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
@@ -482,7 +482,11 @@ void flush_sigqueue(struct sigpending *queue)
 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+		/*
+		 * Pairs with the list_empty() in posixtimer_send_sigqueue().

No. That list_empty() would need to be changed to list_empty_careful() to be correct on weakly ordered architectures.

Aside of that I'm not convinced that this is the right way to handle this as it cures the symptom and not the underlying problem. Let me stare at this some more.

fs/exec.c +0 -0

@@ -983,6 +983,18 @@ static int de_thread(struct task_struct
 		}
 
 		/*

Quoted 6 lines · Thomas Gleixner

fs/exec.c +0 -0

@@ -983,6 +983,18 @@ static int de_thread(struct task_struct
 		}
 

fs/exec.c static int de_thread(struct task_struct · line 995

Quoted 11 lines

fs/exec.c +10 -0

 		/*
+		 * Ensure that POSIX timer SIGEV_THREAD_ID signals pending for
+		 * the former leader are removed under sighand::siglock _before_
+		 * taking over the leader's TID. Otherwise the lockless cleanup
+		 * in release_task() can race against a concurrent signal
+		 * delivery to the new leader. The former leader has PF_EXITING
+		 * set which prevents queueing of SIGEV_THREAD_ID signals up to
+		 * the point where it's sighand gets cleared.
+		 */
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);

Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any changes, no?

Quoted 8 lines · Oleg Nesterov

Quoted 6 lines · Thomas Gleixner

fs/exec.c +0 -0

@@ -983,6 +983,18 @@ static int de_thread(struct task_struct
 		}
 

Quoted 11 lines

fs/exec.c static int de_thread(struct task_struct · line 995

Quoted 11 lines

fs/exec.c +10 -0

 		/*
+		 * Ensure that POSIX timer SIGEV_THREAD_ID signals pending for
+		 * the former leader are removed under sighand::siglock _before_
+		 * taking over the leader's TID. Otherwise the lockless cleanup
+		 * in release_task() can race against a concurrent signal
+		 * delivery to the new leader. The former leader has PF_EXITING
+		 * set which prevents queueing of SIGEV_THREAD_ID signals up to
+		 * the point where it's sighand gets cleared.
+		 */
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);

scoped_guard(spinlock_irq) is not right. This needs scoped_guard(spinlock), the code runs with irqs disabled.

Quoted 3 lines

Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any changes, no?

IOW. Unless I am totally confused, we only need to flush the SIGQUEUE_PREALLOC sigqueue's which were sent to the (old) leader before it changed its pid. So we can do this

fs/exec.c +0 -0

index a14f28b15607..550367e7fe6c 100644

Quoted 9 lines · Oleg Nesterov

fs/exec.c +0 -0

index a14f28b15607..550367e7fe6c 100644
@@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 

fs/exec.c static int de_thread(struct task_struct *tsk) · line 1034

Quoted 3 lines

fs/exec.c +3 -0

+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+

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?

Quoted 16 lines · Oleg Nesterov

Quoted 3 lines

Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any changes, no?

IOW. Unless I am totally confused, we only need to flush the SIGQUEUE_PREALLOC sigqueue's which were sent to the (old) leader before it changed its pid. So we can do this

fs/exec.c +0 -0

index a14f28b15607..550367e7fe6c 100644
@@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 

fs/exec.c static int de_thread(struct task_struct *tsk) · line 1034

Quoted 3 lines

fs/exec.c +3 -0

+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+

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?

Quoted 17 lines · Frederic Weisbecker

Quoted 16 lines · Oleg Nesterov

Quoted 3 lines

Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any changes, no?

IOW. Unless I am totally confused, we only need to flush the SIGQUEUE_PREALLOC sigqueue's which were sent to the (old) leader before it changed its pid. So we can do this

fs/exec.c +0 -0

index a14f28b15607..550367e7fe6c 100644
@@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 

Quoted 9 lines

fs/exec.c static int de_thread(struct task_struct *tsk) · line 1034

Quoted 3 lines

fs/exec.c +3 -0

+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+

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. :)

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines

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).

Quoted 4 lines · Oleg Nesterov

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.

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;

        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

and in posixtimer_send_sigqueue()

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

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

Quoted 8 lines · Thomas Gleixner

Quoted 4 lines · Oleg Nesterov

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.

Quoted 8 lines

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()

{
        ...

Quoted 4 lines

	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.

Quoted 7 lines

        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.

Quoted 7 lines

#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?

Quoted 7 lines

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

Quoted 6 lines · Oleg Nesterov

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

Sure, but that's an orthogonal optimization once we fixed the exec() mess :)

Thomas,

I am already sleeping, but let me ask anyway

Quoted 9 lines · Thomas Gleixner

Quoted 6 lines · Oleg Nesterov

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

Quoted 2 lines

Sure, but that's an orthogonal optimization once we fixed the exec() mess :)

I am almost sure I missed something again. But I thought that this "optimization" can also fix the exec() mess we discuss in this thread?

No?

Could the posix timers cleanup be moved from __exit_signal in release_task (which is really for cleanup for zombies but has been historically abused because it was the only place that knew when the whole group was dead), into somewhere in do_exit?

Say near where hrtimers_cancel and exit_itimers are called.

Then perhaps move the posix timer disabling before de_thread?

Quoted 6 lines · Eric W. Biederman

Could the posix timers cleanup be moved from __exit_signal in release_task (which is really for cleanup for zombies but has been historically abused because it was the only place that knew when the whole group was dead), into somewhere in do_exit?

Say near where hrtimers_cancel and exit_itimers are called.

That's only for the group_dead case in do_exit().

But a single task existing from a process needs to clean up task::pending, i.e. signals which are targeted at the exiting task.

The safe and obvious place is to do that is _after_ setting task::sighand to NULL because that ensures that no new signal can be queued and nothing can touch task::pending anymore.

Quoted 1 line

Then perhaps move the posix timer disabling before de_thread?

That does not work because between that and de_thread() any thread of the thread group can create a new posix timer unless we prevent that somehow in timer_create().

So in any case we need some mechanism in posixtimer related code to handle this situation gracefully.

Quoted 3 lines · Thomas Gleixner

The safe and obvious place is to do that is _after_ setting task::sighand to NULL because that ensures that no new signal can be queued and nothing can touch task::pending anymore.

Not really. Using release_task (which is what is called when a zombie is reaped) for anything except cleaning up state that a zombie needs is a bit of a misfeature. Timers should not be active in a zombie. Signals also should be deactivated long before then.

The obvious place to clean up task::pending i.e. signals is in exit_signals().

I expect if I read through the history again that I would find that exit_signals() used to call flush_sigqueue, and that during the addition of posix thread signal handling flush_sigqueue was moved into __exit_signal in release_task because knowing if the entire thread group is dead was not available during that part of 2.5.

We should honor PF_EXITING on a task and simply stop delivering signals to it. Today the code goes halfway there and does not set sig-pending after PF_EXITING is set.

There is the goofy case that we need to be able to deliver signals to the entire process through a zombie thread (in particular a zombie thread group leader). That goofy case unfortunately means that except for signals to just the thread we have to deliver signals when PF_EXITING is set. That goofy case also unfortunately means that sighand_struct needs to be retained past the point where signals are delivered.

Quoted 9 lines · Eric W. Biederman

The obvious place to clean up task::pending i.e. signals is in exit_signals().

I expect if I read through the history again that I would find that exit_signals() used to call flush_sigqueue, and that during the addition of posix thread signal handling flush_sigqueue was moved into __exit_signal in release_task because knowing if the entire thread group is dead was not available during that part of 2.5.

Quoted 3 lines

We should honor PF_EXITING on a task and simply stop delivering signals to it. Today the code goes halfway there and does not set sig-pending after PF_EXITING is set.

If flushing tsk::pending in exit_signals() is safe and stopping signals to be queued when PF_EXITING is observed under sighand lock, then sure that's the right thing to do. I'll look into that tomorrow.

Quoted 14 lines · Thomas Gleixner

Quoted 12 lines · Eric W. Biederman

The obvious place to clean up task::pending i.e. signals is in exit_signals().

I expect if I read through the history again that I would find that exit_signals() used to call flush_sigqueue, and that during the addition of posix thread signal handling flush_sigqueue was moved into __exit_signal in release_task because knowing if the entire thread group is dead was not available during that part of 2.5.

We should honor PF_EXITING on a task and simply stop delivering signals to it. Today the code goes halfway there and does not set sig-pending after PF_EXITING is set.

Quoted 3 lines

If flushing tsk::pending in exit_signals() is safe and stopping signals to be queued when PF_EXITING is observed under sighand lock, then sure that's the right thing to do. I'll look into that tomorrow.

By some definition of tomorrow. :)

Something like the below?

---

kernel/exit.c +6 -5

@@ -299,12 +299,13 @@ void release_task(struct task_struct *p)
 	free_pids(post.pids);
 	release_thread(p);
 	/*
-	 * This task was already removed from the process/thread/pid lists
-	 * and lock_task_sighand(p) can't succeed. Nobody else can touch
-	 * ->pending or, if group dead, signal->shared_pending. We can call
-	 * flush_sigqueue() lockless.
+	 * This task was already removed from the process/thread/pid lists and
+	 * lock_task_sighand(p) can't succeed. If it's the group leader then
+	 * flush tsk->signal->shared_pending. tsk->pending has been flushed
+	 * already in exit_signals(). Nothing else can touch
+	 * signal->shared_pending anymore, so flush_sigqueue() can be invoked
+	 * lockless.
 	 */
-	flush_sigqueue(&p->pending);
 	if (thread_group_leader(p))
 		flush_sigqueue(&p->signal->shared_pending);
 

kernel/signal.c +22 -1

@@ -1030,6 +1030,10 @@ static int __send_signal_locked(int sig,
 	lockdep_assert_held(&t->sighand->siglock);
 
 	result = TRACE_SIGNAL_IGNORED;
+
+	if (unlikely(type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		goto ret;
+
 	if (!prepare_signal(sig, t, force))
 		goto ret;
 

@@ -1990,6 +1994,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().

@@ -3118,6 +3125,16 @@ static void retarget_shared_pending(stru
 	}
 }
 
+/*
+ * tsk::flags has PF_EXITING set which prevents signals to be queued for on
+ * tsk::pending. Nothing else can touch the tsk::pending anymore so it can be
+ * flushed lockless.
+ */
+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

@@ -3130,8 +3147,10 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+			tsk->flags |= PF_EXITING;
 		cgroup_threadgroup_change_end(tsk);
+		flush_pending_unlocked(tsk);
 		return;
 	}
 

@@ -3157,6 +3176,8 @@ void exit_signals(struct task_struct *ts
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_pending_unlocked(tsk);
+
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.

Quoted 1 line · Thomas Gleixner

Something like the below?

Yes.

It all comes after
ptrace_event(PTRACE_EVENT_EXIT, code) and
coredump_task_exit(tsk, core_state)
so should be completely invisible to userspace.

I don't see any problems with your proposed patch.

I am pondering what it would take to move
"flush_sigqueue(&p->signal->shared_pending);"
into do_exit in the group_dead case.

Perhaps exit_signals could perform the decrement of signal->live and return group_dead. If so all of the work could be performed in exit_signals(). As a follow-on change of cource.

Hyunwoo debugged the following KASAN UAF splat:

  BUG: KASAN: slab-use-after-free in __send_signal_locked+0xb27/0xba0
  Write of size 8 at addr ffff888007ed80c8 by task poc/79
  ...
  Call Trace:
   __send_signal_locked+0xb27/0xba0
   do_send_sig_info+0xa7/0x160
   do_send_specific+0x76/0xa0
   __x64_sys_tgkill+0x193/0x270
  ...
  Allocated by task 80:
   do_timer_create+0x1a4/0x1030
   __x64_sys_timer_create+0x145/0x190
  ...
  Freed by task 12:
   kmem_cache_free_bulk+0x1f8/0x4a0
   kvfree_rcu_bulk+0x14f/0x1c0
   kfree_rcu_work+0x128/0x1a0
  ...
  Last potentially related work creation:
   kvfree_call_rcu+0x39/0x390
   __flush_itimer_signals+0x211/0x320
   flush_itimer_signals+0x47/0x90
   begin_new_exec+0xa6b/0x28c0

It turned out that this happens with a non-leader exec() as Hyunwoo explained:

de_thread() calls exchange_tids() before release_task(leader), so the struct pid held by a SIGEV_THREAD_ID timer created against the leader's tid now points to the thread which called execve(). pid_task() returns that thread and lock_task_sighand() on it succeeds.

If the timer signal is blocked, its sigqueue stays queued on the leader's task::pending. The next expiry of that timer can then run while release_task() flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads list_head::next. list_del_init() is not atomic and INIT_LIST_HEAD() stores list_head::next before list_head::prev, so the check can pass in between. list_add_tail() queues the entry on the task::pending of the live thread, and the list_head::prev store from the flush then overwrites the list_head::prev link that list_add_tail() has just set.

__flush_itimer_signals() does not undo that either. With list_head::prev pointing at the entry itself, its list_del_init() only stores the same values again, so the entry is not removed from the list. It is still there after the last reference is dropped and the timer is freed by RCU, and the list_add_tail() of a later tgkill() follows that list_head::prev into the freed timer.

This problem is due to a recent commit which moved the sigqueue flush out of the sighand lock held region. Before that it was properly serialized.

Hyonwoo proposed to fix this by using list_del_init_careful(), but that just papers over the underlying problem. After some disucssions and various attempts to solve it, Eric pointed out that there is no reason to flush task::pending late in release_task() and it should be done in exit_signals() already.

As nothing can collect and deliver signals which are queued in a dying task's pending queue, there is no reason to delay it further.

But it has to be ensured that no signals can be queued into it after that point. exit_signals() sets PF_EXITING in task::flags, which can be used as an indicator for this.

Cure it by:

- Preventing signal queueing for task private signals (PIDTYPE_PID) when the task has PF_EXITING set in __send_signal_locked() and in posixtimer_send_sigqueue().

- Protecting the unlocked setting of PF_EXITING in exit_signals() for the task group empty and the group exit case with sighand lock

- Flushing task::pending signals right there.

This can be done unlocked because PF_EXITING prevents further signals to be queued and there is no other code which accesses task::pending.

Fixes fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")

Reported-by Hyunwoo Kim <[email protected]>

Debugged-by Hyunwoo Kim <[email protected]>

Suggested-by "Eric W. Biederman" <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

Closes https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel open here

---
 kernel/exit.c   |   11 ++++++-----
 kernel/signal.c |   23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

kernel/exit.c +6 -5

@@ -299,12 +299,13 @@ void release_task(struct task_struct *p)
 	free_pids(post.pids);
 	release_thread(p);
 	/*
-	 * This task was already removed from the process/thread/pid lists
-	 * and lock_task_sighand(p) can't succeed. Nobody else can touch
-	 * ->pending or, if group dead, signal->shared_pending. We can call
-	 * flush_sigqueue() lockless.
+	 * This task was already removed from the process/thread/pid lists and
+	 * lock_task_sighand(p) can't succeed. If it's the group leader then
+	 * flush tsk->signal->shared_pending. tsk->pending has been flushed
+	 * already in exit_signals(). Nothing else can touch
+	 * signal->shared_pending anymore, so flush_sigqueue() can be invoked
+	 * lockless.
 	 */
-	flush_sigqueue(&p->pending);
 	if (thread_group_leader(p))
 		flush_sigqueue(&p->signal->shared_pending);
 

kernel/signal.c +19 -1

+
 	if (!prepare_signal(sig, t, force))
 		goto ret;
 

@@ -1990,6 +1994,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().

@@ -3118,6 +3125,16 @@ static void retarget_shared_pending(stru
 	}
 }
 
+/*
+ * tsk::flags has PF_EXITING set which prevents signals to be queued on
+ * tsk::pending. Nothing else can touch tsk::pending anymore so it can be
+ * flushed lockless.
+ */
+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

@@ -3130,8 +3147,10 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+			tsk->flags |= PF_EXITING;
 		cgroup_threadgroup_change_end(tsk);
+		flush_pending_unlocked(tsk);
 		return;
 	}
 

@@ -3157,6 +3176,8 @@ void exit_signals(struct task_struct *ts
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_pending_unlocked(tsk);
+
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.

kernel/signal.c +5 -0

+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

Quoted 12 lines · Thomas Gleixner

kernel/signal.c +5 -0

+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

@@ -3130,8 +3147,10 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 

kernel/signal.c void exit_signals(struct task_struct *ts · line 3153

Quoted 6 lines

kernel/signal.c +3 -1

 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+			tsk->flags |= PF_EXITING;
 		cgroup_threadgroup_change_end(tsk);
+		flush_pending_unlocked(tsk);

Hmm... the exiting thread is still visible to for_each_thread(). Can't this flush_pending_unlocked() race with (say) do_sigaction() ?

Hyunwoo debugged the following KASAN UAF splat:

  BUG: KASAN: slab-use-after-free in __send_signal_locked+0xb27/0xba0
  Write of size 8 at addr ffff888007ed80c8 by task poc/79
  ...
  Call Trace:
   __send_signal_locked+0xb27/0xba0
   do_send_sig_info+0xa7/0x160
   do_send_specific+0x76/0xa0
   __x64_sys_tgkill+0x193/0x270
  ...
  Allocated by task 80:
   do_timer_create+0x1a4/0x1030
   __x64_sys_timer_create+0x145/0x190
  ...
  Freed by task 12:
   kmem_cache_free_bulk+0x1f8/0x4a0
   kvfree_rcu_bulk+0x14f/0x1c0
   kfree_rcu_work+0x128/0x1a0
  ...
  Last potentially related work creation:
   kvfree_call_rcu+0x39/0x390
   __flush_itimer_signals+0x211/0x320
   flush_itimer_signals+0x47/0x90
   begin_new_exec+0xa6b/0x28c0

It turned out that this happens with a non-leader exec() as Hyunwoo explained:

de_thread() calls exchange_tids() before release_task(leader), so the struct pid held by a SIGEV_THREAD_ID timer created against the leader's tid now points to the thread which called execve(). pid_task() returns that thread and lock_task_sighand() on it succeeds.

If the timer signal is blocked, its sigqueue stays queued on the leader's task::pending. The next expiry of that timer can then run while release_task() flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads list_head::next. list_del_init() is not atomic and INIT_LIST_HEAD() stores list_head::next before list_head::prev, so the check can pass in between. list_add_tail() queues the entry on the task::pending of the live thread, and the list_head::prev store from the flush then overwrites the list_head::prev link that list_add_tail() has just set.

__flush_itimer_signals() does not undo that either. With list_head::prev pointing at the entry itself, its list_del_init() only stores the same values again, so the entry is not removed from the list. It is still there after the last reference is dropped and the timer is freed by RCU, and the list_add_tail() of a later tgkill() follows that list_head::prev into the freed timer.

This problem is due to a recent commit which moved the sigqueue flush out of the sighand lock held region. Before that it was properly serialized.

Hyonwoo proposed to fix this by using list_del_init_careful(), but that just papers over the underlying problem. After some disucssions and various attempts to solve it, Eric pointed out that there is no reason to flush task::pending late in release_task() and it should be done in exit_signals() already.

As nothing can collect and deliver signals which are queued in a dying task's pending queue, there is no reason to delay it further.

But it has to be ensured that no signals can be queued into it after that point. exit_signals() sets PF_EXITING in task::flags, which can be used as an indicator for this.

Cure it by:

- Preventing signal queueing for task private signals (PIDTYPE_PID) when the task has PF_EXITING set in __send_signal_locked() and in posixtimer_send_sigqueue().

- Protecting the unlocked setting of PF_EXITING in exit_signals() for the task group empty and the group exit case with sighand lock

- Flushing task::pending signals right there.

This can be done unlocked because PF_EXITING prevents further signals to be queued and there is no other code which accesses task::pending.

Fixes fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")

Reported-by Hyunwoo Kim <[email protected]>

Debugged-by Hyunwoo Kim <[email protected]>

Suggested-by "Eric W. Biederman" <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

Closes https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel open here

---
 kernel/exit.c   |   11 ++++++-----
 kernel/signal.c |   23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

kernel/exit.c +6 -5

@@ -299,12 +299,13 @@ void release_task(struct task_struct *p)
 	free_pids(post.pids);
 	release_thread(p);
 	/*
-	 * This task was already removed from the process/thread/pid lists
-	 * and lock_task_sighand(p) can't succeed. Nobody else can touch
-	 * ->pending or, if group dead, signal->shared_pending. We can call
-	 * flush_sigqueue() lockless.
+	 * This task was already removed from the process/thread/pid lists and
+	 * lock_task_sighand(p) can't succeed. If it's the group leader then
+	 * flush tsk->signal->shared_pending. tsk->pending has been flushed
+	 * already in exit_signals(). Nothing else can touch
+	 * signal->shared_pending anymore, so flush_sigqueue() can be invoked
+	 * lockless.
 	 */
-	flush_sigqueue(&p->pending);
 	if (thread_group_leader(p))
 		flush_sigqueue(&p->signal->shared_pending);
 

kernel/signal.c +20 -1

@@ -1030,6 +1030,10 @@ static int __send_signal_locked(int sig,
 	lockdep_assert_held(&t->sighand->siglock);
 
 	result = TRACE_SIGNAL_IGNORED;
+
+	if (unlikely(type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		goto ret;
+
 	if (!prepare_signal(sig, t, force))
 		goto ret;
 

@@ -1990,6 +1994,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().

@@ -3118,6 +3125,16 @@ static void retarget_shared_pending(stru
 	}
 }
 
+/*
+ * tsk::flags has PF_EXITING set which prevents signals to be queued on
+ * tsk::pending. Nothing else can touch tsk::pending anymore so it can be
+ * flushed lockless.
+ */
+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

@@ -3130,8 +3147,10 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+			tsk->flags |= PF_EXITING;
 		cgroup_threadgroup_change_end(tsk);
+		flush_pending_unlocked(tsk);
 		return;
 	}
 

Le Mon, Aug 31, 2026 at 12:50:46PM +0200, Thomas Gleixner a écrit :

Is the following situation possible?

CPU 0                                CPU 1                   CPU 2
-----                                -----                   -----

exit_signals()
spin_lock(sighand)

   tsk->flags |= PF_EXITING;
   spin_unlock(sighand)

   flush_pending_unlocked(tsk);

...
do_task_dead()

                                     de_thread()
                                        // acquired tsk->flags
                                        // and signal flushed
                                        // through tasklist_lock
                                        transfer_pid()
                                                           
                                                           posix_timer_fn()
                                                              posixtimer_send_sigqueue()
                                                                 // happen to see new leader
                                                                 t = posixtimer_get_target(tmr)
                                                                 lock_task_sighand()
                                                                 // passes !PF_EXITING cond
                                                                 // but what makes sure that flush_pending_unlocked()
                                                                 // is observed here? So that signal list isn't messed up
                                                                 // pid_task() doesn't have acquire semantics

Quoted 16 lines · Frederic Weisbecker

Le Mon, Aug 31, 2026 at 12:50:46PM +0200, Thomas Gleixner a écrit :

Is the following situation possible?

CPU 0                                CPU 1                   CPU 2
-----                                -----                   -----

exit_signals()
spin_lock(sighand)

   tsk->flags |= PF_EXITING;
   spin_unlock(sighand)

   flush_pending_unlocked(tsk);

...
do_task_dead()

Quoted 15 lines

                                     de_thread()
                                        // acquired tsk->flags
                                        // and signal flushed
                                        // through tasklist_lock
                                        transfer_pid()
                                                           
                                                           posix_timer_fn()
                                                              posixtimer_send_sigqueue()
                                                                 // happen to see new leader
                                                                 t = posixtimer_get_target(tmr)
                                                                 lock_task_sighand()
                                                                 // passes !PF_EXITING cond
                                                                 // but what makes sure that flush_pending_unlocked()
                                                                 // is observed here? So that signal list isn't messed up
                                                                 // pid_task() doesn't have acquire semantics

On some far fetched completely out of order CPU that might be possible, but it's moot as it's already established that we can't do that lockless at this point.

Quoted 3 lines · Thomas Gleixner

On some far fetched completely out of order CPU that might be possible, but it's moot as it's already established that we can't do that lockless at this point.

Sorry I probably missed something in the discussion, what can't we do lockless?

Hyunwoo debugged the following KASAN UAF splat:

  BUG: KASAN: slab-use-after-free in __send_signal_locked+0xb27/0xba0
  Write of size 8 at addr ffff888007ed80c8 by task poc/79
  ...
  Call Trace:
   __send_signal_locked+0xb27/0xba0
   do_send_sig_info+0xa7/0x160
   do_send_specific+0x76/0xa0
   __x64_sys_tgkill+0x193/0x270
  ...
  Allocated by task 80:
   do_timer_create+0x1a4/0x1030
   __x64_sys_timer_create+0x145/0x190
  ...
  Freed by task 12:
   kmem_cache_free_bulk+0x1f8/0x4a0
   kvfree_rcu_bulk+0x14f/0x1c0
   kfree_rcu_work+0x128/0x1a0
  ...
  Last potentially related work creation:
   kvfree_call_rcu+0x39/0x390
   __flush_itimer_signals+0x211/0x320
   flush_itimer_signals+0x47/0x90
   begin_new_exec+0xa6b/0x28c0

It turned out that this happens with a non-leader exec() as Hyunwoo explained:

de_thread() calls exchange_tids() before release_task(leader), so the struct pid held by a SIGEV_THREAD_ID timer created against the leader's tid now points to the thread which called execve(). pid_task() returns that thread and lock_task_sighand() on it succeeds.

If the timer signal is blocked, its sigqueue stays queued on the leader's task::pending. The next expiry of that timer can then run while release_task() flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads list_head::next. list_del_init() is not atomic and INIT_LIST_HEAD() stores list_head::next before list_head::prev, so the check can pass in between. list_add_tail() queues the entry on the task::pending of the live thread, and the list_head::prev store from the flush then overwrites the list_head::prev link that list_add_tail() has just set.

__flush_itimer_signals() does not undo that either. With list_head::prev pointing at the entry itself, its list_del_init() only stores the same values again, so the entry is not removed from the list. It is still there after the last reference is dropped and the timer is freed by RCU, and the list_add_tail() of a later tgkill() follows that list_head::prev into the freed timer.

This problem is due to a recent commit which moved the sigqueue flush out of the sighand lock held region. Before that it was properly serialized.

Hyonwoo proposed to fix this by using list_del_init_careful(), but that just papers over the underlying problem. After some disucssions and various attempts to solve it, Eric pointed out that there is no reason to flush task::pending late in release_task() and it should be done in exit_signals() already.

As nothing can collect and deliver signals which are queued in a dying task's pending queue, there is no reason to delay it further.

But it has to be ensured that no signals can be queued into it after that point. exit_signals() sets PF_EXITING in task::flags, which can be used as an indicator for this.

Cure it by:

- Preventing signal queueing for task private signals (PIDTYPE_PID) when the task has PF_EXITING set in __send_signal_locked() and in posixtimer_send_sigqueue().

- Protecting the unlocked setting of PF_EXITING in exit_signals() for the task group empty and the group exit case with sighand lock

- Flushing task::pending signals right there.

This can be done unlocked because PF_EXITING prevents further signals to be queued and there is no other code which accesses task::pending.

Fixes fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")

Reported-by Hyunwoo Kim <[email protected]>

Debugged-by Hyunwoo Kim <[email protected]>

Suggested-by "Eric W. Biederman" <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

Closes https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel open here

---
 kernel/exit.c   |   11 ++++++-----
 kernel/signal.c |   23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

kernel/exit.c +6 -5

@@ -299,12 +299,13 @@ void release_task(struct task_struct *p)
 	free_pids(post.pids);
 	release_thread(p);
 	/*
-	 * This task was already removed from the process/thread/pid lists
-	 * and lock_task_sighand(p) can't succeed. Nobody else can touch
-	 * ->pending or, if group dead, signal->shared_pending. We can call
-	 * flush_sigqueue() lockless.
+	 * This task was already removed from the process/thread/pid lists and
+	 * lock_task_sighand(p) can't succeed. If it's the group leader then
+	 * flush tsk->signal->shared_pending. tsk->pending has been flushed
+	 * already in exit_signals(). Nothing else can touch
+	 * signal->shared_pending anymore, so flush_sigqueue() can be invoked
+	 * lockless.
 	 */
-	flush_sigqueue(&p->pending);
 	if (thread_group_leader(p))
 		flush_sigqueue(&p->signal->shared_pending);
 

kernel/signal.c +22 -1

@@ -1030,6 +1030,10 @@ static int __send_signal_locked(int sig,
 	lockdep_assert_held(&t->sighand->siglock);
 
 	result = TRACE_SIGNAL_IGNORED;
+
+	if (unlikely(type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		goto ret;
+
 	if (!prepare_signal(sig, t, force))
 		goto ret;
 

@@ -1990,6 +1994,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().

@@ -3118,6 +3125,16 @@ static void retarget_shared_pending(stru
 	}
 }
 
+/*
+ * tsk::flags has PF_EXITING set which prevents signals to be queued on
+ * tsk::pending. Nothing else can touch tsk::pending anymore so it can be
+ * flushed lockless.
+ */
+static inline void flush_pending_unlocked(struct task_struct *tsk)
+{
+	flush_sigqueue(&tsk->pending);
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;

@@ -3130,8 +3147,10 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+			tsk->flags |= PF_EXITING;
 		cgroup_threadgroup_change_end(tsk);
+		flush_pending_unlocked(tsk);
 		return;
 	}
 

@@ -3157,6 +3176,8 @@ void exit_signals(struct task_struct *ts
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_pending_unlocked(tsk);
+
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.

This changes partially fixes another bug. Recursive UCOUNT_RLIMIT_SIGPENDING should be decremented when the process exits and not when the process is reaped.

Others have noticed possible races flushing the siqueue not holding siglock.

If I read the history correctly in flush_sigqueue with irqs disabled can trigger the NMI lock-up detector. So flush_sigqueue was moved outside of siglock_irq.

Apparently it took KASAN to make kmem_cache_free slow enough to trigger the lock-up detector.

The fix to avoid the lock-up detector was not comprehensive and flush_sigqueue is still called in many places with irqs disabled. So if necessary the code can probably just take siglock.

We can also avoid problems by updating the loops that go:
for_each_thread(p, q)

	flush_sigqueue_mask(p, &flush, &t->pending)

To include

	if (t->flags & PF_EXITING)
        	continue;

Or perhaps better tweak flush_sigqueue_mask to take t (and not p) and perform the test of PF_EXITING there. The only current uses I see of the passed in task is to get a reference to signal_struct.

Quoted 7 lines · Eric W. Biederman

This changes partially fixes another bug. Recursive UCOUNT_RLIMIT_SIGPENDING should be decremented when the process exits and not when the process is reaped.

Others have noticed possible races flushing the siqueue not holding siglock.

Yes. I doesn't work.

Quoted 10 lines

If I read the history correctly in flush_sigqueue with irqs disabled can trigger the NMI lock-up detector. So flush_sigqueue was moved outside of siglock_irq.

Apparently it took KASAN to make kmem_cache_free slow enough to trigger the lock-up detector.

The fix to avoid the lock-up detector was not comprehensive and flush_sigqueue is still called in many places with irqs disabled. So if necessary the code can probably just take siglock.

Right, invoke flush_sigqueue() right after setting PF_EXITING.

But we can be smarter than that. See below.

Quoted 8 lines

We can also avoid problems by updating the loops that go:
for_each_thread(p, q)

	flush_sigqueue_mask(p, &flush, &t->pending)

To include

	if (t->flags & PF_EXITING)
        	continue;

Quoted 3 lines

Or perhaps better tweak flush_sigqueue_mask to take t (and not p) and perform the test of PF_EXITING there. The only current uses I see of the passed in task is to get a reference to signal_struct.

Correct. Though that check would have to be limited to flushing tsk::pending not signal::shared_pending.

Thanks,

tglx
---

kernel/signal.c +32 -24

@@ -457,30 +457,44 @@ static void __sigqueue_free(struct sigqu
 	kmem_cache_free(sigqueue_cachep, q);
 }
 
-void flush_sigqueue(struct sigpending *queue)
+static void flush_sigqueue_list(struct list_head *head)
 {
-	struct sigqueue *q;
+	struct sigqueue *q, *tmp;
 
-	sigemptyset(&queue->signal);
-	while (!list_empty(&queue->list)) {
-		q = list_entry(queue->list.next, struct sigqueue , list);
+	list_for_each_entry_safe(q, tmp, head, list) {
 		list_del_init(&q->list);
 		__sigqueue_free(q);
 	}
 }
 
+void flush_sigqueue(struct sigpending *queue)
+{
+	sigemptyset(&queue->signal);
+	flush_sigqueue_list(&queue->list);
+}
+
+static void sigqueue_splice_pending(struct sigpending *queue, struct list_head *head)
+{
+	sigemptyset(&queue->signal);
+	list_splice_init(&queue->list, head);
+}
+
 /*
  * Flush all pending signals for this kthread.
  */
 void flush_signals(struct task_struct *t)
 {
-	unsigned long flags;
+	LIST_HEAD(pending);
+	LIST_HEAD(shared);
 
-	spin_lock_irqsave(&t->sighand->siglock, flags);
-	clear_tsk_thread_flag(t, TIF_SIGPENDING);
-	flush_sigqueue(&t->pending);
-	flush_sigqueue(&t->signal->shared_pending);
-	spin_unlock_irqrestore(&t->sighand->siglock, flags);
+	scoped_guard(spinlock_irqsave, &t->sighand->siglock) {
+		clear_tsk_thread_flag(t, TIF_SIGPENDING);
+		sigqueue_splice_pending(&t->pending, &pending);
+		sigqueue_splice_pending(&t->signal->shared_pending, &shared);
+	}
+
+	flush_sigqueue_list(&pending);
+	flush_sigqueue_list(&shared);
 }
 EXPORT_SYMBOL(flush_signals);
 

@@ -3125,18 +3139,9 @@ static void retarget_shared_pending(stru
 	}
 }
 
-/*
- * tsk::flags has PF_EXITING set which prevents signals to be queued on
- * tsk::pending. Nothing else can touch tsk::pending anymore so it can be
- * flushed lockless.
- */
-static inline void flush_pending_unlocked(struct task_struct *tsk)
-{
-	flush_sigqueue(&tsk->pending);
-}
-
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
 	sigset_t unblocked;
 

@@ -3147,10 +3152,12 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		scoped_guard(spinlock_irq, &tsk->sighand->siglock)
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
 			tsk->flags |= PF_EXITING;
+			sigqueue_splice_pending(&tsk->pending, &sigq_list);
+		}
 		cgroup_threadgroup_change_end(tsk);
-		flush_pending_unlocked(tsk);
+		flush_sigqueue_list(&sigq_list);
 		return;
 	}
 

@@ -3160,6 +3167,7 @@ void exit_signals(struct task_struct *ts
 	 * see wants_signal(), do_signal_stop().
 	 */
 	tsk->flags |= PF_EXITING;
+	sigqueue_splice_pending(&tsk->pending, &sigq_list);
 
 	cgroup_threadgroup_change_end(tsk);
 

@@ -3176,7 +3184,7 @@ void exit_signals(struct task_struct *ts
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
-	flush_pending_unlocked(tsk);
+	flush_sigqueue_list(&sigq_list);
 
 	/*
 	 * If group stop has completed, deliver the notification.  This

If the timer signal is blocked, its sigqueue stays queued on the leader's task::pending. The next expiry of that timer can then run while release_task() flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads list_head::next.

Quoted 7 lines · Thomas Gleixner

If the timer signal is blocked, its sigqueue stays queued on the leader's task::pending. The next expiry of that timer can then run while release_task() flushes the queue.

posixtimer_send_sigqueue() checks whether the sigqueue is already queued with a plain list_empty(), which only reads list_head::next.

If timer->sigq is queued on T->pending list, then T has a reference. Even if this timer is destroyed, it and its ->sigq can't go away until __sigqueue_free() -> posixtimer_sigqueue_putref(timer->sigq). Right?

So, If we change posixtimer_send_sigqueue() to check PF_EXITING and return, then why do we need other changes?

Ah, and I just noticed...

kernel/signal.c void posixtimer_send_sigqueue(struct k_i · line 2008

Quoted 6 lines

kernel/signal.c +2 -0

@@ -1990,6 +2004,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;

this lacks unlock_sighand().

kernel/signal.c void posixtimer_send_sigqueue(struct k_i · line 2008

Quoted 6 lines

kernel/signal.c +2 -0

@@ -1990,6 +2004,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+		return;

this lacks unlock_sighand().

(quoted diff) +7 -1

@@ -3120,6 +3137,7 @@ static void retarget_shared_pending(stru
 
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
 	sigset_t unblocked;
 

@@ -3130,8 +3148,12 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
+			tsk->flags |= PF_EXITING;
+			sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
+		}
 		cgroup_threadgroup_change_end(tsk);
+		flush_sigqueue_list(&sigq_list);
 		return;
 	}
 

@@ -3141,6 +3163,7 @@ void exit_signals(struct task_struct *ts
 	 * see wants_signal(), do_signal_stop().
 	 */
 	tsk->flags |= PF_EXITING;
+	sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
 
 	cgroup_threadgroup_change_end(tsk);
 

as for the change on exit_signal,

Quoted 32 lines · Thomas Gleixner

kernel/signal.c +7 -1

@@ -3120,6 +3137,7 @@ static void retarget_shared_pending(stru
 
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
 	sigset_t unblocked;
 

@@ -3130,8 +3148,12 @@ void exit_signals(struct task_struct *ts
 	cgroup_threadgroup_change_begin(tsk);
 
 	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
+		scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
+			tsk->flags |= PF_EXITING;
+			sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
+		}
 		cgroup_threadgroup_change_end(tsk);
+		flush_sigqueue_list(&sigq_list);
 		return;
 	}
 

@@ -3141,6 +3163,7 @@ void exit_signals(struct task_struct *ts
 	 * see wants_signal(), do_signal_stop().
 	 */
 	tsk->flags |= PF_EXITING;
+	sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
 
 	cgroup_threadgroup_change_end(tsk);
 

kernel/signal.c void exit_signals(struct task_struct *ts · line 3187

Quoted 8 lines

kernel/signal.c +2 -0

@@ -3157,6 +3180,8 @@ void exit_signals(struct task_struct *ts
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_sigqueue_list(&sigq_list);
+
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.

This is subjective and mostly cosmetic, but what do you think about the alternative change below?

I won't insist, but to me both the patch and resulting code look a bit simpler this way.

Oleg.
---

kernel/signal.c +5 -6

@@ -3120,6 +3120,7 @@ static void retarget_shared_pending(struct task_struct *tsk, sigset_t *which)
 
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
 	sigset_t unblocked;
 

@@ -3129,21 +3130,18 @@ void exit_signals(struct task_struct *tsk)
 	 */
 	cgroup_threadgroup_change_begin(tsk);
 
-	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
-		cgroup_threadgroup_change_end(tsk);
-		return;
-	}
-
 	spin_lock_irq(&tsk->sighand->siglock);
 	/*
 	 * From now this task is not visible for group-wide signals,
 	 * see wants_signal(), do_signal_stop().
 	 */
 	tsk->flags |= PF_EXITING;
+	sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
 
 	cgroup_threadgroup_change_end(tsk);
 
+	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT))
+		goto out;
 	if (!task_sigpending(tsk))
 		goto out;

@@ -3157,6 +3155,7 @@ void exit_signals(struct task_struct *tsk)
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_sigqueue_list(&sigq_list);
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.

Quoted 2 lines · Oleg Nesterov

I won't insist, but to me both the patch and resulting code look a bit simpler this way.

I agree that simply removing the special case that could skip grabbing siglock is more maintainable. Just one last thing to think about.

Oleg it appears you were the one who added the special case to skip taking siglock. So if you aren't worried about us removing it then I am happy to see it go.

This is subjective and mostly cosmetic, but what do you think about the alternative change below?

I won't insist, but to me both the patch and resulting code look a bit simpler this way.

Quoted 5 lines · Oleg Nesterov

This is subjective and mostly cosmetic, but what do you think about the alternative change below?

I won't insist, but to me both the patch and resulting code look a bit simpler this way.

Yeah, though if we restructure the code then I rather prefer to get rid of the gotos and also move the cgroup...end() part out of the sighand lock held region to make that as short as possible.

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Oleg Nesterov

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

Quoted 5 lines

Quoted 2 lines

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

Sure, but that's an orthogonal optimization once we fixed the exec() mess :)

I am almost sure I missed something again. But I thought that this "optimization" can also fix the exec() mess we discuss in this thread?

I'm actually tempted to move the posix timer cleanup _before_ de_thread() and just make sure that no new timers can be created anymore.

Quoted 6 lines · Thomas Gleixner

fs/exec.c +0 -0

@@ -983,6 +983,18 @@ static int de_thread(struct task_struct
 		}
 

fs/exec.c static int de_thread(struct task_struct · line 995

Quoted 11 lines

fs/exec.c +10 -0

 		/*
+		 * Ensure that POSIX timer SIGEV_THREAD_ID signals pending for
+		 * the former leader are removed under sighand::siglock _before_
+		 * taking over the leader's TID. Otherwise the lockless cleanup
+		 * in release_task() can race against a concurrent signal
+		 * delivery to the new leader. The former leader has PF_EXITING
+		 * set which prevents queueing of SIGEV_THREAD_ID signals up to
+		 * the point where it's sighand gets cleared.
+		 */
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);

Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any changes, no?

Quoted 16 lines · Thomas Gleixner

kernel/signal.c void flush_sigqueue(struct sigpending *queue) · line 486

Quoted 11 lines · Hyunwoo Kim

kernel/signal.c +2 -1

index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
@@ -482,7 +482,11 @@ void flush_sigqueue(struct sigpending *queue)
 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+		/*
+		 * Pairs with the list_empty() in posixtimer_send_sigqueue().

No. That list_empty() would need to be changed to list_empty_careful() to be correct on weakly ordered architectures.

Quoted 3 lines

Aside of that I'm not convinced that this is the right way to handle this as it cures the symptom and not the underlying problem. Let me stare at this some more.

Something like the untested below.

Thanks,

tglx
---

fs/exec.c +12 -0

@@ -983,6 +983,18 @@ static int de_thread(struct task_struct
 		}
 
 		/*
+		 * Ensure that POSIX timer SIGEV_THREAD_ID signals pending for
+		 * the former leader are removed under sighand::siglock _before_
+		 * taking over the leader's TID. Otherwise the lockless cleanup
+		 * in release_task() can race against a concurrent signal
+		 * delivery to the new leader. The former leader has PF_EXITING
+		 * set which prevents queueing of SIGEV_THREAD_ID signals up to
+		 * the point where it's sighand gets cleared.
+		 */
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+
+		/*
 		 * The only record we have of the real-time age of a
 		 * process, regardless of execs it's done, is start_time.
 		 * All the past CPU time is accumulated in signal_struct

kernel/signal.c +7 -0

@@ -1998,6 +1998,13 @@ void posixtimer_send_sigqueue(struct k_i
 		return;
 
 	/*
+	 * If the signal is targeted at a specific thread, validate with sighand
+	 * lock held that the thread is not exiting.
+	 */
+	if (unlikely(tmr->it_pid_type == PIDTYPE_PID  && t->flags & PF_EXITING))
+		goto unlock;
+
+	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().
 	 */