[to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Andrew Morton]

The quilt patch titled
     Subject: taskstats: copy signal->stats under siglock in taskstats_exit
has been removed from the -mm tree.  Its filename was
     taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch

This patch was dropped because an updated version will be issued

------------------------------------------------------
From: Bradley Morgan <brads@mainlining.org>
Subject: taskstats: copy signal->stats under siglock in taskstats_exit
Date: Tue, 4 Aug 2026 09:34:00 +0000

taskstats_exit() copies tsk->signal->stats into the exit reply without
taking any lock.  Every other writer of this struct holds sighand->siglock
before touching it, and this copy does not.

The copy happens on the last thread of a thread group that exits. 
group_dead being 1 only says that every thread has dropped signal->live,
it does not say how far the other threads got in do_exit().  One of them
can still be inside fill_tgid_exit() adding its counters to the struct
while the last thread copies it out, so the copy can read the struct in
the middle of an update.

The commit that added the copy assumed no locking was needed because the
group was dead:

	/* No locking needed for tsk->signal->stats since
	   group is dead */

but at that point the other threads have not necessarily finished
their exit path.

cpu0 (thread A, not last)        cpu1 (thread B, last)
===========================      ==============================
atomic_dec(&signal->live)        atomic_dec(&signal->live) -> 0
group_dead = 0                   group_dead = 1
...                              taskstats_exit(tsk, 1)
taskstats_exit(tsk, 0)           fill_tgid_exit(tsk) [siglock]
fill_tgid_exit(tsk)              memcpy(stats, signal->stats)
  spin_lock(siglock)               reads ac_utime (new)
  stats->ac_utime += x             reads ac_stime (old)
  stats->ac_stime += y             torn snapshot -> netlink
  spin_unlock(siglock)

The listeners receive a partially updated tgid snapshot, with some fields
from before the concurrent update and some from after.  There is no crash
or splat, which is likely why this went unnoticed since 2006.  A userspace
model of the same shape, writer under a lock and a lockless memcpy reader,
produces millions of torn reads in a few seconds.

Take siglock around the copy like every other access does.  sighand is
still alive here because taskstats_exit() runs before exit_notify(), and
fill_tgid_exit() already takes this same lock earlier in this function.

Link: https://lore.kernel.org/20260804093400.3922-1-include@grrlz.net
Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")
Signed-off-by: Bradley Morgan <brads@mainlining.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Balbir Singh <bsingharora@gmail.com>
---

 kernel/taskstats.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/kernel/taskstats.c~taskstats-copy-signal-stats-under-siglock-in-taskstats_exit
+++ a/kernel/taskstats.c
@@ -590,6 +590,7 @@ void taskstats_exit(struct task_struct *
 	struct sk_buff *rep_skb;
 	size_t size;
 	int is_thread_group;
+	unsigned long flags;
 
 	if (!family_registered)
 		return;
@@ -635,7 +636,10 @@ void taskstats_exit(struct task_struct *
 	if (!stats)
 		goto err;
 
+	/* This was racy before, copy the stats under siglock. */
+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
 	memcpy(stats, tsk->signal->stats, sizeof(*stats));
+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 	stats->version = TASKSTATS_VERSION;
 
 send:
_

Patches currently in -mm which might be from brads@mainlining.org are

mailmap-update-email-address-for-bradley-morgan.patch
taskstats-drop-the-unused-cpu_dont_care-enum-member.patch
panic-fix-redirect-cpu-race-in-panic_try_force_cpu.patch
panic-flatten-nmi_panic-control-flow.patch
panic-fix-va_list-reuse-in-panic_try_force_cpu.patch
panic-restore-variable-arguments-to-nmi_panic.patch
panic-allow-force_cpu-redirect-from-an-nmi.patch
panic-kill-the-buffer-unavailable-redirect-fallback.patch
watchdog-perf-one-digit-too-short-in-the-raw-event-config-copy.patch

Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Bradley Morgan]

On 7 October 2026 02:21:08 BST, Andrew Morton <akpm@linux-foundation.org>
wrote:
>
>The quilt patch titled
>     Subject: taskstats: copy signal->stats under siglock in taskstats_exit
>has been removed from the -mm tree.  Its filename was
>     taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch
>
>This patch was dropped because an updated version will be issued


Any reason? I didn't get let known about this! :)

>
>------------------------------------------------------
>From: Bradley Morgan <brads@mainlining.org>
>Subject: taskstats: copy signal->stats under siglock in taskstats_exit
>Date: Tue, 4 Aug 2026 09:34:00 +0000
>
>taskstats_exit() copies tsk->signal->stats into the exit reply without
>taking any lock.  Every other writer of this struct holds sighand->siglock
>before touching it, and this copy does not.
>
>The copy happens on the last thread of a thread group that exits. 
>group_dead being 1 only says that every thread has dropped signal->live,
>it does not say how far the other threads got in do_exit().  One of them
>can still be inside fill_tgid_exit() adding its counters to the struct
>while the last thread copies it out, so the copy can read the struct in
>the middle of an update.
>
>The commit that added the copy assumed no locking was needed because the
>group was dead:
>
>	/* No locking needed for tsk->signal->stats since
>	   group is dead */
>
>but at that point the other threads have not necessarily finished
>their exit path.
>
>cpu0 (thread A, not last)        cpu1 (thread B, last)
>===========================      ==============================
>atomic_dec(&signal->live)        atomic_dec(&signal->live) -> 0
>group_dead = 0                   group_dead = 1
>...                              taskstats_exit(tsk, 1)
>taskstats_exit(tsk, 0)           fill_tgid_exit(tsk) [siglock]
>fill_tgid_exit(tsk)              memcpy(stats, signal->stats)
>  spin_lock(siglock)               reads ac_utime (new)
>  stats->ac_utime += x             reads ac_stime (old)
>  stats->ac_stime += y             torn snapshot -> netlink
>  spin_unlock(siglock)
>
>The listeners receive a partially updated tgid snapshot, with some fields
>from before the concurrent update and some from after.  There is no crash
>or splat, which is likely why this went unnoticed since 2006.  A userspace
>model of the same shape, writer under a lock and a lockless memcpy reader,
>produces millions of torn reads in a few seconds.
>
>Take siglock around the copy like every other access does.  sighand is
>still alive here because taskstats_exit() runs before exit_notify(), and
>fill_tgid_exit() already takes this same lock earlier in this function.
>
>Link: https://lore.kernel.org/20260804093400.3922-1-include@grrlz.net
>Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")
>Signed-off-by: Bradley Morgan <brads@mainlining.org>
>Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>Acked-by: Oleg Nesterov <oleg@redhat.com>
>Cc: Balbir Singh <bsingharora@gmail.com>
>---
>
> kernel/taskstats.c |    4 ++++
> 1 file changed, 4 insertions(+)
>
>--- a/kernel/taskstats.c~taskstats-copy-signal-stats-under-siglock-in-taskstats_exit
>+++ a/kernel/taskstats.c
>@@ -590,6 +590,7 @@ void taskstats_exit(struct task_struct *
> 	struct sk_buff *rep_skb;
> 	size_t size;
> 	int is_thread_group;
>+	unsigned long flags;
> 
> 	if (!family_registered)
> 		return;
>@@ -635,7 +636,10 @@ void taskstats_exit(struct task_struct *
> 	if (!stats)
> 		goto err;
> 
>+	/* This was racy before, copy the stats under siglock. */
>+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
> 	memcpy(stats, tsk->signal->stats, sizeof(*stats));
>+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> 	stats->version = TASKSTATS_VERSION;
> 
> send:
>_
>
>Patches currently in -mm which might be from brads@mainlining.org are
>
>mailmap-update-email-address-for-bradley-morgan.patch
>taskstats-drop-the-unused-cpu_dont_care-enum-member.patch
>panic-fix-redirect-cpu-race-in-panic_try_force_cpu.patch
>panic-flatten-nmi_panic-control-flow.patch
>panic-fix-va_list-reuse-in-panic_try_force_cpu.patch
>panic-restore-variable-arguments-to-nmi_panic.patch
>panic-allow-force_cpu-redirect-from-an-nmi.patch
>panic-kill-the-buffer-unavailable-redirect-fallback.patch
>watchdog-perf-one-digit-too-short-in-the-raw-event-config-copy.patch
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds

Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Andrew Morton]

On Wed, 07 Oct 2026 07:39:03 +0100 Bradley Morgan <brads@mainlining.org> wrote:

> On 7 October 2026 02:21:08 BST, Andrew Morton <akpm@linux-foundation.org>
> wrote:
> >
> >The quilt patch titled
> >     Subject: taskstats: copy signal->stats under siglock in taskstats_exit
> >has been removed from the -mm tree.  Its filename was
> >     taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch
> >
> >This patch was dropped because an updated version will be issued
> 
> 
> Any reason? I didn't get let known about this! :)

That's what happens when you change email addresses :(



Begin forwarded message:

Date: Tue, 6 Oct 2026 18:20:53 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Bradley Morgan <include@grrlz.net>
Subject: Re: [PATCH] taskstats: copy signal->stats under siglock in taskstats_exit


On Tue,  4 Aug 2026 09:34:00 +0000 Bradley Morgan <include@grrlz.net> wrote:

> taskstats_exit() copies tsk->signal->stats into the exit reply
> without taking any lock. Every other writer of this struct holds
> sighand->siglock before touching it, and this copy does not.
> 
> The copy happens on the last thread of a thread group that exits.
> group_dead being 1 only says that every thread has dropped
> signal->live, it does not say how far the other threads got in
> do_exit(). One of them can still be inside fill_tgid_exit() adding
> its counters to the struct while the last thread copies it out, so
> the copy can read the struct in the middle of an update.

I managed to lose the v2 of this.  Please resend sometime?  Add
a cc:oleg while in there.

Thanks.

Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Bradley Morgan]

On 7 October 2026 18:12:42 BST, Andrew Morton <akpm@linux-foundation.org>
wrote:
>On Wed, 07 Oct 2026 07:39:03 +0100 Bradley Morgan <brads@mainlining.org>
>wrote:
>
>> On 7 October 2026 02:21:08 BST, Andrew Morton
><akpm@linux-foundation.org>
>> wrote:
>> >
>> >The quilt patch titled
>> >     Subject: taskstats: copy signal->stats under siglock in
>taskstats_exit
>> >has been removed from the -mm tree.  Its filename was
>> >     taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch
>> >
>> >This patch was dropped because an updated version will be issued
>> 
>> 
>> Any reason? I didn't get let known about this! :)
>
>That's what happens when you change email addresses :(
>
>
>
>Begin forwarded message:
>
>Date: Tue, 6 Oct 2026 18:20:53 -0700
>From: Andrew Morton <akpm@linux-foundation.org>
>To: Bradley Morgan <include@grrlz.net>
>Subject: Re: [PATCH] taskstats: copy signal->stats under siglock in taskstats_exit
>
>
>On Tue,  4 Aug 2026 09:34:00 +0000 Bradley Morgan <include@grrlz.net>
>wrote:
>
>> taskstats_exit() copies tsk->signal->stats into the exit reply
>> without taking any lock. Every other writer of this struct holds
>> sighand->siglock before touching it, and this copy does not.
>> 
>> The copy happens on the last thread of a thread group that exits.
>> group_dead being 1 only says that every thread has dropped
>> signal->live, it does not say how far the other threads got in
>> do_exit(). One of them can still be inside fill_tgid_exit() adding
>> its counters to the struct while the last thread copies it out, so
>> the copy can read the struct in the middle of an update.
>
>I managed to lose the v2 of this.  Please resend sometime?  Add
>a cc:oleg while in there.
>
>Thanks.
>
From 137b873cce7721384c9d24f4c0864bb002c8d039 Mon Sep 17 00:00:00 2001
From: Bradley Morgan <include@grrlz.net>
Date: Tue, 4 Aug 2026 09:34:00 +0000
Subject: [PATCH v2] taskstats: copy signal->stats under siglock in
 taskstats_exit

taskstats_exit() copies tsk->signal->stats into the exit reply
without taking any lock. Every other writer of this struct holds
sighand->siglock before touching it, and this copy does not.

The copy happens on the last thread of a thread group that exits.
group_dead being 1 only says that every thread has dropped
signal->live, it does not say how far the other threads got in
do_exit(). One of them can still be inside fill_tgid_exit() adding
its counters to the struct while the last thread copies it out, so
the copy can read the struct in the middle of an update.

The commit that added the copy assumed no locking was needed because
the group was dead:

	/* No locking needed for tsk->signal->stats since
	   group is dead */

but at that point the other threads have not necessarily finished
their exit path.

cpu0 (thread A, not last)        cpu1 (thread B, last)
===========================      ==============================
atomic_dec(&signal->live)        atomic_dec(&signal->live) -> 0
group_dead = 0                   group_dead = 1
...                              taskstats_exit(tsk, 1)
taskstats_exit(tsk, 0)           fill_tgid_exit(tsk) [siglock]
fill_tgid_exit(tsk)              memcpy(stats, signal->stats)
  spin_lock(siglock)               reads ac_utime (new)
  stats->ac_utime += x             reads ac_stime (old)
  stats->ac_stime += y             torn snapshot -> netlink
  spin_unlock(siglock)

The listeners receive a partially updated tgid snapshot, with some
fields from before the concurrent update and some from after. There
is no crash or splat, which is likely why this went unnoticed since
2006. A userspace model of the same shape, writer under a lock and
a lockless memcpy reader, produces millions of torn reads in a few
seconds.

Take siglock around the copy like every other access does. sighand
is still alive here because taskstats_exit() runs before
exit_notify(), and fill_tgid_exit() already takes this same lock
earlier in this function.

Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")
Signed-off-by: Bradley Morgan <include@grrlz.net>

The only caller is do_exit(), which runs with irqs enabled, and
fill_tgid_exit() already takes the same lock with spin_lock_irq().
Use that instead of spin_lock_irqsave(). (Oleg)

---
Changes since V1:
- spin_lock_irq() instead of spin_lock_irqsave() (Oleg)
---
 kernel/taskstats.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index f31df72f0e9d..300fa06ac84a 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -635,7 +635,10 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
 	if (!stats)
 		goto err;
 
+	/* This was racy before, copy the stats under siglock. */
+	spin_lock_irq(&tsk->sighand->siglock);
 	memcpy(stats, tsk->signal->stats, sizeof(*stats));
+	spin_unlock_irq(&tsk->sighand->siglock);
 	stats->version = TASKSTATS_VERSION;
 
 send:
-- 
2.47.3


https://lore.kernel.org/all/5727577F-39B4-43AF-8E6B-003E9A980A12@grrlz.net/
--- Thanks!
"I'm not a very positive person" - Linus torvalds

Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Andrew Morton]

On Wed, 07 Oct 2026 18:19:50 +0100 Bradley Morgan <brads@mainlining.org> wrote:

> >
> >From 137b873cce7721384c9d24f4c0864bb002c8d039 Mon Sep 17 00:00:00 2001
> From: Bradley Morgan <include@grrlz.net>
> Date: Tue, 4 Aug 2026 09:34:00 +0000
> Subject: [PATCH v2] taskstats: copy signal->stats under siglock in
>  taskstats_exit

Thanks.  Please send as a standalone email and cc linux-kernel?  So we
get a nice Link: in the permanent record.  (mm-commits Links: work
though).

> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -635,7 +635,10 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
>  	if (!stats)
>  		goto err;
>  
> +	/* This was racy before, copy the stats under siglock. */

That's not a great comment - we're really not interested in what is
essentially embedded changelogging.

A good comment would explain *why* we're taking the lock.  Briefly
describe what we're excluding against.

> +	spin_lock_irq(&tsk->sighand->siglock);
>  	memcpy(stats, tsk->signal->stats, sizeof(*stats));
> +	spin_unlock_irq(&tsk->sighand->siglock);
>  	stats->version = TASKSTATS_VERSION;

Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Oleg Nesterov]

On 10/07, Bradley Morgan wrote:
>
> Changes since V1:
> - spin_lock_irq() instead of spin_lock_irqsave() (Oleg)

I have already reviewed this version... Let me quote
https://lore.kernel.org/all/anMC3eRoRdaST0Se@redhat.com/

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

	However. I won't blame your fix, but I am not sure it actually fixes the
	real problem.

	In the scenario above, if thread B takes ->siglock first (before thread A)
	it will report TASKSTATS_TYPE_TGID/AGROUP without accounting thread A.
	Looks "obviously wrong" but I forgot everything about taskstats.

	And OTOH... for_each_thread() in fill_stats_for_tgid() can't rely on the
	tsk->exit_state check, the same thread can be accounted twice.

So perhaps the changelog should mention that this patch doesn't fix all the
related problems? Or may be you can try to make V3 which fixes them all?

> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -635,7 +635,10 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
>  	if (!stats)
>  		goto err;
>
> +	/* This was racy before, copy the stats under siglock. */

Well, this comment looks strange to me. I'd suggest to drop it.

> +	spin_lock_irq(&tsk->sighand->siglock);
>  	memcpy(stats, tsk->signal->stats, sizeof(*stats));
> +	spin_unlock_irq(&tsk->sighand->siglock);
>  	stats->version = TASKSTATS_VERSION;

Oleg.