[patch V2 0/8] exec/exit: POSIX timer related bugfixes and related cleanups

[patch V2 0/8] exec/exit: POSIX timer related bugfixes and related cleanups

[Thomas Gleixner]

This is a follow up to V1 which can be found here:

  https://lore.kernel.org/20260904112100.683893401@kernel.org

Recent findings from Hyunwoo unearthed two bugs in handling POSIX timers on
exec().

The relevant patches, reports and discussions can be found here:

   https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
   https://patch.msgid.link/ao7Q8miiuLAPVnWv@v4bel

TLDR:

Both problems are related to non-leader exec(). POSIX CPU timers which are
targeted at tasks hold a pid reference of the target task, which is used to
look up the task in the related POSIX timer operations.

The non-leader exec() switches the TID of the old and the new leader, which
obviously invalidates these references for pid_task(PIDTYPE_PID) lookups.

This causes UAFs due to the resulting list corruptions or premature freeing
without removing the underlying POSIX CPU timers from the involved tasks.

The first issue which corrupts the signal pending list is solved by:

    - Preventing the queueing of per task signals on a task which has
      PF_EXITING set.

    - Protecting the unlocked setting of PF_EXITING in exit_signals() with
      sighand lock.

    - Flushing all per task signals right in exit_signals()

The second issue which keeps the POSIX CPU timers queued on the new leader
is solved by:

    - Moving the exec related POSIX timer cleanup right after de_thread()
      which ensures that the timers queued in new_leader::posix_cputimers
      are removed before the underlying POSIX timers are deleted.

After looking deeper at the exit() handling it turned out that the POSIX
timer cleanups can be done early in do_exit() instead of delaying them
until release_task().

The reason for this late cleanup is that POSIX CPU timers can be created,
rearmed and deleted as long as a task is visible, i.e. the pid is hashed
and sighand is not NULL. This allows to retrieve information from the timer
up to the point where the task is gone for real and that can't be changed
easily as that'd be a user visible change.

But once PF_EXITING is set on a task the task does not longer expire POSIX
CPU timers. So it makes no sense that the timers stay queued in
task::posix_cputimers after that point.

The only thing which needs to be prevented is that timers are requeued on
task::posix_cputimers once PF_EXITING is set or requeued on
signal::posix_cputimers when PF_EXITING is set and signal::live is zero,
which indicates that the thread group is dead.

With that solved the timers can be dequeued from task::posix_cputimer
pending when a task exits and from signal::posix_cputimer pending once the
threadgroup reaches the dead state, i.e. signal::live goes to zero in
do_exit().

The changes vs. V1:

    - Prevent requeuing POSIX timer signals in posixtimer_sig_unignore()
      when the target is exiting - Oleg, Frederic

    - Use signal::flags SIGNAL_GROUP_EXIT instead of signal::live to
      determine whether there is a group exit in progress - Eric

    - Refine existing and add new comments

The delta patch against V1 is below.

The series applies on 7.3-rc1 and is avalaible from git:

    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git posix-timers

Thanks,

	tglx

---
diff --git a/fs/exec.c b/fs/exec.c
index caed7c3566ca..67418118df5a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1156,9 +1156,9 @@ int begin_new_exec(struct linux_binprm * bprm)
 	/*
 	 * This must be done here to ensure that POSIX CPU timers which were
 	 * armed on the current task are dequeued from me::posix_cputimers.
-	 * That ensures that in case of a TID switch the deletion of the related
-	 * POSIX timer will not free an enqueued timer because the TID lookup
-	 * failed as the original target TID was the old leader.
+	 * Otherwise in case of a TID switch the deletion of the related POSIX
+	 * timer would not remove an enqueued timer because the TID lookup
+	 * of the old TID fails.
 	 */
 	posixtimer_exec();
 
diff --git a/kernel/signal.c b/kernel/signal.c
index a3c6a47460af..f93d8f77ec1a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1029,6 +1029,21 @@ static inline bool legacy_queue(struct sigpending *signals, int sig)
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+/*
+ * When PF_EXITING is set the task is on the way out and has t::pending
+ * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
+ * be leaked.
+ */
+static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
+{
+	lockdep_assert_held(&t->sighand->siglock);
+
+	if (!(t->flags & PF_EXITING))
+		return true;
+
+	return type != PIDTYPE_PID;
+}
+
 static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 				struct task_struct *t, enum pid_type type, bool force)
 {
@@ -1041,7 +1056,7 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 
 	result = TRACE_SIGNAL_IGNORED;
 
-	if (unlikely(type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+	if (!task_can_queue_signal(t, type))
 		goto ret;
 
 	if (!prepare_signal(sig, t, force))
@@ -1982,11 +1997,25 @@ static inline struct task_struct *posixtimer_get_target(struct k_itimer *tmr)
 	struct task_struct *t = pid_task(tmr->it_pid, tmr->it_pid_type);
 
 	if (t && tmr->it_pid_type != PIDTYPE_PID &&
-	    same_thread_group(t, current) && !current->exit_state)
+	    same_thread_group(t, current) && !(current->flags & PF_EXITING))
 		t = current;
 	return t;
 }
 
+/*
+ * Find the target task for the POSIX timer signal and prevent that a
+ * PIDTYPE_PID signal is queued on a task which has PF_EXITING set.
+ */
+static inline struct task_struct *posixtimer_get_unignore_target(struct k_itimer *tmr)
+{
+	struct task_struct *t = posixtimer_get_target(tmr);
+
+	if (t && task_can_queue_signal(t, tmr->it_pid_type))
+		return t;
+
+	return NULL;
+}
+
 void posixtimer_send_sigqueue(struct k_itimer *tmr)
 {
 	struct sigqueue *q = &tmr->sigq;
@@ -2004,7 +2033,7 @@ void posixtimer_send_sigqueue(struct k_itimer *tmr)
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
-	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
+	if (!task_can_queue_signal(t, tmr->it_pid_type))
 		goto unlock;
 
 	/*
@@ -2154,7 +2183,7 @@ static void posixtimer_sig_unignore(struct task_struct *tsk, int sig)
 		 * has exited by now, drop the reference count.
 		 */
 		guard(rcu)();
-		target = posixtimer_get_target(tmr);
+		target = posixtimer_get_unignore_target(tmr);
 		if (target)
 			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);
 		else
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 29ee485d4d0b..53e47c1b56c2 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -686,13 +686,25 @@ void posix_cpu_timers_exit_group(void)
 	cleanup_timers(&current->signal->posix_cputimers);
 }
 
-static inline bool task_can_enqueue(struct k_itimer *timer, struct task_struct *p)
+/*
+ * This function validates that POSIX CPU timers can be safely enqueued on the
+ * target task.
+ *
+ * Enqueue is allowed when PF_EXITING is not set. If set then it is only allowed
+ * for process shared timers (type = PIDTYPE_TGID) as long as tsk::signal::flags
+ * does not have SIGNAL_GROUP_EXIT set. PIDTYPE_PID targets are not allowed at
+ * all when the task has PF_EXITING set.
+ *
+ * This guarantees that after the POSIX timer cleanup in posixtimer_exit() no
+ * POSIX CPU timers are queued on the task or in case of a group exit on the
+ * process.
+ */
+static inline bool task_can_enqueue_timer(struct task_struct *tsk, enum pid_type type)
 {
-	if (likely(!(p->flags & PF_EXITING)))
+	if (likely(!(tsk->flags & PF_EXITING)))
 		return true;
 
-	/* Allow TGID type unless the last thread is on the way out. */
-	return clock_pid_type(timer->it_clock) == PIDTYPE_TGID && atomic_read(&p->signal->live);
+	return type == PIDTYPE_TGID && !(tsk->signal->flags & SIGNAL_GROUP_EXIT);
 }
 
 /*
@@ -709,18 +721,7 @@ static void arm_timer(struct k_itimer *timer, struct task_struct *p)
 
 	timer->it_status = POSIX_TIMER_ARMED;
 
-	/*
-	 * Don't enqueue timers when the task or the group is exiting. That
-	 * ensures that timer operations are still succeeding as long as the
-	 * tasks are visible, but won't enqueue the timers on the task or
-	 * process. They won't expire anyway because run_posix_cpu_timers()
-	 * exits early when PF_EXITING is set.
-	 *
-	 * Enqueue is skipped if PF_EXITING is set when the timer is per task
-	 * and when the last thread decremented p::signal::live to zero also for
-	 * per process timers.
-	 */
-	if (unlikely(!task_can_enqueue(timer, p)))
+	if (unlikely(!task_can_enqueue_timer(p, clock_pid_type(timer->it_clock))))
 		return;
 
 	if (!cpu_timer_enqueue(&base->tqhead, ctmr))

[patch V2 1/8] 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>
Cc: stable@vger.kernel.org
Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
---
V4: Prevent requeuing of signals which are unignored - Oleg, Eric

    Split out the decision into an inline which can be reused by the posix
    CPU timer follow up changes.

V3: Restructure code and fix the missing unlock - Oleg

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 |  103 +++++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 78 insertions(+), 36 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.
  */
@@ -1019,6 +1029,21 @@ static inline bool legacy_queue(struct s
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
+/*
+ * When PF_EXITING is set the task is on the way out and has t::pending
+ * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
+ * be leaked.
+ */
+static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
+{
+	lockdep_assert_held(&t->sighand->siglock);
+
+	if (!(t->flags & PF_EXITING))
+		return true;
+
+	return type != PIDTYPE_PID;
+}
+
 static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 				struct task_struct *t, enum pid_type type, bool force)
 {
@@ -1030,6 +1055,10 @@ static int __send_signal_locked(int sig,
 	lockdep_assert_held(&t->sighand->siglock);
 
 	result = TRACE_SIGNAL_IGNORED;
+
+	if (!task_can_queue_signal(t, type))
+		goto ret;
+
 	if (!prepare_signal(sig, t, force))
 		goto ret;
 
@@ -1968,11 +1997,25 @@ static inline struct task_struct *posixt
 	struct task_struct *t = pid_task(tmr->it_pid, tmr->it_pid_type);
 
 	if (t && tmr->it_pid_type != PIDTYPE_PID &&
-	    same_thread_group(t, current) && !current->exit_state)
+	    same_thread_group(t, current) && !(current->flags & PF_EXITING))
 		t = current;
 	return t;
 }
 
+/*
+ * Find the target task for the POSIX timer signal and prevent that a
+ * PIDTYPE_PID signal is queued on a task which has PF_EXITING set.
+ */
+static inline struct task_struct *posixtimer_get_unignore_target(struct k_itimer *tmr)
+{
+	struct task_struct *t = posixtimer_get_target(tmr);
+
+	if (t && task_can_queue_signal(t, tmr->it_pid_type))
+		return t;
+
+	return NULL;
+}
+
 void posixtimer_send_sigqueue(struct k_itimer *tmr)
 {
 	struct sigqueue *q = &tmr->sigq;
@@ -1990,6 +2033,9 @@ void posixtimer_send_sigqueue(struct k_i
 	if (!likely(lock_task_sighand(t, &flags)))
 		return;
 
+	if (!task_can_queue_signal(t, tmr->it_pid_type))
+		goto unlock;
+
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
 	 * locked to prevent a race against dequeue_signal().
@@ -2081,6 +2127,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);
 }
 
@@ -2136,7 +2183,7 @@ static void posixtimer_sig_unignore(stru
 		 * has exited by now, drop the reference count.
 		 */
 		guard(rcu)();
-		target = posixtimer_get_target(tmr);
+		target = posixtimer_get_unignore_target(tmr);
 		if (target)
 			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);
 		else
@@ -3120,42 +3167,36 @@ static void retarget_shared_pending(stru
 
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
-	sigset_t unblocked;
 
 	/*
 	 * @tsk is about to have PF_EXITING set - lock out users which
-	 * expect stable threadgroup.
+	 * expect a stable threadgroup.
 	 */
 	cgroup_threadgroup_change_begin(tsk);
 
-	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
+	scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
 		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 (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;
+		}
+	}
 
-	if (!task_sigpending(tsk))
-		goto out;
+	cgroup_threadgroup_change_end(tsk);
 
-	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;
-out:
-	spin_unlock_irq(&tsk->sighand->siglock);
+	flush_sigqueue_list(&sigq_list);
 
 	/*
 	 * If group stop has completed, deliver the notification.  This

[patch V2 2/8] exec: Cleanup POSIX timers right after de_thread()

[Thomas Gleixner]

From: Hyunwoo Kim <imv4bel@gmail.com>

A per-thread CPU timer holds a reference to the PID of the thread it is
attached to and, while it is armed, its node is queued in that thread's
posix_cputimers. The task is looked up by that PID.

When a non-leader thread exec()s, de_thread() changes which task owns
that PID. pid_task(timer->it.cpu.pid, PIDTYPE_PID) then returns NULL,
but the node is still queued on tsk, which is alive. timer_lock_sighand()
takes a failed lookup to mean that the node is already dequeued, so it
has nothing to undo.

begin_new_exec() calls posix_cpu_timers_exit(me) right after
exec_task_namespaces() and that removes the leftover node, so the state
normally stays invisible. But bprm->point_of_no_return is set before
de_thread(), so if unshare_files(), set_mm_exe_file(), exec_mmap() or
exec_task_namespaces() fails, the task dies before it gets there.
exit_itimers() then frees the k_itimer while its node is still queued,
and reaping tsk later erases that freed node from the rbtree.

In short:

      the non-leader thread B           the parent

  timer_create(CLOCK_THREAD_CPUTIME_ID)
  timer_settime()
    arm_timer()            // the node is queued on B
  execve()
    de_thread(B)
      exchange_tids(B, leader)  // B's PID now belongs to the leader
      release_task(leader)
        __exit_signal(leader)
          posix_cpu_timers_exit(leader)  // cleans leader's queue, not B's
          __unhash_process(leader)  // that PID has no task anymore
    exec_mmap()
      mmap_read_lock_killable(old_mm)
                                kill(B, SIGKILL)
      // -EINTR
  get_signal()
    do_exit()
      exit_itimers()
        posix_timer_delete()
          posix_cpu_timer_del()
        posix_timer_unhash_and_free()  // freed while still queued
                                wait4()
                                  release_task(B)
                                    posix_cpu_timers_exit(B)
                                      cleanup_timerqueue()
                                        timerqueue_del()  // use-after-free

Move the POSIX timer cleanup right after de_thread() before any of the
later failure conditions brings the task into do_exit().

[ tglx: Move the cleanup right after de_thread() ]

Fixes: 55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a task")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Cc: stable@vger.kernel.org
Link: https://patch.msgid.link/ao7Q8miiuLAPVnWv@v4bel
---
Changes in v3:
  Move the cleanup right after de_thread() - Oleg
  Rework change log

Changes in v2:
- Add the trigger sequence and the KASAN log to the commit message.
- v1: https://lore.kernel.org/all/anfgrsPlUdwBhdrp@v4bel/
---
 fs/exec.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1115,6 +1115,17 @@ static struct file *bprm_identity_file(c
 	return bprm->file;
 }
 
+static void posixtimer_exec(struct task_struct *me)
+{
+#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
+}
+
 /*
  * Calling this is the point of no return. None of the failures will be
  * seen by userspace since either the process is already taking a fatal
@@ -1152,6 +1163,16 @@ int begin_new_exec(struct linux_binprm *
 	retval = de_thread(me);
 	if (retval)
 		goto out;
+
+	/*
+	 * This must be done here to ensure that POSIX CPU timers which were
+	 * armed on the current task are dequeued from me::posix_cputimers.
+	 * Otherwise in case of a TID switch the deletion of the related POSIX
+	 * timer would not remove an enqueued timer because the TID lookup
+	 * of the old TID fails.
+	 */
+	posixtimer_exec(me);
+
 	/* see the comment in check_unsafe_exec() */
 	current->fs->in_exec = 0;
 	/*
@@ -1192,14 +1213,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.
 	 */

[patch V2 3/8] posix-timers: Move posixtimer_exec_cleanup() out of exec.c

[Thomas Gleixner]

Move it to the POSIX timer code and provide a proper stub when POSIX timers
are disabled in Kconfig.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 fs/exec.c                    |   13 +------------
 include/linux/posix-timers.h |    3 +++
 kernel/time/posix-timers.c   |    9 +++++++++
 3 files changed, 13 insertions(+), 12 deletions(-)

--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1115,17 +1115,6 @@ static struct file *bprm_identity_file(c
 	return bprm->file;
 }
 
-static void posixtimer_exec(struct task_struct *me)
-{
-#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
-}
-
 /*
  * Calling this is the point of no return. None of the failures will be
  * seen by userspace since either the process is already taking a fatal
@@ -1171,7 +1160,7 @@ int begin_new_exec(struct linux_binprm *
 	 * timer would not remove an enqueued timer because the TID lookup
 	 * of the old TID fails.
 	 */
-	posixtimer_exec(me);
+	posixtimer_exec();
 
 	/* see the comment in check_unsafe_exec() */
 	current->fs->in_exec = 0;
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -232,6 +232,8 @@ void set_process_cpu_timer(struct task_s
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
 #ifdef CONFIG_POSIX_TIMERS
+void posixtimer_exec(void);
+
 static inline void posixtimer_putref(struct k_itimer *tmr)
 {
 	if (rcuref_put(&tmr->rcuref))
@@ -259,6 +261,7 @@ static inline bool posixtimer_valid(cons
 	return !(val & 0x1UL);
 }
 #else  /* CONFIG_POSIX_TIMERS */
+static inline void posixtimer_exec(void) { }
 static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { }
 static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { }
 #endif /* !CONFIG_POSIX_TIMERS */
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1120,6 +1120,15 @@ void exit_itimers(struct task_struct *ts
 	}
 }
 
+void posixtimer_exec(void)
+{
+	scoped_guard(spinlock_irq, &current->sighand->siglock)
+		posix_cpu_timers_exit(current);
+
+	exit_itimers(current);
+	flush_itimer_signals();
+}
+
 SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
 		const struct __kernel_timespec __user *, tp)
 {

[patch V2 4/8] posix-timers: Move POSIX timer group exit related code out of do_exit()

[Thomas Gleixner]

Move the POSIX timer group exit handling into the posix timer code and
provide a proper stub when POSIX timers are disabled in Kconfig.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 include/linux/posix-timers.h |    2 ++
 include/linux/sched/task.h   |    1 -
 kernel/exit.c                |    7 +++----
 kernel/time/posix-timers.c   |   16 +++++++++-------
 4 files changed, 14 insertions(+), 12 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -233,6 +233,7 @@ int update_rlimit_cpu(struct task_struct
 
 #ifdef CONFIG_POSIX_TIMERS
 void posixtimer_exec(void);
+void posixtimer_exit(void);
 
 static inline void posixtimer_putref(struct k_itimer *tmr)
 {
@@ -262,6 +263,7 @@ static inline bool posixtimer_valid(cons
 }
 #else  /* CONFIG_POSIX_TIMERS */
 static inline void posixtimer_exec(void) { }
+static inline void posixtimer_exit(void) { }
 static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { }
 static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { }
 #endif /* !CONFIG_POSIX_TIMERS */
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -94,7 +94,6 @@ static inline void exit_thread(struct ta
 extern __noreturn void do_group_exit(int);
 
 extern void exit_files(struct task_struct *);
-extern void exit_itimers(struct task_struct *);
 
 extern pid_t kernel_clone(struct kernel_clone_args *kargs);
 struct task_struct *copy_process(struct pid *pid, int trace, int node,
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -963,13 +963,12 @@ void __noreturn do_exit(long code)
 			panic("Attempted to kill init! exitcode=0x%08x\n",
 				tsk->signal->group_exit_code ?: (int)code);
 
-#ifdef CONFIG_POSIX_TIMERS
-		hrtimer_cancel(&tsk->signal->real_timer);
-		exit_itimers(tsk);
-#endif
+		posixtimer_exit();
+
 		if (tsk->mm)
 			setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
 	}
+
 	acct_collect(code, group_dead);
 	if (group_dead)
 		tty_audit_exit();
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1077,13 +1077,9 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
 	return 0;
 }
 
-/*
- * Invoked from do_exit() when the last thread of a thread group exits.
- * At that point no other task can access the timers of the dying
- * task anymore.
- */
-void exit_itimers(struct task_struct *tsk)
+static void posixtimer_delete_timers(void)
 {
+	struct task_struct *tsk = current;
 	struct hlist_head timers;
 	struct hlist_node *next;
 	struct k_itimer *timer;
@@ -1120,12 +1116,18 @@ void exit_itimers(struct task_struct *ts
 	}
 }
 
+void posixtimer_exit(void)
+{
+	hrtimer_cancel(&current->signal->real_timer);
+	posixtimer_delete_timers();
+}
+
 void posixtimer_exec(void)
 {
 	scoped_guard(spinlock_irq, &current->sighand->siglock)
 		posix_cpu_timers_exit(current);
 
-	exit_itimers(current);
+	posixtimer_delete_timers();
 	flush_itimer_signals();
 }
 

[patch V2 5/8] posix-cpu-timers: Move inlines out of public header

[Thomas Gleixner]

They are only used in the POSIX CPU timer code. No point in exposing them
globally and parsing them for nothing.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 include/linux/posix-timers.h   |   32 --------------------------------
 kernel/time/posix-cpu-timers.c |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 32 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -66,38 +66,6 @@ struct cpu_timer {
 	struct task_struct __rcu	*handling;
 };
 
-static inline bool cpu_timer_enqueue(struct timerqueue_head *head,
-				     struct cpu_timer *ctmr)
-{
-	ctmr->head = head;
-	return timerqueue_add(head, &ctmr->node);
-}
-
-static inline bool cpu_timer_queued(struct cpu_timer *ctmr)
-{
-	return !!ctmr->head;
-}
-
-static inline bool cpu_timer_dequeue(struct cpu_timer *ctmr)
-{
-	if (cpu_timer_queued(ctmr)) {
-		timerqueue_del(ctmr->head, &ctmr->node);
-		ctmr->head = NULL;
-		return true;
-	}
-	return false;
-}
-
-static inline u64 cpu_timer_getexpires(struct cpu_timer *ctmr)
-{
-	return ctmr->node.expires;
-}
-
-static inline void cpu_timer_setexpires(struct cpu_timer *ctmr, u64 exp)
-{
-	ctmr->node.expires = exp;
-}
-
 static inline void posix_cputimers_init(struct posix_cputimers *pct)
 {
 	memset(pct, 0, sizeof(*pct));
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -438,6 +438,38 @@ static void trigger_base_recalc_expires(
 	base->nextevt = 0;
 }
 
+static inline bool cpu_timer_enqueue(struct timerqueue_head *head,
+				     struct cpu_timer *ctmr)
+{
+	ctmr->head = head;
+	return timerqueue_add(head, &ctmr->node);
+}
+
+static inline bool cpu_timer_queued(struct cpu_timer *ctmr)
+{
+	return !!ctmr->head;
+}
+
+static inline bool cpu_timer_dequeue(struct cpu_timer *ctmr)
+{
+	if (cpu_timer_queued(ctmr)) {
+		timerqueue_del(ctmr->head, &ctmr->node);
+		ctmr->head = NULL;
+		return true;
+	}
+	return false;
+}
+
+static inline u64 cpu_timer_getexpires(struct cpu_timer *ctmr)
+{
+	return ctmr->node.expires;
+}
+
+static inline void cpu_timer_setexpires(struct cpu_timer *ctmr, u64 exp)
+{
+	ctmr->node.expires = exp;
+}
+
 /*
  * Dequeue the timer and reset the base if it was its earliest expiration.
  * It makes sure the next tick recalculates the base next expiration so we

[patch V2 6/8] posix-cpu-timers: Use PF_EXITING to indicate exit

[Thomas Gleixner]

Right now POSIX CPU timers use task::exit_state to check whether a task is
exiting. That works correctly, but exit_state is set later in do_exit() and
too late for allowing to cleanup POSIX timers earlier.

It does not matter in case of exit whether the cutoff is a bit earlier.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 kernel/time/posix-cpu-timers.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1505,7 +1505,7 @@ void run_posix_cpu_timers(void)
 	 * posix_cpu_timer_del() may fail to lock_task_sighand(tsk) and
 	 * miss timer->it.cpu.firing != 0.
 	 */
-	if (tsk->exit_state)
+	if (tsk->flags & PF_EXITING)
 		return;
 
 	/*

[patch V2 7/8] posix-cpu-timers: Prevent enqueueing when PF_EXITING is set

[Thomas Gleixner]

To prepare for cleaning up POSIX CPU timers in do_exit(), prevent
enqueueing POSIX CPU timers on a task which has PF_EXITING set.  Queueing a
timer on such a task is pointless because the task won't expire the timer
anymore.

The same applies to process wide timers when tsk::signal::flags has
SIGNAL_GROUP_EXIT set.

Pretending that the timer is armed allows to keep the POSIX timer mechanism
"working" so that the timer stays accessible up to the point where a task
is unhashed.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 kernel/time/posix-cpu-timers.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -628,6 +628,7 @@ static int posix_cpu_timer_del(struct k_
 	}
 
 	if (!ret) {
+		WARN_ON_ONCE(cpu_timer_queued(&timer->it.cpu));
 		put_pid(timer->it.cpu.pid);
 		timer->it_status = POSIX_TIMER_DISARMED;
 	}
@@ -675,6 +676,27 @@ void posix_cpu_timers_exit_group(struct
 }
 
 /*
+ * This function validates that POSIX CPU timers can be safely enqueued on the
+ * target task.
+ *
+ * Enqueue is allowed when PF_EXITING is not set. If set then it is only allowed
+ * for process shared timers (type = PIDTYPE_TGID) as long as tsk::signal::flags
+ * does not have SIGNAL_GROUP_EXIT set. PIDTYPE_PID targets are not allowed at
+ * all when the task has PF_EXITING set.
+ *
+ * This guarantees that after the POSIX timer cleanup in posixtimer_exit() no
+ * POSIX CPU timers are queued on the task or in case of a group exit on the
+ * process.
+ */
+static inline bool task_can_enqueue_timer(struct task_struct *tsk, enum pid_type type)
+{
+	if (likely(!(tsk->flags & PF_EXITING)))
+		return true;
+
+	return type == PIDTYPE_TGID && !(tsk->signal->flags & SIGNAL_GROUP_EXIT);
+}
+
+/*
  * Insert the timer on the appropriate list before any timers that
  * expire later.  This must be called with the sighand lock held.
  */
@@ -684,7 +706,13 @@ static void arm_timer(struct k_itimer *t
 	struct cpu_timer *ctmr = &timer->it.cpu;
 	u64 newexp = cpu_timer_getexpires(ctmr);
 
+	lockdep_assert_held(&p->sighand->siglock);
+
 	timer->it_status = POSIX_TIMER_ARMED;
+
+	if (unlikely(!task_can_enqueue_timer(p, clock_pid_type(timer->it_clock))))
+		return;
+
 	if (!cpu_timer_enqueue(&base->tqhead, ctmr))
 		return;
 

[patch V2 8/8] posix-timers: Handle exit in do_exit() completely

[Thomas Gleixner]

Now that POSIX CPU timers cannot be enqueued on a task after PF_EXITING is
set and process wide timers cannot be enqueued when PF_EXITING is set and
the last thread in the group is exiting, it is possible to mop up POSIX
timers in do_exit() completely.

This requires to cancel an eventually pending POSIX CPU timer task work
right there because do_exit() invokes exit_task_work() later, which would
be acting on torn down data.

Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 include/linux/posix-timers.h   |    6 ++----
 kernel/exit.c                  |   10 ++--------
 kernel/time/posix-cpu-timers.c |   38 +++++++++++++++++++++++++++++++-------
 kernel/time/posix-timers.c     |   15 +++++++++------
 kernel/time/posix-timers.h     |    3 +++
 5 files changed, 47 insertions(+), 25 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -192,8 +192,6 @@ struct k_itimer {
 } ____cacheline_aligned_in_smp;
 
 void run_posix_cpu_timers(void);
-void posix_cpu_timers_exit(struct task_struct *task);
-void posix_cpu_timers_exit_group(struct task_struct *task);
 void set_process_cpu_timer(struct task_struct *task, unsigned int clock_idx,
 			   u64 *newval, u64 *oldval);
 
@@ -201,7 +199,7 @@ int update_rlimit_cpu(struct task_struct
 
 #ifdef CONFIG_POSIX_TIMERS
 void posixtimer_exec(void);
-void posixtimer_exit(void);
+void posixtimer_exit(bool group_dead);
 
 static inline void posixtimer_putref(struct k_itimer *tmr)
 {
@@ -231,7 +229,7 @@ static inline bool posixtimer_valid(cons
 }
 #else  /* CONFIG_POSIX_TIMERS */
 static inline void posixtimer_exec(void) { }
-static inline void posixtimer_exit(void) { }
+static inline void posixtimer_exit(bool group_dead) { }
 static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { }
 static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { }
 #endif /* !CONFIG_POSIX_TIMERS */
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -167,12 +167,6 @@ static void __exit_signal(struct release
 					lockdep_tasklist_lock_is_held());
 	spin_lock(&sighand->siglock);
 
-#ifdef CONFIG_POSIX_TIMERS
-	posix_cpu_timers_exit(tsk);
-	if (group_dead)
-		posix_cpu_timers_exit_group(tsk);
-#endif
-
 	if (group_dead) {
 		tty = sig->tty;
 		sig->tty = NULL;
@@ -963,12 +957,12 @@ void __noreturn do_exit(long code)
 			panic("Attempted to kill init! exitcode=0x%08x\n",
 				tsk->signal->group_exit_code ?: (int)code);
 
-		posixtimer_exit();
-
 		if (tsk->mm)
 			setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
 	}
 
+	posixtimer_exit(group_dead);
+
 	acct_collect(code, group_dead);
 	if (group_dead)
 		tty_audit_exit();
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -661,18 +661,29 @@ static void cleanup_timers(struct posix_
 	cleanup_timerqueue(&pct->bases[CPUCLOCK_SCHED].tqhead);
 }
 
+static inline void posix_cpu_timers_exit_work(void);
+
 /*
- * These are both called with the siglock held, when the current thread
- * is being reaped.  When the final (leader) thread in the group is reaped,
- * posix_cpu_timers_exit_group will be called after posix_cpu_timers_exit.
+ * Invoked from posixtimer_exit_task() after PF_EXITING was set in tsk::flags or
+ * from posixtimer_exec_cleanup().
  */
-void posix_cpu_timers_exit(struct task_struct *tsk)
+void posix_cpu_timers_exit_task(void)
 {
-	cleanup_timers(&tsk->posix_cputimers);
+	posix_cpu_timers_exit_work();
+
+	guard(spinlock_irq)(&current->sighand->siglock);
+	cleanup_timers(&current->posix_cputimers);
 }
-void posix_cpu_timers_exit_group(struct task_struct *tsk)
+
+/*
+ * Invoked from posixtimer_exit_group() after PF_EXITING was set in tsk::flags.
+ */
+void posix_cpu_timers_exit_group(void)
 {
-	cleanup_timers(&tsk->signal->posix_cputimers);
+	posix_cpu_timers_exit_task();
+
+	guard(spinlock_irq)(&current->sighand->siglock);
+	cleanup_timers(&current->signal->posix_cputimers);
 }
 
 /*
@@ -1257,6 +1268,17 @@ static void posix_cpu_timers_work(struct
 	mutex_unlock(&cw->mutex);
 }
 
+static inline void posix_cpu_timers_exit_work(void)
+{
+	/*
+	 * current->flags has PF_EXITING set so this can be done lockless and
+	 * with interrupts enabled as PF_EXITING prevents the interrupt from
+	 * scheduling the work.
+	 */
+	if (current->posix_cputimers_work.scheduled)
+		task_work_cancel(current, &current->posix_cputimers_work.work);
+}
+
 /*
  * Invoked from the posix-timer core when a cancel operation failed because
  * the timer is marked firing. The caller holds rcu_read_lock(), which
@@ -1387,6 +1409,8 @@ static inline void __run_posix_cpu_timer
 	lockdep_posixtimer_exit();
 }
 
+static inline void posix_cpu_timers_exit_work(void) { }
+
 static void posix_cpu_timer_wait_running(struct k_itimer *timr)
 {
 	cpu_relax();
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1116,17 +1116,20 @@ static void posixtimer_delete_timers(voi
 	}
 }
 
-void posixtimer_exit(void)
+void posixtimer_exit(bool group_dead)
 {
-	hrtimer_cancel(&current->signal->real_timer);
-	posixtimer_delete_timers();
+	if (group_dead) {
+		hrtimer_cancel(&current->signal->real_timer);
+		posix_cpu_timers_exit_group();
+		posixtimer_delete_timers();
+	} else {
+		posix_cpu_timers_exit_task();
+	}
 }
 
 void posixtimer_exec(void)
 {
-	scoped_guard(spinlock_irq, &current->sighand->siglock)
-		posix_cpu_timers_exit(current);
-
+	posix_cpu_timers_exit_task();
 	posixtimer_delete_timers();
 	flush_itimer_signals();
 }
--- a/kernel/time/posix-timers.h
+++ b/kernel/time/posix-timers.h
@@ -51,3 +51,6 @@ int common_timer_set(struct k_itimer *ti
 		     struct itimerspec64 *old_setting);
 void posix_timer_set_common(struct k_itimer *timer, struct itimerspec64 *new_setting);
 int common_timer_del(struct k_itimer *timer);
+
+void posix_cpu_timers_exit_task(void);
+void posix_cpu_timers_exit_group(void);

Re: [patch V2 1/8] signal: Prevent exec() race

[Oleg Nesterov]

On 09/05, Thomas Gleixner wrote:
>
> +/*
> + * Find the target task for the POSIX timer signal and prevent that a
> + * PIDTYPE_PID signal is queued on a task which has PF_EXITING set.
> + */
> +static inline struct task_struct *posixtimer_get_unignore_target(struct k_itimer *tmr)
> +{
> +	struct task_struct *t = posixtimer_get_target(tmr);
> +
> +	if (t && task_can_queue_signal(t, tmr->it_pid_type))
> +		return t;
> +
> +	return NULL;
> +}
...
> @@ -2136,7 +2183,7 @@ static void posixtimer_sig_unignore(stru
>  		 * has exited by now, drop the reference count.
>  		 */
>  		guard(rcu)();
> -		target = posixtimer_get_target(tmr);
> +		target = posixtimer_get_unignore_target(tmr);
>  		if (target)
>  			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);

Not sure posixtimer_get_unignore_target() makes a lot of sence...
It has a single caller, and posixtimer_sig_unignore() can do

		target = posixtimer_get_target(tmr);
		if (target && task_can_queue_signal(target, tmr->it_pid_type))
			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);

But this is cosmetic and subjective. I believe the patch is correct.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [patch V2 2/8] exec: Cleanup POSIX timers right after de_thread()

[Oleg Nesterov]

On 09/05, Thomas Gleixner wrote:
>
> Move the POSIX timer cleanup right after de_thread() before any of the
> later failure conditions brings the task into do_exit().

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [patch V2 7/8] posix-cpu-timers: Prevent enqueueing when PF_EXITING is set

[Oleg Nesterov]

I do not want to spam lkml, so let me say that all the previous changes
look good to me, feel free to add

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


But I am confused by this patch, even if it looks correct to me too.

On 09/05, Thomas Gleixner wrote:
>
> @@ -684,7 +706,13 @@ static void arm_timer(struct k_itimer *t
>  	struct cpu_timer *ctmr = &timer->it.cpu;
>  	u64 newexp = cpu_timer_getexpires(ctmr);
>
> +	lockdep_assert_held(&p->sighand->siglock);
> +
>  	timer->it_status = POSIX_TIMER_ARMED;
> +
> +	if (unlikely(!task_can_enqueue_timer(p, clock_pid_type(timer->it_clock))))
> +		return;

I can't understand why does it check task_can_enqueue_timer() after
setting POSIX_TIMER_ARMED. This adds the new armed-but-not-enqueued state,
afaics.

I see nothing wrong, it seems that this can only affect __posix_cpu_timer_get()
which checks ->it_status, other code paths do not check ->it_status.

But I don't understand this code, so let me ask: is it on purpose? I mean,
is there any reason to set _ARMED unconditionally ?

Oleg.

Re: [patch V2 8/8] posix-timers: Handle exit in do_exit() completely

[Oleg Nesterov]

I am still trying to understand this patch, one question for now.

On 09/05, Thomas Gleixner wrote:
>
> +void posix_cpu_timers_exit_task(void)
>  {
> -	cleanup_timers(&tsk->posix_cputimers);
> +	posix_cpu_timers_exit_work();
> +
> +	guard(spinlock_irq)(&current->sighand->siglock);
> +	cleanup_timers(&current->posix_cputimers);
>  }

So it calls posix_cpu_timers_exit_work()

> +static inline void posix_cpu_timers_exit_work(void)
> +{
> +	/*
> +	 * current->flags has PF_EXITING set so this can be done lockless and
> +	 * with interrupts enabled as PF_EXITING prevents the interrupt from
> +	 * scheduling the work.
> +	 */
> +	if (current->posix_cputimers_work.scheduled)
> +		task_work_cancel(current, &current->posix_cputimers_work.work);

... which does not clear ->scheduled

>  void posixtimer_exec(void)
>  {
> -	scoped_guard(spinlock_irq, &current->sighand->siglock)
> -		posix_cpu_timers_exit(current);
> -
> +	posix_cpu_timers_exit_task();

... and this looks obviously wrong for posixtimer_exec() ?

Oleg.

Re: [patch V2 1/8] signal: Prevent exec() race

[Eric W. Biederman]

Thomas Gleixner <tglx@kernel.org> writes:

> @@ -1019,6 +1029,21 @@ static inline bool legacy_queue(struct s
>  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
>  }
>  
> +/*
> + * When PF_EXITING is set the task is on the way out and has t::pending
> + * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
> + * be leaked.
> + */
> +static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
> +{
> +	lockdep_assert_held(&t->sighand->siglock);
> +
> +	if (!(t->flags & PF_EXITING))
> +		return true;
> +

I don't know if we care but I just noticed that this disallows
using tkill(..., SIGKILL) or tgkill(..., SIGKILL) to stop coredumps.

> +	return type != PIDTYPE_PID;
> +}
> +
>  static int __send_signal_locked(int sig, struct kernel_siginfo *info,
>  				struct task_struct *t, enum pid_type type, bool force)
>  {

Eric

Re: [patch V2 1/8] signal: Prevent exec() race

[Oleg Nesterov]

On 09/06, Eric W. Biederman wrote:
>
> Thomas Gleixner <tglx@kernel.org> writes:
>
> > @@ -1019,6 +1029,21 @@ static inline bool legacy_queue(struct s
> >  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
> >  }
> >
> > +/*
> > + * When PF_EXITING is set the task is on the way out and has t::pending
> > + * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
> > + * be leaked.
> > + */
> > +static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
> > +{
> > +	lockdep_assert_held(&t->sighand->siglock);
> > +
> > +	if (!(t->flags & PF_EXITING))
> > +		return true;
> > +
>
> I don't know if we care but I just noticed that this disallows
> using tkill(..., SIGKILL) or tgkill(..., SIGKILL) to stop coredumps.

I think we do care, but the coredumping thread is not PF_EXITING ?

Oleg.

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Sun, Sep 06 2026 at 17:39, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@kernel.org> writes:
>> @@ -1019,6 +1029,21 @@ static inline bool legacy_queue(struct s
>>  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
>>  }
>>  
>> +/*
>> + * When PF_EXITING is set the task is on the way out and has t::pending
>> + * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
>> + * be leaked.
>> + */
>> +static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
>> +{
>> +	lockdep_assert_held(&t->sighand->siglock);
>> +
>> +	if (!(t->flags & PF_EXITING))
>> +		return true;
>> +
>
> I don't know if we care but I just noticed that this disallows
> using tkill(..., SIGKILL) or tgkill(..., SIGKILL) to stop coredumps.

The thread running the coredump does not have PF_EXITING set:

get_signal()
        ....
        vfs_coredump()
        ...
        do_group_exit()

The only interaction with coredumps of a task which reached do_exit() is
via:

        synchronize_group_exit()
           coredump_task_exit()
        ...
        exit_signals() ; // sets PF_EXITING.

coredump_task_exit() waits until the dumper thread finished, so even if
tkill() is directed at a non-dumper thread which is stuck there in
coredump_task_exit() the signal will be queued and complete_signal()
will set signal->flags = SIGNAL_GROUP_EXIT and wake everyone up
including the dumper thread.

So the only case where this matters is when a task sets PF_EXITING
_before_ the dumper starts:

T1			T2
                        do_exit()
vfs_coredump()
                          synchronize_group_exit()
                            lock(sighand)
                            tsk->flags |= PF_POSTCOREDUMP;
                            core_state = signal->core_state;
                            unlock(sighand);
                            // core_state == NULL

		          exit_signals() // Sets PF_EXITING
  zap_threads()
    lock(sighand)
    // Observes T2->flags PF_POSTCOREDUMP
    // and skips T2

Now in current mainline a tkill(T2, SIGKILL) will queue the SIGKILL in
T2->pending, but complete_signal() will not turn it into a group exit
either because it is a PIDTYPE_PID signal when PF_EXITING is set:

complete_signal()
  // wants_signal() returns false because PF_EXITING is set
  if (wants_signal(sig, p)) 
      t = p;
  else if ((type == PIDTYPE_PID) || thread_group_empty(p))
      return;  // path taken because type == PIDTYPE_PID

So it is queued for nothing and just sitting in T2->pending until
flush_sigqueue() mops it up.

That has been so since:

  5fcd835bf8c2 ("signals: use __group_complete_signal() for the specific signals too")

which was merged 18 years ago in the 2.6.26 merge window.

Which means not queueing it in the first place has exactly the same
outcome vs. SIGKILL.

The only difference is that current mainline still reaches
signalfd_notify() further down in __send_signal_locked(), while with the
early exit it will not. Does it actually matter?

If it matters we could simply force PIDTYPE_TGID for SIGKILL if type ==
PIDTYPE_PID because complete_signal() converts SIGKILL into a group exit
anyway.

Thanks,

        tglx

Re: [patch V2 7/8] posix-cpu-timers: Prevent enqueueing when PF_EXITING is set

[Thomas Gleixner]

On Sun, Sep 06 2026 at 18:26, Oleg Nesterov wrote:
> On 09/05, Thomas Gleixner wrote:
>>
>> @@ -684,7 +706,13 @@ static void arm_timer(struct k_itimer *t
>>  	struct cpu_timer *ctmr = &timer->it.cpu;
>>  	u64 newexp = cpu_timer_getexpires(ctmr);
>>
>> +	lockdep_assert_held(&p->sighand->siglock);
>> +
>>  	timer->it_status = POSIX_TIMER_ARMED;
>> +
>> +	if (unlikely(!task_can_enqueue_timer(p, clock_pid_type(timer->it_clock))))
>> +		return;
>
> I can't understand why does it check task_can_enqueue_timer() after
> setting POSIX_TIMER_ARMED. This adds the new armed-but-not-enqueued state,
> afaics.
>
> I see nothing wrong, it seems that this can only affect __posix_cpu_timer_get()
> which checks ->it_status, other code paths do not check ->it_status.
>
> But I don't understand this code, so let me ask: is it on purpose? I mean,
> is there any reason to set _ARMED unconditionally ?

It is intentional.

The problem is that timer_create(2), timer_settime(2), timer_gettime(2)
and timer_delete(2) are "functional" today as long as a task is visible,
i.e. hashed. By some definition of functional.

Since f90fff1e152d ("posix-cpu-timers: fix race between
handle_posix_cpu_timers() and posix_cpu_timer_del()") an exiting task
does not expire timers anymore. That commit used task->exit_state, which
is set way after exit_signals() in do_exit(). I made that earlier in the
previous commit.

So while timers still can be [re-]armed they won't expire which means
that queueing them in the first place is pointless. But I kept the state
modification to avoid behavioural changes. The current state is that a
timer is "armed" until it expired and the signal is queued. I just
preserved that.

Of course one could argue that once a task reached PF_EXITING, timers
armed on them don't matter anymore and simply return -ESRCH, but that's
a user visible change which I wanted to avoid right now.

I'm happy to stop pretending that this "works" after PF_EXITING is set,
but don't have strong opinions either.

Thanks,

        tglx

Re: [patch V2 8/8] posix-timers: Handle exit in do_exit() completely

[Thomas Gleixner]

On Sun, Sep 06 2026 at 18:40, Oleg Nesterov wrote:

> I am still trying to understand this patch, one question for now.
>
> On 09/05, Thomas Gleixner wrote:
>>
>> +void posix_cpu_timers_exit_task(void)
>>  {
>> -	cleanup_timers(&tsk->posix_cputimers);
>> +	posix_cpu_timers_exit_work();
>> +
>> +	guard(spinlock_irq)(&current->sighand->siglock);
>> +	cleanup_timers(&current->posix_cputimers);
>>  }
>
> So it calls posix_cpu_timers_exit_work()
>
>> +static inline void posix_cpu_timers_exit_work(void)
>> +{
>> +	/*
>> +	 * current->flags has PF_EXITING set so this can be done lockless and
>> +	 * with interrupts enabled as PF_EXITING prevents the interrupt from
>> +	 * scheduling the work.
>> +	 */
>> +	if (current->posix_cputimers_work.scheduled)
>> +		task_work_cancel(current, &current->posix_cputimers_work.work);
>
> ... which does not clear ->scheduled
>
>>  void posixtimer_exec(void)
>>  {
>> -	scoped_guard(spinlock_irq, &current->sighand->siglock)
>> -		posix_cpu_timers_exit(current);
>> -
>> +	posix_cpu_timers_exit_task();
>
> ... and this looks obviously wrong for posixtimer_exec() ?

Yes.

Thanks,

        tglx
---
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1270,6 +1270,9 @@ static void posix_cpu_timers_work(struct callback_head *work)
 
 static inline void posix_cpu_timers_exit_work(void)
 {
+	/* Canceling the work is only valid for exit() but not for exec() */
+	if (!(current->flags & PF_EXITING))
+		return;
 	/*
 	 * current->flags has PF_EXITING set so this can be done lockless and
 	 * with interrupts enabled as PF_EXITING prevents the interrupt from

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Sat, Sep 05, 2026 at 08:59:01PM +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 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>
> Cc: stable@vger.kernel.org
> Closes: https://patch.msgid.link/aok1rdkBgZsynHZB@v4bel
> ---
> V4: Prevent requeuing of signals which are unignored - Oleg, Eric
> 
>     Split out the decision into an inline which can be reused by the posix
>     CPU timer follow up changes.
> 
> V3: Restructure code and fix the missing unlock - Oleg
> 
> 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 |  103 +++++++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 78 insertions(+), 36 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.
>   */
> @@ -1019,6 +1029,21 @@ static inline bool legacy_queue(struct s
>  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
>  }
>  
> +/*
> + * When PF_EXITING is set the task is on the way out and has t::pending
> + * flushed already. Prevent queueing of PIDTYPE_PID signals as they would
> + * be leaked.
> + */
> +static inline bool task_can_queue_signal(struct task_struct *t, enum pid_type type)
> +{
> +	lockdep_assert_held(&t->sighand->siglock);
> +
> +	if (!(t->flags & PF_EXITING))
> +		return true;
> +
> +	return type != PIDTYPE_PID;
> +}
> +
>  static int __send_signal_locked(int sig, struct kernel_siginfo *info,
>  				struct task_struct *t, enum pid_type type, bool force)
>  {
> @@ -1030,6 +1055,10 @@ static int __send_signal_locked(int sig,
>  	lockdep_assert_held(&t->sighand->siglock);
>  
>  	result = TRACE_SIGNAL_IGNORED;
> +
> +	if (!task_can_queue_signal(t, type))
> +		goto ret;
> +
>  	if (!prepare_signal(sig, t, force))
>  		goto ret;
>  
> @@ -1968,11 +1997,25 @@ static inline struct task_struct *posixt
>  	struct task_struct *t = pid_task(tmr->it_pid, tmr->it_pid_type);
>  
>  	if (t && tmr->it_pid_type != PIDTYPE_PID &&
> -	    same_thread_group(t, current) && !current->exit_state)
> +	    same_thread_group(t, current) && !(current->flags & PF_EXITING))
>  		t = current;
>  	return t;
>  }
>  
> +/*
> + * Find the target task for the POSIX timer signal and prevent that a
> + * PIDTYPE_PID signal is queued on a task which has PF_EXITING set.
> + */
> +static inline struct task_struct *posixtimer_get_unignore_target(struct k_itimer *tmr)
> +{
> +	struct task_struct *t = posixtimer_get_target(tmr);
> +
> +	if (t && task_can_queue_signal(t, tmr->it_pid_type))
> +		return t;
> +
> +	return NULL;
> +}
> +
>  void posixtimer_send_sigqueue(struct k_itimer *tmr)
>  {
>  	struct sigqueue *q = &tmr->sigq;
> @@ -1990,6 +2033,9 @@ void posixtimer_send_sigqueue(struct k_i
>  	if (!likely(lock_task_sighand(t, &flags)))
>  		return;
>  
> +	if (!task_can_queue_signal(t, tmr->it_pid_type))
> +		goto unlock;
> +
>  	/*
>  	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
>  	 * locked to prevent a race against dequeue_signal().
> @@ -2081,6 +2127,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);
>  }
>  
> @@ -2136,7 +2183,7 @@ static void posixtimer_sig_unignore(stru
>  		 * has exited by now, drop the reference count.
>  		 */
>  		guard(rcu)();
> -		target = posixtimer_get_target(tmr);
> +		target = posixtimer_get_unignore_target(tmr);
>  		if (target)
>  			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);
>  		else
> @@ -3120,42 +3167,36 @@ static void retarget_shared_pending(stru
>  
>  void exit_signals(struct task_struct *tsk)
>  {
> +	LIST_HEAD(sigq_list);
>  	int group_stop = 0;
> -	sigset_t unblocked;
>  
>  	/*
>  	 * @tsk is about to have PF_EXITING set - lock out users which
> -	 * expect stable threadgroup.
> +	 * expect a stable threadgroup.
>  	 */
>  	cgroup_threadgroup_change_begin(tsk);
>  
> -	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
> +	scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
>  		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 (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;
> +		}
> +	}
>  
> -	if (!task_sigpending(tsk))
> -		goto out;
> +	cgroup_threadgroup_change_end(tsk);
>  
> -	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;
> -out:
> -	spin_unlock_irq(&tsk->sighand->siglock);
> +	flush_sigqueue_list(&sigq_list);

It probably doesn't matter in practice, I don't know feel free to ignore,
but FWIW it looks like it's still vulnerable to the theoretical far fetched
race I described. The head is moved under the lock but individual nodes are
deleted without the lock.

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

exit_signals()
   spin_lock(sighand)
   tsk->flags |= PF_EXITING;
   list_splice_init(&queue->list, head);
   spin_unlock(sighand)

   list_for_each_safe(head, node)
      list_del_init(node)
         node->next = node // A
         node->prev = node // B
...
                                     de_thread()
                                        // acquired tsk->flags
                                        // and signal flushed
                                        // through tasklist_lock
                                        transfer_pid() // C
                                                           
                                                           posix_timer_fn()
                                                              posixtimer_send_sigqueue()
                                                                 // OBSERVES C
                                                                 t = posixtimer_get_target(tmr)
                                                                 lock_task_sighand()
                                                                    // OBSERVES A
                                                                    if (!list_empty(q))
                                                                       // BUT NOT B
                                                                       list_add_tail(q) // D

Then who knows which write wins, B or D?

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Mon, Sep 07 2026 at 14:31, Frederic Weisbecker wrote:
> Le Sat, Sep 05, 2026 at 08:59:01PM +0200, Thomas Gleixner a écrit :
>> -out:
>> -	spin_unlock_irq(&tsk->sighand->siglock);
>> +	flush_sigqueue_list(&sigq_list);
>
> It probably doesn't matter in practice, I don't know feel free to ignore,
> but FWIW it looks like it's still vulnerable to the theoretical far fetched
> race I described. The head is moved under the lock but individual nodes are
> deleted without the lock.
>
> CPU 0                                CPU 1                   CPU 2
> -----                                -----                   -----
>
> exit_signals()
>    spin_lock(sighand)
>    tsk->flags |= PF_EXITING;
>    list_splice_init(&queue->list, head);
>    spin_unlock(sighand)
>
>    list_for_each_safe(head, node)
>       list_del_init(node)
>          node->next = node // A
>          node->prev = node // B
> ...
>                                      de_thread()
>                                         // acquired tsk->flags
>                                         // and signal flushed
>                                         // through tasklist_lock
>                                         transfer_pid() // C
>                                                            
>                                                            posix_timer_fn()
>                                                               posixtimer_send_sigqueue()
>                                                                  // OBSERVES C
>                                                                  t = posixtimer_get_target(tmr)
>                                                                  lock_task_sighand()
>                                                                     // OBSERVES A
>                                                                     if (!list_empty(q))
>                                                                        // BUT NOT B
>                                                                        list_add_tail(q) // D
>
> Then who knows which write wins, B or D?

For a moment you almost convinced me, but that's not possible:

    de_thread()
        ....

	if (!thread_leader()) {
            wait_until(old_leader->exit_state);

            transfer_pid();

old_leader sets the exit_state in exit_notify():

    do_exit()
      exit_signals()
        lock(sighand)
          old_leader->flags |= PF_EXITING;
          head = remove_signals()
        unlock(sighand)
        flush_list(head)
     ...
     exit_notify()
       old_leader->exit_state = EXIT_XXX;

From a program order POV the flush is completed _before_ the new leader
can observe old_leader->exit_state and swap TIDS. exit_notify() and the
wait in de_thread() are serialized via tasklist_lock.

The signal is either dropped before transfer_pid() is observable due to
PF_EXITING on the old leader or queued on the new leader and then
discarded in posixtimer_exit() -> flush_itimer_signals().

The only valid question is whether it is guaranteed that on a weakly
ordered system the stores in flush_sigqueue_list() are visible _before_
transfer_pid() is visible to the third party.

It's not obvious of course and might deserve a comment.

      exit_signals()
        lock(sighand)
          old_leader->flags |= PF_EXITING;
          head = remove_signals()
#1      // RELEASE: PF_EXITING must become visible
        unlock(sighand)
        flush_list(head)

     ...
     posixtimer_exit()
       posix_cpu_timers_exit_task()
        lock(sighand)
        ...
#2      // RELEASE: The stores in flush_list() must become visible
        //          They might be already in case of preemption
        //	    or due a RELEASE operation in seccomp_filter_release()
        unlock(sighand)

    ...
    exit_notify()
      lock(task_list_lock)
      exit_state = EXIT_ZOMBIE;
#3    // RELEASE: exit_state must become visible
      unlock(task_list_lock)

So the new leader cannot proceed before #3 which means it can't swap
TIDs before that point. That requires task_list_lock so there is no way
that the TID swap can trickle before the lock is held and exit_state
being non-zero.

Though the important part is that the third party on CPU3 has to acquire
sighand lock in posixtimer_send_sigqueue(), which is an ACQUIRE
operation. That means _all_ accesses to tsk::flags and to the sigqueue
must happen _after_ the lock is acquired.

If it acquires it after #1 and before the TID swap it must observe
PF_EXITING and return immediately. So a concurrent modification of
timer::sigqueue in flush_list() or not-yet visible stores are
irrelevant.

If it acquires it after #2 it must observe the full writes to the
sigqueue. So after that point it does not longer matter whether the PID
resolves to T1 or T2.

No?

Thanks,

        tglx

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Mon, Sep 07, 2026 at 05:26:04PM +0200, Thomas Gleixner a écrit :
> On Mon, Sep 07 2026 at 14:31, Frederic Weisbecker wrote:
> > Le Sat, Sep 05, 2026 at 08:59:01PM +0200, Thomas Gleixner a écrit :
> >> -out:
> >> -	spin_unlock_irq(&tsk->sighand->siglock);
> >> +	flush_sigqueue_list(&sigq_list);
> >
> > It probably doesn't matter in practice, I don't know feel free to ignore,
> > but FWIW it looks like it's still vulnerable to the theoretical far fetched
> > race I described. The head is moved under the lock but individual nodes are
> > deleted without the lock.
> >
> > CPU 0                                CPU 1                   CPU 2
> > -----                                -----                   -----
> >
> > exit_signals()
> >    spin_lock(sighand)
> >    tsk->flags |= PF_EXITING;
> >    list_splice_init(&queue->list, head);
> >    spin_unlock(sighand)
> >
> >    list_for_each_safe(head, node)
> >       list_del_init(node)
> >          node->next = node // A
> >          node->prev = node // B
> > ...
> >                                      de_thread()
> >                                         // acquired tsk->flags
> >                                         // and signal flushed
> >                                         // through tasklist_lock
> >                                         transfer_pid() // C
> >                                                            
> >                                                            posix_timer_fn()
> >                                                               posixtimer_send_sigqueue()
> >                                                                  // OBSERVES C
> >                                                                  t = posixtimer_get_target(tmr)
> >                                                                  lock_task_sighand()
> >                                                                     // OBSERVES A
> >                                                                     if (!list_empty(q))
> >                                                                        // BUT NOT B
> >                                                                        list_add_tail(q) // D
> >
> > Then who knows which write wins, B or D?
> 
> For a moment you almost convinced me, but that's not possible:
> 
>     de_thread()
>         ....
> 
> 	if (!thread_leader()) {
>             wait_until(old_leader->exit_state);
> 
>             transfer_pid();
> 
> old_leader sets the exit_state in exit_notify():
> 
>     do_exit()
>       exit_signals()
>         lock(sighand)
>           old_leader->flags |= PF_EXITING;
>           head = remove_signals()
>         unlock(sighand)
>         flush_list(head)
>      ...
>      exit_notify()
>        old_leader->exit_state = EXIT_XXX;
> 
> From a program order POV the flush is completed _before_ the new leader
> can observe old_leader->exit_state and swap TIDS. exit_notify() and the
> wait in de_thread() are serialized via tasklist_lock.

Yes on that side all is program order. But the ordering is not mirrored on
the other side (at this stage of the patchset).

> 
> The signal is either dropped before transfer_pid() is observable due to
> PF_EXITING on the old leader or queued on the new leader and then
> discarded in posixtimer_exit() -> flush_itimer_signals().
> 
> The only valid question is whether it is guaranteed that on a weakly
> ordered system the stores in flush_sigqueue_list() are visible _before_
> transfer_pid() is visible to the third party.
> 
> It's not obvious of course and might deserve a comment.
> 
>       exit_signals()
>         lock(sighand)
>           old_leader->flags |= PF_EXITING;
>           head = remove_signals()
> #1      // RELEASE: PF_EXITING must become visible
>         unlock(sighand)
>         flush_list(head)
> 
>      ...
>      posixtimer_exit()
>        posix_cpu_timers_exit_task()
>         lock(sighand)
>         ...
> #2      // RELEASE: The stores in flush_list() must become visible
>         //          They might be already in case of preemption
>         //	    or due a RELEASE operation in seccomp_filter_release()
>         unlock(sighand)

That second step only appears at the end of the patchset, right? Otherwise
it's done on release_task(), which is after transfer_pid().

> 
>     ...
>     exit_notify()
>       lock(task_list_lock)
>       exit_state = EXIT_ZOMBIE;
> #3    // RELEASE: exit_state must become visible
>       unlock(task_list_lock)
> 
> So the new leader cannot proceed before #3 which means it can't swap
> TIDs before that point. That requires task_list_lock so there is no way
> that the TID swap can trickle before the lock is held and exit_state
> being non-zero.
> 
> Though the important part is that the third party on CPU3 has to acquire
> sighand lock in posixtimer_send_sigqueue(), which is an ACQUIRE
> operation. That means _all_ accesses to tsk::flags and to the sigqueue
> must happen _after_ the lock is acquired.
> 
> If it acquires it after #1 and before the TID swap it must observe
> PF_EXITING and return immediately. So a concurrent modification of
> timer::sigqueue in flush_list() or not-yet visible stores are
> irrelevant.
> 
> If it acquires it after #2 it must observe the full writes to the
> sigqueue. So after that point it does not longer matter whether the PID
> resolves to T1 or T2.
> 
> No?

At the end of the patchset yes. But it doesn't look that way in this
very patch which is to be backported alone.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 2/8] exec: Cleanup POSIX timers right after de_thread()

[Frederic Weisbecker]

Le Sat, Sep 05, 2026 at 08:59:06PM +0200, Thomas Gleixner a écrit :
> From: Hyunwoo Kim <imv4bel@gmail.com>
> 
> A per-thread CPU timer holds a reference to the PID of the thread it is
> attached to and, while it is armed, its node is queued in that thread's
> posix_cputimers. The task is looked up by that PID.
> 
> When a non-leader thread exec()s, de_thread() changes which task owns
> that PID. pid_task(timer->it.cpu.pid, PIDTYPE_PID) then returns NULL,
> but the node is still queued on tsk, which is alive. timer_lock_sighand()
> takes a failed lookup to mean that the node is already dequeued, so it
> has nothing to undo.
> 
> begin_new_exec() calls posix_cpu_timers_exit(me) right after
> exec_task_namespaces() and that removes the leftover node, so the state
> normally stays invisible. But bprm->point_of_no_return is set before
> de_thread(), so if unshare_files(), set_mm_exe_file(), exec_mmap() or
> exec_task_namespaces() fails, the task dies before it gets there.
> exit_itimers() then frees the k_itimer while its node is still queued,
> and reaping tsk later erases that freed node from the rbtree.
> 
> In short:
> 
>       the non-leader thread B           the parent
> 
>   timer_create(CLOCK_THREAD_CPUTIME_ID)
>   timer_settime()
>     arm_timer()            // the node is queued on B
>   execve()
>     de_thread(B)
>       exchange_tids(B, leader)  // B's PID now belongs to the leader
>       release_task(leader)
>         __exit_signal(leader)
>           posix_cpu_timers_exit(leader)  // cleans leader's queue, not B's
>           __unhash_process(leader)  // that PID has no task anymore
>     exec_mmap()
>       mmap_read_lock_killable(old_mm)
>                                 kill(B, SIGKILL)
>       // -EINTR
>   get_signal()
>     do_exit()
>       exit_itimers()
>         posix_timer_delete()
>           posix_cpu_timer_del()
>         posix_timer_unhash_and_free()  // freed while still queued
>                                 wait4()
>                                   release_task(B)
>                                     posix_cpu_timers_exit(B)
>                                       cleanup_timerqueue()
>                                         timerqueue_del()  // use-after-free
> 
> Move the POSIX timer cleanup right after de_thread() before any of the
> later failure conditions brings the task into do_exit().
> 
> [ tglx: Move the cleanup right after de_thread() ]
> 
> Fixes: 55e8c8eb2c7b ("posix-cpu-timers: Store a reference to a pid not a task")
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Cc: stable@vger.kernel.org
> Link: https://patch.msgid.link/ao7Q8miiuLAPVnWv@v4bel

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Mon, Sep 07 2026 at 22:15, Frederic Weisbecker wrote:
> Le Mon, Sep 07, 2026 at 05:26:04PM +0200, Thomas Gleixner a écrit :
>> It's not obvious of course and might deserve a comment.
>> 
>>       exit_signals()
>>         lock(sighand)
>>           old_leader->flags |= PF_EXITING;
>>           head = remove_signals()
>> #1      // RELEASE: PF_EXITING must become visible
>>         unlock(sighand)
>>         flush_list(head)
>> 
>>      ...
>>      posixtimer_exit()
>>        posix_cpu_timers_exit_task()
>>         lock(sighand)
>>         ...
>> #2      // RELEASE: The stores in flush_list() must become visible
>>         //          They might be already in case of preemption
>>         //	    or due a RELEASE operation in seccomp_filter_release()
>>         unlock(sighand)
>
> That second step only appears at the end of the patchset, right? Otherwise
> it's done on release_task(), which is after transfer_pid().

Cleaning up the enqueued posix CPU timers has nothing to do with the
signals.

>>     exit_notify()
>>       lock(task_list_lock)
>>       exit_state = EXIT_ZOMBIE;
>> #3    // RELEASE: exit_state must become visible

In context of patch 1 alone, this RELEASE operation guarantees that the
stores in flush_list() are visible.

The new leader cannot proceed with swapping the TIDs _before_ it
acquires task list lock and observes under task_list_lock

         old_leader->exit_state != 0

The TID swap cannot be reordered by the CPU _before_ task list lock is
acquired and the exit_state is observed as non-zero.

As the exit_notify() RELEASE made both the exit_state store and the
preceeding flush_list() stores visible the third party must observe them
correctly as well when it can observe the TID swap.

It does not matter whether the RELEASE operation after flush_list() is
spin_unlock(siglock) or any other RELEASE operation before and including
the final one in exit_notify().

Any of them will provide the guarantee because _all_ preceeding stores
must be visible before the RELEASE operation is complete. 

No?

Thanks,

        tglx

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Tue, Sep 08, 2026 at 12:28:04AM +0200, Thomas Gleixner a écrit :
> On Mon, Sep 07 2026 at 22:15, Frederic Weisbecker wrote:
> > Le Mon, Sep 07, 2026 at 05:26:04PM +0200, Thomas Gleixner a écrit :
> >> It's not obvious of course and might deserve a comment.
> >> 
> >>       exit_signals()
> >>         lock(sighand)
> >>           old_leader->flags |= PF_EXITING;
> >>           head = remove_signals()
> >> #1      // RELEASE: PF_EXITING must become visible
> >>         unlock(sighand)
> >>         flush_list(head)
> >> 
> >>      ...
> >>      posixtimer_exit()
> >>        posix_cpu_timers_exit_task()
> >>         lock(sighand)
> >>         ...
> >> #2      // RELEASE: The stores in flush_list() must become visible
> >>         //          They might be already in case of preemption
> >>         //	    or due a RELEASE operation in seccomp_filter_release()
> >>         unlock(sighand)
> >
> > That second step only appears at the end of the patchset, right? Otherwise
> > it's done on release_task(), which is after transfer_pid().
> 
> Cleaning up the enqueued posix CPU timers has nothing to do with the
> signals.
> 
> >>     exit_notify()
> >>       lock(task_list_lock)
> >>       exit_state = EXIT_ZOMBIE;
> >> #3    // RELEASE: exit_state must become visible
> 
> In context of patch 1 alone, this RELEASE operation guarantees that the
> stores in flush_list() are visible.
> 
> The new leader cannot proceed with swapping the TIDs _before_ it
> acquires task list lock and observes under task_list_lock
> 
>          old_leader->exit_state != 0
> 
> The TID swap cannot be reordered by the CPU _before_ task list lock is
> acquired and the exit_state is observed as non-zero.
> 
> As the exit_notify() RELEASE made both the exit_state store and the
> preceeding flush_list() stores visible the third party must observe them
> correctly as well when it can observe the TID swap.
> 
> It does not matter whether the RELEASE operation after flush_list() is
> spin_unlock(siglock) or any other RELEASE operation before and including
> the final one in exit_notify().
> 
> Any of them will provide the guarantee because _all_ preceeding stores
> must be visible before the RELEASE operation is complete. 
> 
> No?
> 
> Thanks,

Yes this side is well ordered but what about the other side.
Ok let's simplify the picture:

Old leader                           Exec'ing New leader     CPU 2
-----                                -----                   -----

WRITE q->next = q
WRITE q->prev = q

ACQUIRE tasklist
RELEASE tasklist
                                     ACQUIRE tasklist
                                     RELEASE tasklist

                                     WRITE pid
                                                             READ pid
							     // smp_mb()
                                                             if q->next == q
                                                                 WRITE q->prev

Isn't there a missing pairing full barrier in CPU 2 ?

Thanks.

Re: [patch V2 1/8] signal: Prevent exec() race

[Oleg Nesterov]

On 09/08, Frederic Weisbecker wrote:
>
> Old leader                           Exec'ing New leader     CPU 2
> -----                                -----                   -----
>
> WRITE q->next = q
> WRITE q->prev = q

Damn ;) I am shy to suggest this again, but if we have _any_
concerns about the races with list_del_init()...

The lockless flush_sigqueue_list() doesn't need to it, right?
So perhaps something like below (on top of this series) to avoid
the (potential) race explicitly?

Most probably this change is wrong. And in any case we can do better.
Just to explain what I mean.

In short, I mean that exit_signals() -> flush path can do
__sigqueue_free(q) without list_del_init(&q->list).

Yes! I agree in advance that if this change can fix something,
then it fixes the symptom.

But at the same time, why does flush_sigqueue_list(head) need
list_del_init() before __sigqueue_free() ? AFAICS only to make
list_empty(head) == true. This looks confusing to me.

Oh, I am sure I missed something again ;)

Oleg.
---

diff --git a/kernel/signal.c b/kernel/signal.c
index f93d8f77ec1a..32c344ee4769 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -457,14 +457,19 @@ static void __sigqueue_free(struct sigqueue *q)
 	kmem_cache_free(sigqueue_cachep, q);
 }
 
-static void flush_sigqueue_list(struct list_head *head)
+static void __flush_sigqueue_list(struct list_head *head)
 {
 	struct sigqueue *q, *tmp;
 
-	list_for_each_entry_safe(q, tmp, head, list) {
-		list_del_init(&q->list);
+	list_for_each_entry_safe(q, tmp, head, list)
 		__sigqueue_free(q);
-	}
+}
+
+static void flush_sigqueue_list(struct list_head *head)
+{
+	LIST_HEAD(flush);
+	list_splice_init(head, &flush);
+	__flush_sigqueue_list(&flush);
 }
 
 void flush_sigqueue(struct sigpending *queue)
@@ -3196,7 +3201,7 @@ void exit_signals(struct task_struct *tsk)
 
 	cgroup_threadgroup_change_end(tsk);
 
-	flush_sigqueue_list(&sigq_list);
+	__flush_sigqueue_list(&sigq_list);
 
 	/*
 	 * If group stop has completed, deliver the notification.  This

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:

> Yes this side is well ordered but what about the other side.
> Ok let's simplify the picture:
> 
> Old leader                           Exec'ing New leader     CPU 2
> -----                                -----                   -----
> 
> WRITE q->next = q
> WRITE q->prev = q
> 
> ACQUIRE tasklist
> RELEASE tasklist
>                                      ACQUIRE tasklist
>                                      RELEASE tasklist
> 
>                                      WRITE pid
>                                                              READ pid
> 							     // smp_mb()
>                                                              if q->next == q
>                                                                  WRITE q->prev
> 
> Isn't there a missing pairing full barrier in CPU 2 ?


Let me try and have a go :-)


do_exit()				de_thread() 			posix_timer_fn()
  exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
    LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
    tsk->flags |= PF_EXITING; 						    LOCK siglock
    UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
    					    LOCK tasklist_lock
    flush_sigqueue_list();		    if (leader->exit_state)
					      break;
  ...                                       transfer_pid()
					    UNLOCK tasklist_lock
  exit_notify()
    LOCK tasklist_lock
    tsk->exit_state = EXIT_ZOMBIE;
    UNLOCK tasklist_lock



Then there is indeed nothing that makes sure posix_timer_fn() sees
sigqueue updates done by do_exit(), because those are ordered by
tasklist_lock, but posix_timer_fn() doesn't care about that.

The easy solution would probably be to do transfer_pid() while holding
siglock?

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Wed, Sep 09 2026 at 10:04, Peter Zijlstra wrote:
> On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:
> Let me try and have a go :-)
>
>
> do_exit()				de_thread() 			posix_timer_fn()
>   exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
>     LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
>     tsk->flags |= PF_EXITING; 						    LOCK siglock
>     UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
>     					    LOCK tasklist_lock
>     flush_sigqueue_list();		    if (leader->exit_state)
> 					      break;
>   ...                                       transfer_pid()
> 					    UNLOCK tasklist_lock
>   exit_notify()
>     LOCK tasklist_lock
>     tsk->exit_state = EXIT_ZOMBIE;
>     UNLOCK tasklist_lock
>
>
>
> Then there is indeed nothing that makes sure posix_timer_fn() sees
> sigqueue updates done by do_exit(), because those are ordered by
> tasklist_lock, but posix_timer_fn() doesn't care about that.

That's irrelevant because in the above scenario posix_timer_fn() 't'
points to the exiting old leader (on the left) because the PID store has
not happened yet and it therefore observes PF_EXITING on it so it won't
touch the sigqueue. Note, that setting and checking PF_EXITING is
serialized by sighand lock, so this is fine.

 do_exit()
   exit_signals()
     LOCK siglock
     tsk->flags |= PF_EXITING
     UNLOCK siglock

So after this point anything which looks at tsk->flags under siglock
will observe PF_EXITING and not touch the sigqueue. Nothing to see here.

> The easy solution would probably be to do transfer_pid() while holding
> siglock?

That'd be only relevant for the situation Frederic is concerned about,
i.e. the case where the third party observes the TID swap.

Because with that visible 't' in posix_timer_send_sigqueue() won't be
old_leader, which has PF_EXITING set, it will be new_leader which has it
not set.

So Frederic is concerned that posix_timer_send_sigqueue() can observe
the PID store but not observe the sigqueue stores.

I argue that's not possible:

A:  sigqueue stores

B:   AQUIRE tasklist

C:   exit_state store

D:   RELEASE tasklist
     // sigqueue and exit_state stores become globally visible
 ------------------------------------------------------------------------

E                               ACQUIRE tasklist
 ------------------------------------------------------------------------
F                                if (exit_state)
				   swap_pid()
G                                     STORE_PID

				// The PID store can become visible in the
				// system right here so F can observe them before
				// RELEASE tasklist

H                                                             READ PID
							      ....
I                                		              ACQUIRE siglock

After #A the sigqueue stores are maybe visible

After #C the exit_state store is maybe visible

After #D both #A and #C are guaranteed to be visible to _ALL_ agents in
the system and cannot become magically become invisible after that
point.

The new leader cannot swap PIDs before acquiring task list lock and
before it observed exit_state != 0 under it. That's fully serialized
against the old leader as both hold task list lock for their operations.

#F creates a control dependency, so if the new leader acquires task list
   lock before the old it will observe 0, drop the lock and wait. No PID
   store obviously.

#G can be come visible immediately but is only guaranteed to be visible
   globally at the RELEASE of tasklist lock.

#H can only observe the PID store after the store actually happened in
   #G. So it either reads the original PID or the swapped PID.

#I is not really relevant for this. It's only relevant for PF_EXITING
   and other stuff which is directly protected by it. And it does not
   matter whether it locks the old or the new sighand.

Now let's look at the full chain and what can possibly be visible or not
and when:

#A can trickle into the tasklist held section, but not after #D.

#C cannot be reordered against #B and #D

#A is therefore guaranteed to be globally visible _before_ new leader
   observes exit_state != 0 in #F under task list lock

#G cannot be reordered against #F and obviously not against #E either.

   It can become visible at any point after the store, but as argued
   above that visibility can't be reordered before #A (sigqueue stores)
   became visible.

The important part is that the visibility of #A (sigqueue stores) and #G
(PID store) is fully ordered through task list lock.

So #H _cannot_ observe #G without observing #A - not even on PowerPC or
similar insanities.

No?

Also doing the PID swap under sighand lock is not solving anything
either because posix_timer_send_sigqueue() does the lookup without the
lock simply because it does not know which task it is upfront. So it
would have to redo and validate the lookup with the lock held.

Thanks,

        tglx

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 10:04:07AM +0200, Peter Zijlstra a écrit :
> On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:
> 
> > Yes this side is well ordered but what about the other side.
> > Ok let's simplify the picture:
> > 
> > Old leader                           Exec'ing New leader     CPU 2
> > -----                                -----                   -----
> > 
> > WRITE q->next = q
> > WRITE q->prev = q
> > 
> > ACQUIRE tasklist
> > RELEASE tasklist
> >                                      ACQUIRE tasklist
> >                                      RELEASE tasklist
> > 
> >                                      WRITE pid
> >                                                              READ pid
> > 							     // smp_mb()
> >                                                              if q->next == q
> >                                                                  WRITE q->prev
> > 
> > Isn't there a missing pairing full barrier in CPU 2 ?
> 
> 
> Let me try and have a go :-)
> 
> 
> do_exit()				de_thread() 			posix_timer_fn()
>   exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
>     LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
>     tsk->flags |= PF_EXITING; 						    LOCK siglock
>     UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
>     					    LOCK tasklist_lock
>     flush_sigqueue_list();		    if (leader->exit_state)
> 					      break;
>   ...                                       transfer_pid()
> 					    UNLOCK tasklist_lock
>   exit_notify()
>     LOCK tasklist_lock
>     tsk->exit_state = EXIT_ZOMBIE;
>     UNLOCK tasklist_lock
> 
> 
> 
> Then there is indeed nothing that makes sure posix_timer_fn() sees
> sigqueue updates done by do_exit(), because those are ordered by
> tasklist_lock, but posix_timer_fn() doesn't care about that.
> 
> The easy solution would probably be to do transfer_pid() while holding
> siglock?

That should work, especially with a big fat comment, and the tasklist_lock ->
sighand lock dependency already exists.

Thanks!

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 02:03:42AM +0200, Oleg Nesterov a écrit :
> On 09/08, Frederic Weisbecker wrote:
> >
> > Old leader                           Exec'ing New leader     CPU 2
> > -----                                -----                   -----
> >
> > WRITE q->next = q
> > WRITE q->prev = q
> 
> Damn ;) I am shy to suggest this again, but if we have _any_
> concerns about the races with list_del_init()...
> 
> The lockless flush_sigqueue_list() doesn't need to it, right?
> So perhaps something like below (on top of this series) to avoid
> the (potential) race explicitly?
> 
> Most probably this change is wrong. And in any case we can do better.
> Just to explain what I mean.
> 
> In short, I mean that exit_signals() -> flush path can do
> __sigqueue_free(q) without list_del_init(&q->list).
> 
> Yes! I agree in advance that if this change can fix something,
> then it fixes the symptom.
> 
> But at the same time, why does flush_sigqueue_list(head) need
> list_del_init() before __sigqueue_free() ? AFAICS only to make
> list_empty(head) == true. This looks confusing to me.
> 
> Oh, I am sure I missed something again ;)
> 
> Oleg.
> ---
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index f93d8f77ec1a..32c344ee4769 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -457,14 +457,19 @@ static void __sigqueue_free(struct sigqueue *q)
>  	kmem_cache_free(sigqueue_cachep, q);
>  }
>  
> -static void flush_sigqueue_list(struct list_head *head)
> +static void __flush_sigqueue_list(struct list_head *head)
>  {
>  	struct sigqueue *q, *tmp;
>  
> -	list_for_each_entry_safe(q, tmp, head, list) {
> -		list_del_init(&q->list);
> +	list_for_each_entry_safe(q, tmp, head, list)
>  		__sigqueue_free(q);
> -	}
> +}

I must confess that leaves me an uncomfortable taste :-)

What do you think about Peter's solution?

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Wed, Sep 09, 2026 at 11:08:31AM +0200, Thomas Gleixner wrote:
> On Wed, Sep 09 2026 at 10:04, Peter Zijlstra wrote:
> > On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:
> > Let me try and have a go :-)
> >
> >
> > do_exit()				de_thread() 			posix_timer_fn()
> >   exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
> >     LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
> >     tsk->flags |= PF_EXITING; 						    LOCK siglock
> >     UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
> >     					    LOCK tasklist_lock
> >     flush_sigqueue_list();		    if (leader->exit_state)
> > 					      break;
> >   ...                                       transfer_pid()
> > 					    UNLOCK tasklist_lock
> >   exit_notify()
> >     LOCK tasklist_lock
> >     tsk->exit_state = EXIT_ZOMBIE;
> >     UNLOCK tasklist_lock
> >
> >
> >
> > Then there is indeed nothing that makes sure posix_timer_fn() sees
> > sigqueue updates done by do_exit(), because those are ordered by
> > tasklist_lock, but posix_timer_fn() doesn't care about that.
> 
> That's irrelevant because in the above scenario posix_timer_fn() 't'
> points to the exiting old leader (on the left) because the PID store has
> not happened yet and it therefore observes PF_EXITING on it so it won't
> touch the sigqueue. Note, that setting and checking PF_EXITING is
> serialized by sighand lock, so this is fine.

There is nothing that constraints the 3rd column from happening before,
it could happen after transfer_pid().

> > The easy solution would probably be to do transfer_pid() while holding
> > siglock?
> 
> That'd be only relevant for the situation Frederic is concerned about,
> i.e. the case where the third party observes the TID swap.

That is the case I was aiming at.

> Because with that visible 't' in posix_timer_send_sigqueue() won't be
> old_leader, which has PF_EXITING set, it will be new_leader which has it
> not set.

Same as above, there is nothing constraining the 3rd column from sliding
up or down. If it manages to see the new_leader, I don't see why it
would see the sigqueue flush.

> So Frederic is concerned that posix_timer_send_sigqueue() can observe
> the PID store but not observe the sigqueue stores.
> 
> I argue that's not possible:
> 
> A:  sigqueue stores
> 
> B:   AQUIRE tasklist
> 
> C:   exit_state store
> 
> D:   RELEASE tasklist
>      // sigqueue and exit_state stores become globally visible
>  ------------------------------------------------------------------------
> 
> E                               ACQUIRE tasklist
>  ------------------------------------------------------------------------
> F                                if (exit_state)
> 				   swap_pid()
> G                                     STORE_PID
> 
> 				// The PID store can become visible in the
> 				// system right here so F can observe them before
> 				// RELEASE tasklist

The STORE_PID is not a STORE_RELEASE.

> H                                                             READ PID

And this READ is not LOAD_AQUIRE; although the LOCK siglock is probably
sufficient here. The READ MUST happen before LOCK siglock by means of
data dependency, and then the LOCK will constrain later loads.

> 							      ....
> I                                		              ACQUIRE siglock
> 
> After #A the sigqueue stores are maybe visible
> 
> After #C the exit_state store is maybe visible
> 
> After #D both #A and #C are guaranteed to be visible to _ALL_ agents in
> the system and cannot become magically become invisible after that
> point.

No, that is not in fact how Power (or ARM) works AFAICT. Memory ordering
is not global. It is entirely possible some CPUs see a store while
others do not.

The only guarantee here is that IF you acquire tasklist_lock (you
observe the store that unlocked it), you will also observe preceding
stores.  But since the posix_timer_fn() column does not in fact observe
or care about tasklist_lock, there is no ordering.

> The new leader cannot swap PIDs before acquiring task list lock and
> before it observed exit_state != 0 under it. That's fully serialized
> against the old leader as both hold task list lock for their operations.
> 
> #F creates a control dependency, so if the new leader acquires task list
>    lock before the old it will observe 0, drop the lock and wait. No PID
>    store obviously.

A control dependency only ensure *that* CPU will complete the exit_state
load before the store, it is a local LOAD->STORE ordering.

> #G can be come visible immediately but is only guaranteed to be visible
>    globally at the RELEASE of tasklist lock.

Nope, not at all. Can be randomly visible to random sets of CPUs.

> #H can only observe the PID store after the store actually happened in
>    #G. So it either reads the original PID or the swapped PID.

Sure. But that has no bearing on if it sees the sigqueue stores at A.

> #I is not really relevant for this. It's only relevant for PF_EXITING
>    and other stuff which is directly protected by it. And it does not
>    matter whether it locks the old or the new sighand.
> 
> Now let's look at the full chain and what can possibly be visible or not
> and when:
> 
> #A can trickle into the tasklist held section, but not after #D.

Yup.

> #C cannot be reordered against #B and #D

Agreed.

> #A is therefore guaranteed to be globally visible _before_ new leader
>    observes exit_state != 0 in #F under task list lock

Nope, A is therefore visible if you acquire tasklist_lock, specifically,
when you observe the store from D. And only if that matching LOAD is a
LOAD-ACQUIRE, such that subsequent loads are forced to be later.

> #G cannot be reordered against #F and obviously not against #E either.

Indeed.

>    It can become visible at any point after the store, but as argued
>    above that visibility can't be reordered before #A (sigqueue stores)
>    became visible.

Let G' be the unnamed RELEASE after G.

Now, I have deleted and rewritten this tail end at least twice now. And
I *think* I'm agreeing with you. Let me explain:

It all hinges on D-E and H-I.

D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
RELEASE+ACQUIRE. Specifically, we have:

  RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
  UNLOCK+LOCK:     RCtso, the hand-over is store-ordering

So while earlier I was arguing with RCpc in mind, in which case D-E
completely goes away and we can consider B-G' to be one big critical
section from the PoV of a third CPU (our posix_timer_fn() one). In this
case we can push A down and G up and have them cross.

*However*, since these are locks, we actually have D-E be UNLOCK+LOCK,
which is RCtso and that *does* impose store order, so A stores must
happen before G stores

Combine with H-I, which has a data dependency from the LOAD to the LOCK
and thereby constraints later LOADs, those sigqueue loads that come
after I must in fact observe the A stores.

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 11:08:31AM +0200, Thomas Gleixner a écrit :
> On Wed, Sep 09 2026 at 10:04, Peter Zijlstra wrote:
> > On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:
> > Let me try and have a go :-)
> >
> >
> > do_exit()				de_thread() 			posix_timer_fn()
> >   exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
> >     LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
> >     tsk->flags |= PF_EXITING; 						    LOCK siglock
> >     UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
> >     					    LOCK tasklist_lock
> >     flush_sigqueue_list();		    if (leader->exit_state)
> > 					      break;
> >   ...                                       transfer_pid()
> > 					    UNLOCK tasklist_lock
> >   exit_notify()
> >     LOCK tasklist_lock
> >     tsk->exit_state = EXIT_ZOMBIE;
> >     UNLOCK tasklist_lock
> >
> >
> >
> > Then there is indeed nothing that makes sure posix_timer_fn() sees
> > sigqueue updates done by do_exit(), because those are ordered by
> > tasklist_lock, but posix_timer_fn() doesn't care about that.
> 
> That's irrelevant because in the above scenario posix_timer_fn() 't'
> points to the exiting old leader (on the left) because the PID store has
> not happened yet and it therefore observes PF_EXITING on it so it won't
> touch the sigqueue. Note, that setting and checking PF_EXITING is
> serialized by sighand lock, so this is fine.
> 
>  do_exit()
>    exit_signals()
>      LOCK siglock
>      tsk->flags |= PF_EXITING
>      UNLOCK siglock
> 
> So after this point anything which looks at tsk->flags under siglock
> will observe PF_EXITING and not touch the sigqueue. Nothing to see here.
> 
> > The easy solution would probably be to do transfer_pid() while holding
> > siglock?
> 
> That'd be only relevant for the situation Frederic is concerned about,
> i.e. the case where the third party observes the TID swap.
> 
> Because with that visible 't' in posix_timer_send_sigqueue() won't be
> old_leader, which has PF_EXITING set, it will be new_leader which has it
> not set.
> 
> So Frederic is concerned that posix_timer_send_sigqueue() can observe
> the PID store but not observe the sigqueue stores.
> 
> I argue that's not possible:
> 
> A:  sigqueue stores
> 
> B:   AQUIRE tasklist
> 
> C:   exit_state store
> 
> D:   RELEASE tasklist
>      // sigqueue and exit_state stores become globally visible
>  ------------------------------------------------------------------------
> 
> E                               ACQUIRE tasklist
>  ------------------------------------------------------------------------
> F                                if (exit_state)
> 				   swap_pid()
> G                                     STORE_PID
> 
> 				// The PID store can become visible in the
> 				// system right here so F can observe them before
> 				// RELEASE tasklist
> 
> H                                                             READ PID
> 							      ....
> I                                		              ACQUIRE siglock
> 
> After #A the sigqueue stores are maybe visible
> 
> After #C the exit_state store is maybe visible
> 
> After #D both #A and #C are guaranteed to be visible to _ALL_ agents in
> the system and cannot become magically become invisible after that
> point.
> 
> The new leader cannot swap PIDs before acquiring task list lock and
> before it observed exit_state != 0 under it. That's fully serialized
> against the old leader as both hold task list lock for their operations.
> 
> #F creates a control dependency, so if the new leader acquires task list
>    lock before the old it will observe 0, drop the lock and wait. No PID
>    store obviously.
> 
> #G can be come visible immediately but is only guaranteed to be visible
>    globally at the RELEASE of tasklist lock.
> 
> #H can only observe the PID store after the store actually happened in
>    #G. So it either reads the original PID or the swapped PID.
> 
> #I is not really relevant for this. It's only relevant for PF_EXITING
>    and other stuff which is directly protected by it. And it does not
>    matter whether it locks the old or the new sighand.
> 
> Now let's look at the full chain and what can possibly be visible or not
> and when:
> 
> #A can trickle into the tasklist held section, but not after #D.
> 
> #C cannot be reordered against #B and #D
> 
> #A is therefore guaranteed to be globally visible _before_ new leader
>    observes exit_state != 0 in #F under task list lock
> 
> #G cannot be reordered against #F and obviously not against #E either.
> 
>    It can become visible at any point after the store, but as argued
>    above that visibility can't be reordered before #A (sigqueue stores)
>    became visible.
> 
> The important part is that the visibility of #A (sigqueue stores) and #G
> (PID store) is fully ordered through task list lock.
> 
> So #H _cannot_ observe #G without observing #A - not even on PowerPC or
> similar insanities.
> 
> No?

I've always been told that ordering only works if paired.
But in practice I must confess I don't know much about hardware details.

> 
> Also doing the PID swap under sighand lock is not solving anything
> either because posix_timer_send_sigqueue() does the lookup without the
> lock simply because it does not know which task it is upfront. So it
> would have to redo and validate the lookup with the lock held.

It does the lookup without the lock but if pid is rewritten inside
the siglock on the write side and we observe the new pid from read side, then
acquiring the lock afterwards on the read side also acquires what it has
released previously (that is, everything that was acquired by tasklist_lock,
including the list_del_init()).

Not sure if my words are clear but tools/memory-model/litmus-tests/MP+polocks.litmus
explains that better.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Wed, Sep 09, 2026 at 11:55:19AM +0200, Peter Zijlstra wrote:
> D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
> RELEASE+ACQUIRE. Specifically, we have:
> 
>   RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
>   UNLOCK+LOCK:     RCtso, the hand-over is store-ordering
> 

For those that case: UNLOCK+LOCK is RCsc on all architectures except
Power.

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Wed, Sep 09 2026 at 11:55, Peter Zijlstra wrote:
> On Wed, Sep 09, 2026 at 11:08:31AM +0200, Thomas Gleixner wrote:
> Now, I have deleted and rewritten this tail end at least twice now. And
> I *think* I'm agreeing with you. Let me explain:
>
> It all hinges on D-E and H-I.
>
> D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
> RELEASE+ACQUIRE. Specifically, we have:
>
>   RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
>   UNLOCK+LOCK:     RCtso, the hand-over is store-ordering

Yes. I should have argued with UNLOCK+LOCK instead. My bad.

> So while earlier I was arguing with RCpc in mind, in which case D-E
> completely goes away and we can consider B-G' to be one big critical
> section from the PoV of a third CPU (our posix_timer_fn() one). In this
> case we can push A down and G up and have them cross.

Correct.

> *However*, since these are locks, we actually have D-E be UNLOCK+LOCK,
> which is RCtso and that *does* impose store order, so A stores must
> happen before G stores

Yes. That was my thinking, but I obviously expressed it incorrectly.

> Combine with H-I, which has a data dependency from the LOAD to the LOCK
> and thereby constraints later LOADs, those sigqueue loads that come
> after I must in fact observe the A stores.

Right. I guess it's worth to document that somewhere at least in the
change log of this patch.

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 11:55:18AM +0200, Peter Zijlstra a écrit :
> On Wed, Sep 09, 2026 at 11:08:31AM +0200, Thomas Gleixner wrote:
> > On Wed, Sep 09 2026 at 10:04, Peter Zijlstra wrote:
> > > On Tue, Sep 08, 2026 at 12:15:21PM +0200, Frederic Weisbecker wrote:
> > > Let me try and have a go :-)
> > >
> > >
> > > do_exit()				de_thread() 			posix_timer_fn()
> > >   exit_signal()				  LOCK siglock 			  posix_timer_send_sigqueue()
> > >     LOCK siglock			  UNLOCK siglock 		    t = posix_timer_get_target()
> > >     tsk->flags |= PF_EXITING; 						    LOCK siglock
> > >     UNLOCK siglock			  if (!thread_group_leader) 	      if (!list_empty(sigqueue))
> > >     					    LOCK tasklist_lock
> > >     flush_sigqueue_list();		    if (leader->exit_state)
> > > 					      break;
> > >   ...                                       transfer_pid()
> > > 					    UNLOCK tasklist_lock
> > >   exit_notify()
> > >     LOCK tasklist_lock
> > >     tsk->exit_state = EXIT_ZOMBIE;
> > >     UNLOCK tasklist_lock
> > >
> > >
> > >
> > > Then there is indeed nothing that makes sure posix_timer_fn() sees
> > > sigqueue updates done by do_exit(), because those are ordered by
> > > tasklist_lock, but posix_timer_fn() doesn't care about that.
> > 
> > That's irrelevant because in the above scenario posix_timer_fn() 't'
> > points to the exiting old leader (on the left) because the PID store has
> > not happened yet and it therefore observes PF_EXITING on it so it won't
> > touch the sigqueue. Note, that setting and checking PF_EXITING is
> > serialized by sighand lock, so this is fine.
> 
> There is nothing that constraints the 3rd column from happening before,
> it could happen after transfer_pid().
> 
> > > The easy solution would probably be to do transfer_pid() while holding
> > > siglock?
> > 
> > That'd be only relevant for the situation Frederic is concerned about,
> > i.e. the case where the third party observes the TID swap.
> 
> That is the case I was aiming at.
> 
> > Because with that visible 't' in posix_timer_send_sigqueue() won't be
> > old_leader, which has PF_EXITING set, it will be new_leader which has it
> > not set.
> 
> Same as above, there is nothing constraining the 3rd column from sliding
> up or down. If it manages to see the new_leader, I don't see why it
> would see the sigqueue flush.
> 
> > So Frederic is concerned that posix_timer_send_sigqueue() can observe
> > the PID store but not observe the sigqueue stores.
> > 
> > I argue that's not possible:
> > 
> > A:  sigqueue stores
> > 
> > B:   AQUIRE tasklist
> > 
> > C:   exit_state store
> > 
> > D:   RELEASE tasklist
> >      // sigqueue and exit_state stores become globally visible
> >  ------------------------------------------------------------------------
> > 
> > E                               ACQUIRE tasklist
> >  ------------------------------------------------------------------------
> > F                                if (exit_state)
> > 				   swap_pid()
> > G                                     STORE_PID
> > 
> > 				// The PID store can become visible in the
> > 				// system right here so F can observe them before
> > 				// RELEASE tasklist
> 
> The STORE_PID is not a STORE_RELEASE.
> 
> > H                                                             READ PID
> 
> And this READ is not LOAD_AQUIRE; although the LOCK siglock is probably
> sufficient here. The READ MUST happen before LOCK siglock by means of
> data dependency, and then the LOCK will constrain later loads.
> 
> > 							      ....
> > I                                		              ACQUIRE siglock
> > 
> > After #A the sigqueue stores are maybe visible
> > 
> > After #C the exit_state store is maybe visible
> > 
> > After #D both #A and #C are guaranteed to be visible to _ALL_ agents in
> > the system and cannot become magically become invisible after that
> > point.
> 
> No, that is not in fact how Power (or ARM) works AFAICT. Memory ordering
> is not global. It is entirely possible some CPUs see a store while
> others do not.
> 
> The only guarantee here is that IF you acquire tasklist_lock (you
> observe the store that unlocked it), you will also observe preceding
> stores.  But since the posix_timer_fn() column does not in fact observe
> or care about tasklist_lock, there is no ordering.
> 
> > The new leader cannot swap PIDs before acquiring task list lock and
> > before it observed exit_state != 0 under it. That's fully serialized
> > against the old leader as both hold task list lock for their operations.
> > 
> > #F creates a control dependency, so if the new leader acquires task list
> >    lock before the old it will observe 0, drop the lock and wait. No PID
> >    store obviously.
> 
> A control dependency only ensure *that* CPU will complete the exit_state
> load before the store, it is a local LOAD->STORE ordering.
> 
> > #G can be come visible immediately but is only guaranteed to be visible
> >    globally at the RELEASE of tasklist lock.
> 
> Nope, not at all. Can be randomly visible to random sets of CPUs.
> 
> > #H can only observe the PID store after the store actually happened in
> >    #G. So it either reads the original PID or the swapped PID.
> 
> Sure. But that has no bearing on if it sees the sigqueue stores at A.
> 
> > #I is not really relevant for this. It's only relevant for PF_EXITING
> >    and other stuff which is directly protected by it. And it does not
> >    matter whether it locks the old or the new sighand.
> > 
> > Now let's look at the full chain and what can possibly be visible or not
> > and when:
> > 
> > #A can trickle into the tasklist held section, but not after #D.
> 
> Yup.
> 
> > #C cannot be reordered against #B and #D
> 
> Agreed.
> 
> > #A is therefore guaranteed to be globally visible _before_ new leader
> >    observes exit_state != 0 in #F under task list lock
> 
> Nope, A is therefore visible if you acquire tasklist_lock, specifically,
> when you observe the store from D. And only if that matching LOAD is a
> LOAD-ACQUIRE, such that subsequent loads are forced to be later.
> 
> > #G cannot be reordered against #F and obviously not against #E either.
> 
> Indeed.
> 
> >    It can become visible at any point after the store, but as argued
> >    above that visibility can't be reordered before #A (sigqueue stores)
> >    became visible.
> 
> Let G' be the unnamed RELEASE after G.
> 
> Now, I have deleted and rewritten this tail end at least twice now. And
> I *think* I'm agreeing with you. Let me explain:
> 
> It all hinges on D-E and H-I.
> 
> D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
> RELEASE+ACQUIRE. Specifically, we have:
> 
>   RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
>   UNLOCK+LOCK:     RCtso, the hand-over is store-ordering
> 
> So while earlier I was arguing with RCpc in mind, in which case D-E
> completely goes away and we can consider B-G' to be one big critical
> section from the PoV of a third CPU (our posix_timer_fn() one). In this
> case we can push A down and G up and have them cross.
> 
> *However*, since these are locks, we actually have D-E be UNLOCK+LOCK,
> which is RCtso and that *does* impose store order, so A stores must
> happen before G stores
> 
> Combine with H-I, which has a data dependency from the LOAD to the LOCK
> and thereby constraints later LOADs, those sigqueue loads that come
> after I must in fact observe the A stores.

I didn't know about all those UNLOCK+LOCK properties. Well,
I know that UNLOCK+LOCK on the same lock, or on different locks
but the same CPU, equals smp_mb() except on powerpc. Which is why
we have smp_mb__after_unlock_lock(). But what you describe is quite
different.

Is this something that we should expect litmus to modelize?

Because the following doesn't verify that:
---
C MP+farfetched

{}

P0(int *next, int *prev, int *exit_state, spinlock_t *tasklist_lock)
{
	// list_del_init()
	WRITE_ONCE(*next, 1);
	WRITE_ONCE(*prev, 1);
	// exit_notify()
	spin_lock(tasklist_lock);
	WRITE_ONCE(*exit_state, 1);
	spin_unlock(tasklist_lock);
}

P1(int *exit_state, int *pid, spinlock_t *tasklist_lock)
{
	int r0;

	// de_thread()
	spin_lock(tasklist_lock);
	r0 = READ_ONCE(*exit_state);
	if (r0 == 1) {
		// exchange_tids()
		WRITE_ONCE(*pid, 1);
	}
	spin_unlock(tasklist_lock);
}

P2(int *next, int *prev, int *pid, spinlock_t *sighand)
{
	int r0;
	int r1;
	// get target
	r0 = READ_ONCE(*pid);
	spin_lock(sighand);
	// queue signal
	r1 = READ_ONCE(*next);
	if (r1 == 0)
		WRITE_ONCE(*prev, 2);
	spin_unlock(sighand);
}

exists (prev=1 /\ 2:r0=1) (* Bad outcome. *)
---
herd7 -conf linux-kernel.cfg ~/farfetched.litmus 
Test MP+farfetched Allowed
States 4
2:r0=0; [prev]=1;
2:r0=0; [prev]=2;
2:r0=1; [prev]=1;
2:r0=1; [prev]=2;
Ok
Witnesses
Positive: 2 Negative: 7
Condition exists ([prev]=1 /\ 2:r0=1)
Observation MP+farfetched Sometimes 2 7
Time MP+farfetched 0.02
Hash=a44733c870613a81ae096a93babe215

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:

> > > I argue that's not possible:
> > > 
> > > A:   sigqueue stores
> > > 
> > > B:   AQUIRE tasklist
> > > 
> > > C:   exit_state store
> > > 
> > > D:   RELEASE tasklist
> > > 
> > > E                               ACQUIRE tasklist

> > > F                               if (exit_state)
> > > 				    	swap_pid()
> > > G                                   STORE_PID
> > > 
> > > 				      RELEASE tasklist
> > > 
> > > H                                                       READ PID
> > > 							      ....
> > > I                                		              ACQUIRE siglock

> > Let G' be the unnamed RELEASE after G.
> > 
> > Now, I have deleted and rewritten this tail end at least twice now. And
> > I *think* I'm agreeing with you. Let me explain:
> > 
> > It all hinges on D-E and H-I.
> > 
> > D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
> > RELEASE+ACQUIRE. Specifically, we have:
> > 
> >   RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
> >   UNLOCK+LOCK:     RCtso, the hand-over is store-ordering
> > 
> > So while earlier I was arguing with RCpc in mind, in which case D-E
> > completely goes away and we can consider B-G' to be one big critical
> > section from the PoV of a third CPU (our posix_timer_fn() one). In this
> > case we can push A down and G up and have them cross.
> > 
> > *However*, since these are locks, we actually have D-E be UNLOCK+LOCK,
> > which is RCtso and that *does* impose store order, so A stores must
> > happen before G stores
> > 
> > Combine with H-I, which has a data dependency from the LOAD to the LOCK
> > and thereby constraints later LOADs, those sigqueue loads that come
> > after I must in fact observe the A stores.
> 
> I didn't know about all those UNLOCK+LOCK properties. Well,
> I know that UNLOCK+LOCK on the same lock, or on different locks
> but the same CPU, equals smp_mb() except on powerpc. Which is why
> we have smp_mb__after_unlock_lock(). But what you describe is quite
> different.
> 
> Is this something that we should expect litmus to modelize?

IIRC these commits:

6e89e831a901 ("tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire")
ddfe12944e84 ("tools/memory-model: Provide extra ordering for unlock+lock pair on the same CPU")

Were supposed to handle:

	CPU0		CPU1

	UNLOCK(A)
			LOCK(A)

and

	CPU0

	UNLOCK(A)
	LOCK(B)

respectively. I'm forever confused by the actual CAT stuff, nor am I
particularly adept at these litmus things. Boqun, Alan?

> Because the following doesn't verify that:
> ---
> C MP+farfetched
> 
> {}
> 
> P0(int *next, int *prev, int *exit_state, spinlock_t *tasklist_lock)
> {
> 	// list_del_init()
> 	WRITE_ONCE(*next, 1);
> 	WRITE_ONCE(*prev, 1);
> 	// exit_notify()
> 	spin_lock(tasklist_lock);
> 	WRITE_ONCE(*exit_state, 1);
> 	spin_unlock(tasklist_lock);
> }
> 
> P1(int *exit_state, int *pid, spinlock_t *tasklist_lock)
> {
> 	int r0;
> 
> 	// de_thread()
> 	spin_lock(tasklist_lock);
> 	r0 = READ_ONCE(*exit_state);
> 	if (r0 == 1) {
> 		// exchange_tids()
> 		WRITE_ONCE(*pid, 1);
> 	}
> 	spin_unlock(tasklist_lock);
> }
> 
> P2(int *next, int *prev, int *pid, spinlock_t *sighand)
> {
> 	int r0;
> 	int r1;
> 	// get target
> 	r0 = READ_ONCE(*pid);
> 	spin_lock(sighand);
> 	// queue signal
> 	r1 = READ_ONCE(*next);
> 	if (r1 == 0)
> 		WRITE_ONCE(*prev, 2);
> 	spin_unlock(sighand);
> }
> 
> exists (prev=1 /\ 2:r0=1) (* Bad outcome. *)
> ---
> herd7 -conf linux-kernel.cfg ~/farfetched.litmus 
> Test MP+farfetched Allowed
> States 4
> 2:r0=0; [prev]=1;
> 2:r0=0; [prev]=2;
> 2:r0=1; [prev]=1;
> 2:r0=1; [prev]=2;
> Ok
> Witnesses
> Positive: 2 Negative: 7
> Condition exists ([prev]=1 /\ 2:r0=1)
> Observation MP+farfetched Sometimes 2 7
> Time MP+farfetched 0.02
> Hash=a44733c870613a81ae096a93babe215

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:

> > Because the following doesn't verify that:
> > ---
> > C MP+farfetched
> > 
> > {}
> > 
> > P0(int *next, int *prev, int *exit_state, spinlock_t *tasklist_lock)
> > {
> > 	// list_del_init()
> > 	WRITE_ONCE(*next, 1);
> > 	WRITE_ONCE(*prev, 1);
> > 	// exit_notify()
> > 	spin_lock(tasklist_lock);
> > 	WRITE_ONCE(*exit_state, 1);
> > 	spin_unlock(tasklist_lock);
> > }
> > 
> > P1(int *exit_state, int *pid, spinlock_t *tasklist_lock)
> > {
> > 	int r0;
> > 
> > 	// de_thread()
> > 	spin_lock(tasklist_lock);
> > 	r0 = READ_ONCE(*exit_state);
> > 	if (r0 == 1) {
> > 		// exchange_tids()
> > 		WRITE_ONCE(*pid, 1);
> > 	}
> > 	spin_unlock(tasklist_lock);
> > }
> > 
> > P2(int *next, int *prev, int *pid, spinlock_t *sighand)
> > {
> > 	int r0;
> > 	int r1;
> > 	// get target
> > 	r0 = READ_ONCE(*pid);
> > 	spin_lock(sighand);

There is no dependency between r0 and sighand. While I think there is in
posixtimer_send_sigqueue(). Does making it smp_load_acquire() help?

> > 	// queue signal
> > 	r1 = READ_ONCE(*next);
> > 	if (r1 == 0)
> > 		WRITE_ONCE(*prev, 2);
> > 	spin_unlock(sighand);
> > }
> > 
> > exists (prev=1 /\ 2:r0=1) (* Bad outcome. *)

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Wed, Sep 09 2026 at 14:51, Peter Zijlstra wrote:
> On Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra wrote:
>> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:
>> > P2(int *next, int *prev, int *pid, spinlock_t *sighand)
>> > {
>> > 	int r0;
>> > 	int r1;
>> > 	// get target
>> > 	r0 = READ_ONCE(*pid);
>> > 	spin_lock(sighand);
>
> There is no dependency between r0 and sighand. While I think there is in
> posixtimer_send_sigqueue(). Does making it smp_load_acquire() help?

sighand is r0->sighand->siglock and obviously not known before r0 is
read. So yes there is a data dependency in reality :)

>> > 	// queue signal
>> > 	r1 = READ_ONCE(*next);
>> > 	if (r1 == 0)
>> > 		WRITE_ONCE(*prev, 2);
>> > 	spin_unlock(sighand);
>> > }
>> > 
>> > exists (prev=1 /\ 2:r0=1) (* Bad outcome. *)

Re: [patch V2 1/8] signal: Prevent exec() race

[Alan Stern]

On Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:
> > I didn't know about all those UNLOCK+LOCK properties. Well,
> > I know that UNLOCK+LOCK on the same lock, or on different locks
> > but the same CPU, equals smp_mb() except on powerpc. Which is why
> > we have smp_mb__after_unlock_lock(). But what you describe is quite
> > different.
> > 
> > Is this something that we should expect litmus to modelize?
> 
> IIRC these commits:
> 
> 6e89e831a901 ("tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire")
> ddfe12944e84 ("tools/memory-model: Provide extra ordering for unlock+lock pair on the same CPU")
> 
> Were supposed to handle:
> 
> 	CPU0		CPU1
> 
> 	UNLOCK(A)
> 			LOCK(A)
> 
> and
> 
> 	CPU0
> 
> 	UNLOCK(A)
> 	LOCK(B)
> 
> respectively. I'm forever confused by the actual CAT stuff, nor am I
> particularly adept at these litmus things. Boqun, Alan?

You got it right.  The required ordering is that instructions before 
(and on the same CPU as) the unlock are ordered before instructions 
after (and on the same CPU as) the lock for read->read, read->write, and 
write->write cases, but not for write->read cases.  Furthermore, this 
ordering requirement applies from the point of view of all CPUs, whether 
they access the lock variables or not.  In other words, the ordering is 
required to be RCtso.

This ordering is weaker than full smp_mb() in two respects:

	As mentioned above, it doesn't order earlier writes against
	later reads;

	In the WRITE->WRITE case, the guarantee is only that each CPU
	will observe the first write before it observes the second.
	There is no guarantee that _every_ CPU will observe the first
	write before _any_ of them observe the second.

Alan Stern

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra a écrit :
> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:
> 
> > > > I argue that's not possible:
> > > > 
> > > > A:   sigqueue stores
> > > > 
> > > > B:   AQUIRE tasklist
> > > > 
> > > > C:   exit_state store
> > > > 
> > > > D:   RELEASE tasklist
> > > > 
> > > > E                               ACQUIRE tasklist
> 
> > > > F                               if (exit_state)
> > > > 				    	swap_pid()
> > > > G                                   STORE_PID
> > > > 
> > > > 				      RELEASE tasklist
> > > > 
> > > > H                                                       READ PID
> > > > 							      ....
> > > > I                                		              ACQUIRE siglock
> 
> > > Let G' be the unnamed RELEASE after G.
> > > 
> > > Now, I have deleted and rewritten this tail end at least twice now. And
> > > I *think* I'm agreeing with you. Let me explain:
> > > 
> > > It all hinges on D-E and H-I.
> > > 
> > > D-E is a UNLOCK+LOCK hand-over, which is not quite the same as
> > > RELEASE+ACQUIRE. Specifically, we have:
> > > 
> > >   RELEASE+ACQUIRE: RCpc,  only the CPUs involved agree on the ordering
> > >   UNLOCK+LOCK:     RCtso, the hand-over is store-ordering
> > > 
> > > So while earlier I was arguing with RCpc in mind, in which case D-E
> > > completely goes away and we can consider B-G' to be one big critical
> > > section from the PoV of a third CPU (our posix_timer_fn() one). In this
> > > case we can push A down and G up and have them cross.
> > > 
> > > *However*, since these are locks, we actually have D-E be UNLOCK+LOCK,
> > > which is RCtso and that *does* impose store order, so A stores must
> > > happen before G stores
> > > 
> > > Combine with H-I, which has a data dependency from the LOAD to the LOCK
> > > and thereby constraints later LOADs, those sigqueue loads that come
> > > after I must in fact observe the A stores.
> > 
> > I didn't know about all those UNLOCK+LOCK properties. Well,
> > I know that UNLOCK+LOCK on the same lock, or on different locks
> > but the same CPU, equals smp_mb() except on powerpc. Which is why
> > we have smp_mb__after_unlock_lock(). But what you describe is quite
> > different.
> > 
> > Is this something that we should expect litmus to modelize?
> 
> IIRC these commits:
> 
> 6e89e831a901 ("tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire")
> ddfe12944e84 ("tools/memory-model: Provide extra ordering for unlock+lock pair
> on the same CPU")

I didn't know that UNLOCK+LOCK can pair with smp_load_acquire(). Good to know.

But unlock+lock doesn't pair with unlock+lock on different CPU.

C MP+polocks

{}

P0(int *A, int *B, spinlock_t *mylock)
{
	spin_lock(mylock);
	WRITE_ONCE(*A, 1);
	spin_unlock(mylock);
	spin_lock(mylock);
	WRITE_ONCE(*B, 1);
	spin_unlock(mylock);
}

P1(int *A, int *B, spinlock_t *otherlock)
{
	int r0;
	int r1;

	r0 = READ_ONCE(*B);
	spin_lock(otherlock);
	r1 = READ_ONCE(*A);
	spin_unlock(otherlock);
}

exists (1:r0=1 /\ 1:r1=0) (* Bad outcome happens *)

But yeah there is no data dependency involved. Let me answer to
that to tglx.

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 03:45:36PM +0200, Thomas Gleixner a écrit :
> On Wed, Sep 09 2026 at 14:51, Peter Zijlstra wrote:
> > On Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra wrote:
> >> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:
> >> > P2(int *next, int *prev, int *pid, spinlock_t *sighand)
> >> > {
> >> > 	int r0;
> >> > 	int r1;
> >> > 	// get target
> >> > 	r0 = READ_ONCE(*pid);
> >> > 	spin_lock(sighand);
> >
> > There is no dependency between r0 and sighand. While I think there is in
> > posixtimer_send_sigqueue(). Does making it smp_load_acquire() help?
> 
> sighand is r0->sighand->siglock and obviously not known before r0 is
> read. So yes there is a data dependency in reality :)

So unfortunately litmus tests don't support structures. So instead
of transfering the pid, I fake the data dependency by transfering the
sighand directly and then yes it works. I don't know what is the name
of the pattern behind that.

This is not a control dependency as there is no LOAD-cond-store. If
someone can shed some light on this?

C MP+farfetched

{}

P0(int *next, int *prev, int *exit_state, spinlock_t *tasklist_lock)
{
	// list_del_init()
	WRITE_ONCE(*next, 1);
	WRITE_ONCE(*prev, 1);
	// exit_notify()
	spin_lock(tasklist_lock);
	WRITE_ONCE(*exit_state, 1);
	spin_unlock(tasklist_lock);
}

P1(int *exit_state, spinlock_t *tasklist_lock, spinlock_t *sighand, spinlock_t **psighand)
{
	int r0;

	// de_thread()
	spin_lock(tasklist_lock);
	r0 = READ_ONCE(*exit_state);
	if (r0 == 1) {
		// exchange_tids()
		WRITE_ONCE(*psighand, sighand);
	}
	spin_unlock(tasklist_lock);
}

P2(int *next, int *prev, int *pid, spinlock_t *sighand, spinlock_t **psighand)
{
	spinlock_t *r0;
	int r1;

	// get target
	r0 = READ_ONCE(*psighand);
	spin_lock(r0);
	// queue signal
	r1 = READ_ONCE(*next);
	if (r1 == 1) {
		WRITE_ONCE(*prev, 2);
	}
	spin_unlock(r0);
}

exists (prev=1 /\ 2:r0=sighand) (* Bad outcome. *)
---
herd7 -conf linux-kernel.cfg ~/farfetched.litmus 
Test MP+farfetched Allowed
States 3
2:r0=0; [prev]=1;
2:r0=0; [prev]=2;
2:r0=sighand; [prev]=2;
No
Witnesses
Positive: 0 Negative: 7
Condition exists ([prev]=1 /\ 2:r0=sighand)
Observation MP+farfetched Never 0 7

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 03:45:36PM +0200, Thomas Gleixner a écrit :
> On Wed, Sep 09 2026 at 14:51, Peter Zijlstra wrote:
> > On Wed, Sep 09, 2026 at 02:45:55PM +0200, Peter Zijlstra wrote:
> >> On Wed, Sep 09, 2026 at 02:13:11PM +0200, Frederic Weisbecker wrote:
> >> > P2(int *next, int *prev, int *pid, spinlock_t *sighand)
> >> > {
> >> > 	int r0;
> >> > 	int r1;
> >> > 	// get target
> >> > 	r0 = READ_ONCE(*pid);
> >> > 	spin_lock(sighand);
> >
> > There is no dependency between r0 and sighand. While I think there is in
> > posixtimer_send_sigqueue(). Does making it smp_load_acquire() help?
> 
> sighand is r0->sighand->siglock and obviously not known before r0 is
> read. So yes there is a data dependency in reality :)

Ah but the script writes *psighand on one hand and the spin_lock on the
other hand does an acquire so of course it works. But that doesn't tell
if that's transferrable through struct pid->task write on one hand and
struct pid->task deref on the other hand plus task->sighand dependencies.

My brain melts enough for today...

Re: [patch V2 1/8] signal: Prevent exec() race

[Alan Stern]

On Wed, Sep 09, 2026 at 04:45:15PM +0200, Frederic Weisbecker wrote:
> I didn't know that UNLOCK+LOCK can pair with smp_load_acquire(). Good to know.

I don't know what that means.  Regardless, LOCK doesn't pair with 
smp_load_acquire().  UNLOCK does, but only to the extent that acts as a 
release.

> But unlock+lock doesn't pair with unlock+lock on different CPU.
> 
> C MP+polocks
> 
> {}
> 
> P0(int *A, int *B, spinlock_t *mylock)
> {
> 	spin_lock(mylock);
> 	WRITE_ONCE(*A, 1);
> 	spin_unlock(mylock);
> 	spin_lock(mylock);
> 	WRITE_ONCE(*B, 1);
> 	spin_unlock(mylock);
> }
> 
> P1(int *A, int *B, spinlock_t *otherlock)
> {
> 	int r0;
> 	int r1;
> 
> 	r0 = READ_ONCE(*B);
> 	spin_lock(otherlock);
> 	r1 = READ_ONCE(*A);
> 	spin_unlock(otherlock);
> }
> 
> exists (1:r0=1 /\ 1:r1=0) (* Bad outcome happens *)

I can't tell what you're trying to do here.  The UNLOCK-LOCK ordering 
in P0 means that P1 sees A=1 before it sees B=1.  But nothing in this 
litmus test forces P1 to execute READ_ONCE(*b) before READ_ONCE(*A).  If 
the reads are executed in the opposite order, you can see how P1 might 
get r0=1 and r1=0.

If P1 had done this instead:

P1(int *A, int *B)
{
	int r0;
	int r1;

	r0 = READ_ONCE(*B);
	smp_rmb();
	r1 = READ_ONCE(*A);
}

then r0=1 and r1=0 would be impossible.

Alan Stern

Re: [patch V2 1/8] signal: Prevent exec() race

[Thomas Gleixner]

On Wed, Sep 09 2026 at 15:28, Alan Stern wrote:
> On Wed, Sep 09, 2026 at 04:45:15PM +0200, Frederic Weisbecker wrote:
> I can't tell what you're trying to do here.  The UNLOCK-LOCK ordering 
> in P0 means that P1 sees A=1 before it sees B=1.  But nothing in this 
> litmus test forces P1 to execute READ_ONCE(*b) before READ_ONCE(*A).  If 
> the reads are executed in the opposite order, you can see how P1 might 
> get r0=1 and r1=0.

The problem we are debating is:

    C = VAL1

    CPU0                    CPU1		CPU2
    
    STORE(A0, 0)
    STORE(A1, 0)

    LOCK(TLOCK)
    STORE(B, 1) // 0 -> 1
    UNLOCK(TLOCK)

                           LOCK(TLOCK)
                           b = LOAD(B)
                           if (b)
                              STORE(C, VAL0)

                                                c = LOAD(C)
                                                LOCK(c->lock)
                                                  a0 = LOAD(A0)
                                                  if (!a0)
                                                     STORE(A0, X1)
                                                     STORE(A1, X2)

The question is whether CPU2 can observe C == VAL0 and A0 == NULL before
A1 has completed.

My and Peter's argument is that the sequence

   UNLOCK(TLOCK) on CPU0   -> LOCK(TLOCK) on CPU1

implies RCtso and therefore the stores to A0 and A1 on CPU0 must be
before the store to C on CPU1.

Now because the LOAD(C) on CPU2 depends on that STORE(C) the
LOCK(c->lock) ensures that LOAD(A0) can't be reordered and because of
that STORE(A1, 0) has completed before that.

CPU2 LOAD(C) observing VAL0 has a data dependency on the STORE(C, VAL0)
on CPU1, which as argued above can only happen after the UNLOCK/LOCK
sequence CPU1 observes the STORE(B).

Subsequently LOCK(c->lock) has a data dependency on LOAD(C) and the LOCK
operation prevents that LOAD(A0) can be reordered before LOCK(c->lock).

So despite the fact that c->lock != TLOCK the UNLOCK(TLOCK)/LOCK(TLOCK)
sequence, which implies RCtso, the following takes care of it:

    1) the data dependency between the STORE(C, VAL0) on CPU1 and the
       c = LOAD(C) on CPU2 observing VAL0
   
    2) the data dependency of LOCK(c->lock) on #1

    3) due to LOCK() in #2 LOAD(A0) cannot observe the STORE(A0, 0) on
       CPU0 without the STORE(A1, 0) on CPU0 has completed.

If #3 can happen then that would obviously cause undebuggable data
corruption.

I hope this is understandable enough despite my brain having melted
several times by now while writing it up.

Thanks,

        tglx

Re: [patch V2 1/8] signal: Prevent exec() race

[Alan Stern]

On Wed, Sep 09, 2026 at 10:49:30PM +0200, Thomas Gleixner wrote:
> On Wed, Sep 09 2026 at 15:28, Alan Stern wrote:
> > On Wed, Sep 09, 2026 at 04:45:15PM +0200, Frederic Weisbecker wrote:
> > I can't tell what you're trying to do here.  The UNLOCK-LOCK ordering 
> > in P0 means that P1 sees A=1 before it sees B=1.  But nothing in this 
> > litmus test forces P1 to execute READ_ONCE(*b) before READ_ONCE(*A).  If 
> > the reads are executed in the opposite order, you can see how P1 might 
> > get r0=1 and r1=0.
> 
> The problem we are debating is:
> 
>     C = VAL1
> 
>     CPU0                    CPU1		CPU2
>     
>     STORE(A0, 0)
>     STORE(A1, 0)
> 
>     LOCK(TLOCK)
>     STORE(B, 1) // 0 -> 1
>     UNLOCK(TLOCK)
> 
>                            LOCK(TLOCK)
>                            b = LOAD(B)
>                            if (b)
>                               STORE(C, VAL0)
> 
>                                                 c = LOAD(C)
>                                                 LOCK(c->lock)
>                                                   a0 = LOAD(A0)
>                                                   if (!a0)
>                                                      STORE(A0, X1)
>                                                      STORE(A1, X2)
> 
> The question is whether CPU2 can observe C == VAL0 and A0 == NULL before
> A1 has completed.
> 
> My and Peter's argument is that the sequence
> 
>    UNLOCK(TLOCK) on CPU0   -> LOCK(TLOCK) on CPU1
> 
> implies RCtso and therefore the stores to A0 and A1 on CPU0 must be
> before the store to C on CPU1.
> 
> Now because the LOAD(C) on CPU2 depends on that STORE(C) the
> LOCK(c->lock) ensures that LOAD(A0) can't be reordered and because of
> that STORE(A1, 0) has completed before that.
> 
> CPU2 LOAD(C) observing VAL0 has a data dependency on the STORE(C, VAL0)
> on CPU1, which as argued above can only happen after the UNLOCK/LOCK
> sequence CPU1 observes the STORE(B).
> 
> Subsequently LOCK(c->lock) has a data dependency on LOAD(C) and the LOCK
> operation prevents that LOAD(A0) can be reordered before LOCK(c->lock).
> 
> So despite the fact that c->lock != TLOCK the UNLOCK(TLOCK)/LOCK(TLOCK)
> sequence, which implies RCtso, the following takes care of it:
> 
>     1) the data dependency between the STORE(C, VAL0) on CPU1 and the
>        c = LOAD(C) on CPU2 observing VAL0
>    
>     2) the data dependency of LOCK(c->lock) on #1
> 
>     3) due to LOCK() in #2 LOAD(A0) cannot observe the STORE(A0, 0) on
>        CPU0 without the STORE(A1, 0) on CPU0 has completed.
> 
> If #3 can happen then that would obviously cause undebuggable data
> corruption.
> 
> I hope this is understandable enough despite my brain having melted
> several times by now while writing it up.

I see.  Yes, your analysis is right.  And Frederic's latest LKML litmus 
test confirms the result.

Alan Stern

Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker]

Le Wed, Sep 09, 2026 at 05:11:27PM -0400, Alan Stern a écrit :
> On Wed, Sep 09, 2026 at 10:49:30PM +0200, Thomas Gleixner wrote:
> > On Wed, Sep 09 2026 at 15:28, Alan Stern wrote:
> > > On Wed, Sep 09, 2026 at 04:45:15PM +0200, Frederic Weisbecker wrote:
> > > I can't tell what you're trying to do here.  The UNLOCK-LOCK ordering 
> > > in P0 means that P1 sees A=1 before it sees B=1.  But nothing in this 
> > > litmus test forces P1 to execute READ_ONCE(*b) before READ_ONCE(*A).  If 
> > > the reads are executed in the opposite order, you can see how P1 might 
> > > get r0=1 and r1=0.
> > 
> > The problem we are debating is:
> > 
> >     C = VAL1
> > 
> >     CPU0                    CPU1		CPU2
> >     
> >     STORE(A0, 0)
> >     STORE(A1, 0)
> > 
> >     LOCK(TLOCK)
> >     STORE(B, 1) // 0 -> 1
> >     UNLOCK(TLOCK)
> > 
> >                            LOCK(TLOCK)
> >                            b = LOAD(B)
> >                            if (b)
> >                               STORE(C, VAL0)
> > 
> >                                                 c = LOAD(C)
> >                                                 LOCK(c->lock)
> >                                                   a0 = LOAD(A0)
> >                                                   if (!a0)
> >                                                      STORE(A0, X1)
> >                                                      STORE(A1, X2)
> > 
> > The question is whether CPU2 can observe C == VAL0 and A0 == NULL before
> > A1 has completed.
> > 
> > My and Peter's argument is that the sequence
> > 
> >    UNLOCK(TLOCK) on CPU0   -> LOCK(TLOCK) on CPU1
> > 
> > implies RCtso and therefore the stores to A0 and A1 on CPU0 must be
> > before the store to C on CPU1.
> > 
> > Now because the LOAD(C) on CPU2 depends on that STORE(C) the
> > LOCK(c->lock) ensures that LOAD(A0) can't be reordered and because of
> > that STORE(A1, 0) has completed before that.
> > 
> > CPU2 LOAD(C) observing VAL0 has a data dependency on the STORE(C, VAL0)
> > on CPU1, which as argued above can only happen after the UNLOCK/LOCK
> > sequence CPU1 observes the STORE(B).
> > 
> > Subsequently LOCK(c->lock) has a data dependency on LOAD(C) and the LOCK
> > operation prevents that LOAD(A0) can be reordered before LOCK(c->lock).
> > 
> > So despite the fact that c->lock != TLOCK the UNLOCK(TLOCK)/LOCK(TLOCK)
> > sequence, which implies RCtso, the following takes care of it:
> > 
> >     1) the data dependency between the STORE(C, VAL0) on CPU1 and the
> >        c = LOAD(C) on CPU2 observing VAL0
> >    
> >     2) the data dependency of LOCK(c->lock) on #1
> > 
> >     3) due to LOCK() in #2 LOAD(A0) cannot observe the STORE(A0, 0) on
> >        CPU0 without the STORE(A1, 0) on CPU0 has completed.
> > 
> > If #3 can happen then that would obviously cause undebuggable data
> > corruption.
> > 
> > I hope this is understandable enough despite my brain having melted
> > several times by now while writing it up.
> 
> I see.  Yes, your analysis is right.  And Frederic's latest LKML litmus 
> test confirms the result.

Alan, let me ask you something, because I'm the only one here puzzled by
this data dependency.

The following scenario works (the bad outcome never happens) because
UNLOCK+LOCK pairs with smp_load_acquire():

	C MP+polocks

	{}

	P0(int *A, int *B, spinlock_t *mylock)
	{
		spin_lock(mylock);
		WRITE_ONCE(*A, 1);
		spin_unlock(mylock);
		spin_lock(mylock);
		WRITE_ONCE(*B, 1);
		spin_unlock(mylock);
	}

	P1(int *A, int *B)
	{
		int r0;
		int r1;

		r0 = smp_load_acquire(B);
		r1 = READ_ONCE(*A);
	}

	exists (1:r0=1 /\ 1:r1=0) (* Bad outcome. *)


So I understand this one. Now unfortunately litmus doesn't support
structures, but let's suppose it could. I'm taking the previous script
and introduce a small change in P1:

	C MP+polocks

	{}

	P0(int *A, int *B, spinlock_t *mylock)
	{
		spin_lock(mylock);
		WRITE_ONCE(*A, 1);
		spin_unlock(mylock);
		spin_lock(mylock);
		WRITE_ONCE(*B, 1);
		spin_unlock(mylock);
	}

	P1(int *A, int *B)
	{
		int r0;
		int r1

		r0 = READ_ONCE(*B);
		spin_lock(r0->somelock)
		r1 = READ_ONCE(*A);
		spin_unlock(r0->somelock)
	}

	exists (1:r0=1 /\ 1:r1=0) (* Bad outcome. *)


So instead of doing a LOAD-ACQUIRE on B, I do a plain READ but I also
do a spin_lock right after on a data that depends on that READ. I can't
run that on litmus but this is the same (simplified) pattern as what we
had in this discussion and therefore I assume that it also works (ie: the
bad outcome shouldn't happen), is that right?

Would it also work if spin_lock() was just a LOAD-ACQUIRE?

Does it mean that data dependency implies sufficient ordering such that
a LOAD-ACQUIRE to a data that depends on B provides the same guarantees as
a LOAD-ACQUIRE to B?

The reason I'm asking that is because, unlike control dependency, data
dependency and its guarantees are not well documented. It is defined in
tools/memory-model/Documentation/explanation.txt but not really described
in Documentation/memory-barriers.txt. There is a mention in a scenario within
the section "MULTICOPY ATOMICITY" just to show that it's not as strong as
a full memory barrier.

So if data dependency can provide the guarantee above in my second script
but it's not as strong as a full barrier, this suggests that data dependencies
have their own specific properties that should probably be documentated.

Thanks.

Re: [patch V2 1/8] signal: Prevent exec() race

[Peter Zijlstra]

On Thu, Sep 10, 2026 at 03:21:54PM +0200, Frederic Weisbecker wrote:

> So I understand this one. Now unfortunately litmus doesn't support
> structures, but let's suppose it could. I'm taking the previous script
> and introduce a small change in P1:
> 
> 	C MP+polocks
> 
> 	{}
> 
> 	P0(int *A, int *B, spinlock_t *mylock)
> 	{
> 		spin_lock(mylock);
> 		WRITE_ONCE(*A, 1);
> 		spin_unlock(mylock);
> 		spin_lock(mylock);
> 		WRITE_ONCE(*B, 1);
> 		spin_unlock(mylock);
> 	}
> 
> 	P1(int *A, int *B)
> 	{
> 		int r0;
> 		int r1
> 
> 		r0 = READ_ONCE(*B);
> 		spin_lock(r0->somelock)
> 		r1 = READ_ONCE(*A);
> 		spin_unlock(r0->somelock)
> 	}
> 
> 	exists (1:r0=1 /\ 1:r1=0) (* Bad outcome. *)
> 
> 
> So instead of doing a LOAD-ACQUIRE on B, I do a plain READ but I also
> do a spin_lock right after on a data that depends on that READ. I can't
> run that on litmus but this is the same (simplified) pattern as what we
> had in this discussion and therefore I assume that it also works (ie: the
> bad outcome shouldn't happen), is that right?
> 
> Would it also work if spin_lock() was just a LOAD-ACQUIRE?

Yes, see below.

> Does it mean that data dependency implies sufficient ordering such that
> a LOAD-ACQUIRE to a data that depends on B provides the same guarantees as
> a LOAD-ACQUIRE to B?

The way data dependencies work in this case is a LOAD->LOAD ordering.
The LOAD-ACQUIRE that is part of LOCK must happen after the initial
load. And then the later load is constrained by the ACQUIRE.

So LOAD(B) must resolve in order to do LOAD_ACQUIRE(B->lock.value).

Re: [patch V2 3/8] posix-timers: Move posixtimer_exec_cleanup() out of exec.c

[Frederic Weisbecker]

Le Sat, Sep 05, 2026 at 08:59:10PM +0200, Thomas Gleixner a écrit :
> Move it to the POSIX timer code and provide a proper stub when POSIX timers
> are disabled in Kconfig.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 4/8] posix-timers: Move POSIX timer group exit related code out of do_exit()

[Frederic Weisbecker]

Le Sat, Sep 05, 2026 at 08:59:15PM +0200, Thomas Gleixner a écrit :
> Move the POSIX timer group exit handling into the posix timer code and
> provide a proper stub when POSIX timers are disabled in Kconfig.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 5/8] posix-cpu-timers: Move inlines out of public header

[Frederic Weisbecker]

Le Sat, Sep 05, 2026 at 08:59:19PM +0200, Thomas Gleixner a écrit :
> They are only used in the POSIX CPU timer code. No point in exposing them
> globally and parsing them for nothing.
> 
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [patch V2 1/8] signal: Prevent exec() race

[Alan Stern]

On Thu, Sep 10, 2026 at 03:21:54PM +0200, Frederic Weisbecker wrote:
> Alan, let me ask you something, because I'm the only one here puzzled by
> this data dependency.
> 
> The following scenario works (the bad outcome never happens) because
> UNLOCK+LOCK pairs with smp_load_acquire():
> 
> 	C MP+polocks
> 
> 	{}
> 
> 	P0(int *A, int *B, spinlock_t *mylock)
> 	{
> 		spin_lock(mylock);
> 		WRITE_ONCE(*A, 1);
> 		spin_unlock(mylock);
> 		spin_lock(mylock);
> 		WRITE_ONCE(*B, 1);
> 		spin_unlock(mylock);
> 	}
> 
> 	P1(int *A, int *B)
> 	{
> 		int r0;
> 		int r1;
> 
> 		r0 = smp_load_acquire(B);
> 		r1 = READ_ONCE(*A);
> 	}
> 
> 	exists (1:r0=1 /\ 1:r1=0) (* Bad outcome. *)
> 
> 
> So I understand this one. Now unfortunately litmus doesn't support
> structures, but let's suppose it could. I'm taking the previous script
> and introduce a small change in P1:
> 
> 	C MP+polocks
> 
> 	{}
> 
> 	P0(int *A, int *B, spinlock_t *mylock)
> 	{
> 		spin_lock(mylock);
> 		WRITE_ONCE(*A, 1);
> 		spin_unlock(mylock);
> 		spin_lock(mylock);
> 		WRITE_ONCE(*B, 1);
> 		spin_unlock(mylock);
> 	}
> 
> 	P1(int *A, int *B)
> 	{
> 		int r0;
> 		int r1
> 
> 		r0 = READ_ONCE(*B);
> 		spin_lock(r0->somelock)
> 		r1 = READ_ONCE(*A);
> 		spin_unlock(r0->somelock)
> 	}
> 
> 	exists (1:r0=1 /\ 1:r1=0) (* Bad outcome. *)
> 
> 
> So instead of doing a LOAD-ACQUIRE on B, I do a plain READ but I also
> do a spin_lock right after on a data that depends on that READ. I can't
> run that on litmus but this is the same (simplified) pattern as what we
> had in this discussion and therefore I assume that it also works (ie: the
> bad outcome shouldn't happen), is that right?

Yes.

> Would it also work if spin_lock() was just a LOAD-ACQUIRE?

Yes.

> Does it mean that data dependency implies sufficient ordering such that
> a LOAD-ACQUIRE to a data that depends on B provides the same guarantees as
> a LOAD-ACQUIRE to B?

Indeed it does, with the obvious exception that a load-acquire of B 
also provides ordering to any statements in between it and the load of 
the data depending on B.  That is:

	r0 = smp_load_acquire(B);
	X;
	r1 = READ_ONCE(r0->A);

orders the load from B before everything that follows, including X, 
whereas:

	r0 = READ_ONCE(B);
	X;
	r1 = smp_load_acquire(r0->A);

orders the load from B before the load from r0->A and everything 
following it, but not before X.

> The reason I'm asking that is because, unlike control dependency, data
> dependency and its guarantees are not well documented. It is defined in
> tools/memory-model/Documentation/explanation.txt but not really described
> in Documentation/memory-barriers.txt. There is a mention in a scenario within
> the section "MULTICOPY ATOMICITY" just to show that it's not as strong as
> a full memory barrier.
> 
> So if data dependency can provide the guarantee above in my second script
> but it's not as strong as a full barrier, this suggests that data dependencies
> have their own specific properties that should probably be documentated.

Perhaps so.  Can you suggest a place in explanations.txt that could be 
improved?

Here's how I think about ordering guarantees in general.  Not in terms 
of pairing of memory barriers, since (as you pointed out) dependencies 
aren't memory barriers, and also since ordering cycles can involve more 
than two CPUs (so triples or higher, not just pairs).

Instead there's a hierarchy of ordering classes.  The lowest level only 
orders events on a single CPU; it includes dependencies, smp_rmb(), and 
load-acquires.

The next level orders cross-CPU events (i.e., writes), but only in a way 
that affects two CPUs at a time.  It includes things like smp_wmb() and 
store-releases, and it guarantees that if CPU 1 writes A first and B 
second, then CPU 2 will observe the store to A before it observes the 
store to B.  Likewise for CPU 3, CPU 4, etc., but there is no guarantee 
about the order in which differing CPUs will observe the stores.

The highest level orders events in a way that involves all CPUs.  It 
includes things like smp_mb() and synchronize_rcu(), and it says that if 
CPU 1 writes A first and B second, then _every_ CPU will observe the 
store to A before _any_ CPU (including CPU 1!) observes the store to B.

This is a little imprecise, and there are varying details within the 
levels, but the overall idea is basically right.

At any rate, the point you're raising is that dependencies and 
load-acquires both sit at the lowest level of this hierarchy, so they 
provide pretty much the same ordering guarantees.

Alan Stern