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 <imv4bel@gmail.com>
Debugged-by: Hyunwoo Kim <imv4bel@gmail.com>
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Cc: stable@vger.kernel.org
Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
---
 kernel/exit.c   |   11 ++++++-----
 kernel/signal.c |   23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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);
 
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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.

Thomas Gleixner <tglx@kernel.org> writes:

> 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 <imv4bel@gmail.com>
> Debugged-by: Hyunwoo Kim <imv4bel@gmail.com>
> Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Cc: stable@vger.kernel.org
> Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel

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.


Eric

> ---
>  kernel/exit.c   |   11 ++++++-----
>  kernel/signal.c |   23 ++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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);
>  
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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.

On Mon, Aug 31 2026 at 10:26, Eric W. Biederman wrote:
>
> 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.

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

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

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

Thanks,

        tglx
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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

Thomas Gleixner <tglx@kernel.org> writes:

> On Mon, Aug 31 2026 at 10:26, Eric W. Biederman wrote:
>>
>> 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.
>
>> 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.
>
>> 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.
>
> Correct. Though that check would have to be limited to flushing
> tsk::pending not signal::shared_pending.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I was just about to suggest removing the entire list under the lock,
and then cleaning up the list entries outside of the lock, then I saw
this email :)

I am not wild about the name sigqueue_splice_pending (what is being
spliced together).

Perhaps call it sigqueue_dequeue_pending?  I think that conveys what
is happening a little better.

Eric



>
> Thanks,
>
>         tglx
> ---
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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

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 surfaced with the recent commit which moved the sigqueue flush
out of the sighand lock held region.

Hyonwoo proposed to fix this by using list_del_init_careful(), but that
just papers over the 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.

    Optimize that by moving the whole pending list to an on-stack list head
    under sighand lock and free the signals without the lock held.

Fixes: fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")
Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Debugged-by: Hyunwoo Kim <imv4bel@gmail.com>
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: stable@vger.kernel.org
Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
---
V2: Don't flush w/o sighand lock held - Oleg
    Move the while pending list under the lock and free it lockless
---
 kernel/exit.c   |   11 ++++++-----
 kernel/signal.c |   37 +++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 11 deletions(-)

--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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);
 
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -457,18 +457,28 @@ 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_dequeue_pending(struct sigpending *queue, struct list_head *head)
+{
+	sigemptyset(&queue->signal);
+	list_splice_init(&queue->list, head);
+}
+
 /*
  * Flush all pending signals for this kthread.
  */
@@ -1030,6 +1040,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 +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;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().
@@ -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);
 
@@ -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.

as for the change on exit_signal,

On 09/01, Thomas Gleixner wrote:
>
> @@ -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);
>
> @@ -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.
---

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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.

On Wed, Sep 02 2026 at 16:19, Oleg Nesterov wrote:
> 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.

void exit_signals(struct task_struct *tsk)
{
	LIST_HEAD(sigq_list);
	int group_stop = 0;

	/*
	 * @tsk is about to have PF_EXITING set - lock out users which
	 * expect a stable threadgroup.
	 */
	cgroup_threadgroup_change_begin(tsk);

	scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
		tsk->flags |= PF_EXITING;

		sigqueue_dequeue_pending(&tsk->pending, &sigq_list);

		if (task_sigpending(tsk) && !thread_group_empty(tsk) &&
		    !(tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
			sigset_t unblocked = tsk->blocked;

			signotset(&unblocked);
			retarget_shared_pending(tsk, &unblocked);

			if (unlikely(tsk->jobctl & JOBCTL_STOP_PENDING) &&
			    task_participate_group_stop(tsk))
				group_stop = CLD_STOPPED;
		}
	}

	cgroup_threadgroup_change_end(tsk);

	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.
	 */
	if (unlikely(group_stop)) {
		read_lock(&tasklist_lock);
		do_notify_parent_cldstop(tsk, false, group_stop);
		read_unlock(&tasklist_lock);
	}
}

On 09/03, Thomas Gleixner wrote:
>
> On Wed, Sep 02 2026 at 16:19, Oleg Nesterov wrote:
> > 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.

Agreed, the resulting code looks good to me.

Oleg.

Oleg Nesterov <oleg@redhat.com> writes:

> as for the change on exit_signal,
>
> On 09/01, Thomas Gleixner wrote:
>>
>> @@ -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);
>>
>> @@ -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.

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.

Eric


> Oleg.
> ---
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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.

On 09/02, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > 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.

Well, but the patch from Thomas adds

		scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
			tsk->flags |= PF_EXITING;
			sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
		}

into the fast-path, so either way exit_signals() can no longer skip
grabbing siglock.

Or I missed something again?

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

I am worried. But see above. We need to fix the bug first. Then perhaps
we can add some other optimizations.

Oleg.

On 09/01, Thomas Gleixner wrote:
>
> @@ -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;

OK... lets suppose the exiting task T passes exit_signals().

Suppose we have an "ignored" timer tmr. Another sub-thread calls
do_sigaction() -> posixtimer_sig_unignore() and finds that tmr
in ->ignored_posix_timers list.

But posixtimer_queue_sigqueue() doesn't check PF_EXITING, I guess
it should check it too?

Or perhaps it makes more sense to check PF_EXITING in
posixtimer_get_target() ?

Oleg.

As I said many times in this thread I am all confused ;)
And of course I don't understand posix-timers.c enough.

So let me ask the stupid question...

On 09/01, Thomas Gleixner wrote:
>
> 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?

Perhaps they make sense, but why do we need them to fix this particular
problem?

I am sure I missed something obvious, please help me to understand.

Ah, and I just noticed...

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

Oleg.

On Wed, Sep 02 2026 at 12:28, Oleg Nesterov wrote:
>> @@ -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().

Bah.

On 09/02, Oleg Nesterov wrote:
>
> On 09/01, Thomas Gleixner wrote:
> >
> > 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?

Aaah. I am stupid. the PF_EXITING check in posixtimer_send_sigqueue()
is obviously not enough, posixtimer_get_target() can return the execing
thread which is alive and doesn't have PF_EXITING set...

> Ah, and I just noticed...
>
> > @@ -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().

Oleg.

Le Mon, Aug 31, 2026 at 12:50:46PM +0200, Thomas Gleixner a écrit :
> 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 <imv4bel@gmail.com>
> Debugged-by: Hyunwoo Kim <imv4bel@gmail.com>
> Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Cc: stable@vger.kernel.org
> Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
> ---
>  kernel/exit.c   |   11 ++++++-----
>  kernel/signal.c |   23 ++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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);
>  
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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);
> +

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


-- 
Frederic Weisbecker
SUSE Labs

On Mon, Aug 31 2026 at 14:52, Frederic Weisbecker wrote:
> 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


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.

Le Tue, Sep 01, 2026 at 02:55:02PM +0200, Thomas Gleixner a écrit :
> On Mon, Aug 31 2026 at 14:52, Frederic Weisbecker wrote:
> > 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
> 
> 
> 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?

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

On Tue, Sep 01 2026 at 15:27, Frederic Weisbecker wrote:
> Le Tue, Sep 01, 2026 at 02:55:02PM +0200, Thomas Gleixner a écrit :
>> 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?

Flushing the signals.

On 08/31, Thomas Gleixner wrote:
>
> +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);

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

Oleg.

On Mon, Aug 31 2026 at 14:44, Oleg Nesterov wrote:
> On 08/31, Thomas Gleixner wrote:
>>
>> +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);
>
> Hmm... the exiting thread is still visible to for_each_thread().
> Can't this flush_pending_unlocked() race with (say) do_sigaction() ?

Bah. Yes.

On Mon, 31 Aug 2026 12:50:46 +0200
Thomas Gleixner <tglx@kernel.org> wrote:

> 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 <imv4bel@gmail.com>
> Debugged-by: Hyunwoo Kim <imv4bel@gmail.com>
> Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Cc: stable@vger.kernel.org
> Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
> ---
>  kernel/exit.c   |   11 ++++++-----
>  kernel/signal.c |   23 ++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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);
>  
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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;

Is that unlikely() going do the right thing?
Pretty much the only way to avoid a branch in the 'usual' path is to test
PF_EXITING first.
So you could do:
	if (unlikely(t->flags & PF_EXITING) && type == PIDTYPE_PID)
		goto ret;
(assuming t->flags is unlikely to be a cache miss).
Or, if you can persuade the compiler not to use a branch for the ?:
	if (unlikely(t->flags & (type == PIDTYPE_PID ? PF_EXITING : 0)))
		goto ret;

David

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