[PATCH] exit: fix lost-update race on the child_subreaper bitfields

[PATCH] exit: fix lost-update race on the child_subreaper bitfields

[Nickolai Zeldovich]

is_child_subreaper, has_child_subreaper and autoreap are 1-bit fields
that share one word of struct signal_struct.  A store to any of them is
a read-modify-write of the whole word, and two of the stores run
without a common lock:

  - PR_SET_CHILD_SUBREAPER writes me->signal->is_child_subreaper with
    no lock held.

  - propagate_has_child_subreaper(), run by walk_process_tree() from
    the same prctl in an ancestor, writes p->signal->has_child_subreaper
    of every descendant p under read_lock(&tasklist_lock), which does
    not exclude the unlocked writer.

When a process calls prctl(PR_SET_CHILD_SUBREAPER, 1) while an
ancestor's PR_SET_CHILD_SUBREAPER walk visits it, the two
read-modify-write sequences interleave and one of the bits is silently
lost:

  - If has_child_subreaper is lost, the process's orphaned descendants
    are reparented straight to init when it exits: find_new_reaper()
    tests father->signal->has_child_subreaper first and never looks for
    the ancestor subreaper.  The service manager that registered as the
    subreaper never gets SIGCHLD for them and cannot wait() on them.

  - If is_child_subreaper is lost, the prctl returned 0 but the process
    is not a subreaper: orphans of its descendants skip it and are
    reparented to the next subreaper up or to init.
    PR_GET_CHILD_SUBREAPER reads back 0.

Documentation/memory-barriers.txt requires all fields of one bitfield
to be protected by one lock for exactly this reason.  No lock is taken
or documented for these flags, and the prctl needs no privilege, so any
process can hit the race against its own ancestors or descendants.

Make the three flags separate bool members.  Each store then has its
own memory location and no read-modify-write of a neighbour.  The
readers and their locking are unchanged.  autoreap is only written to
a not yet published signal_struct in copy_process() and is not racy by
itself, but it shares the word and is converted with the other two.

Fixes: 749860ce2427 ("prctl: propagate has_child_subreaper flag to every descendant")
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
 include/linux/sched/signal.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index d45a5476b..f7266dd2e 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -130,9 +130,9 @@ struct signal_struct {
 	 * process will inherit a flag if they should look for a
 	 * child_subreaper process at exit.
 	 */
-	unsigned int		is_child_subreaper:1;
-	unsigned int		has_child_subreaper:1;
-	unsigned int		autoreap:1;
+	bool			is_child_subreaper;
+	bool			has_child_subreaper;
+	bool			autoreap;
 
 #ifdef CONFIG_POSIX_TIMERS
 

base-commit: fc1c25014ff8e416d75e8a5ba3b9c0ba5eb8e877
-- 
2.56.0

Re: [PATCH] exit: fix lost-update race on the child_subreaper bitfields

[Oleg Nesterov]

On 10/09, Nickolai Zeldovich wrote:
>
> is_child_subreaper, has_child_subreaper and autoreap are 1-bit fields
> that share one word of struct signal_struct.  A store to any of them is
> a read-modify-write of the whole word, and two of the stores run
> without a common lock:

Yes, thanks, this is obviously wrong. But I am a bit confused...

> autoreap is only written to
> a not yet published signal_struct in copy_process() and is not racy by
> itself,

Yes,

> but it shares the word and is converted with the other two.

So why? If you turn the *_child_subreaper members to bool, then
autoreap:1 is fine?

Oleg.

> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -130,9 +130,9 @@ struct signal_struct {
>  	 * process will inherit a flag if they should look for a
>  	 * child_subreaper process at exit.
>  	 */
> -	unsigned int		is_child_subreaper:1;
> -	unsigned int		has_child_subreaper:1;
> -	unsigned int		autoreap:1;
> +	bool			is_child_subreaper;
> +	bool			has_child_subreaper;
> +	bool			autoreap;

Re: [PATCH] exit: fix lost-update race on the child_subreaper bitfields

[Nickolai Zeldovich]

On Sun, Oct 11, 2026 at 8:09 AM Oleg Nesterov <oleg@redhat.com> wrote:
> On 10/09, Nickolai Zeldovich wrote:
> ...
> > but it shares the word and is converted with the other two.
>
> So why? If you turn the *_child_subreaper members to bool, then
> autoreap:1 is fine?

You're right, autoreap can stay a bitfield.  (I somehow missed the
fact that there's one more bitfield below under CONFIG_POSIX_TIMERS.)
With autoreap as a bitfield, it can then share memory with
timer_create_restore_ids:1 (CONFIG_POSIX_TIMERS=y), and that pair is
race-free: autoreap is stored exactly once, in copy_process() before
the new signal_struct is reachable by anyone, and
timer_create_restore_ids is only stored to current->signal (prctl) or
tsk->signal on exec, i.e. never to a struct that can still receive the
autoreap store.

Will send the v2 patch shortly.

Thanks,

Nickolai.

[PATCH v2] exit: fix lost-update race on the child_subreaper bitfields

[Nickolai Zeldovich]

is_child_subreaper, has_child_subreaper, autoreap and, with
CONFIG_POSIX_TIMERS=y, timer_create_restore_ids are 1-bit fields that
share one memory location in struct signal_struct.  A store to any of
them is a read-modify-write of the whole word, and three of the stores
run without a common lock:

  - PR_SET_CHILD_SUBREAPER writes me->signal->is_child_subreaper with
    no lock held.

  - propagate_has_child_subreaper(), run by walk_process_tree() from
    the same prctl in an ancestor, writes p->signal->has_child_subreaper
    of every descendant p under read_lock(&tasklist_lock), which does
    not exclude the other writers.

  - PR_TIMER_CREATE_RESTORE_IDS writes
    current->signal->timer_create_restore_ids with no lock held.

When a process calls prctl(PR_SET_CHILD_SUBREAPER, 1) while an
ancestor's PR_SET_CHILD_SUBREAPER walk visits it, or while another
thread of the same process calls prctl(PR_TIMER_CREATE_RESTORE_IDS),
the read-modify-write sequences interleave and one of the bits is
silently lost:

  - If has_child_subreaper is lost, the process's orphaned descendants
    are reparented straight to init when it exits: find_new_reaper()
    tests father->signal->has_child_subreaper first and never looks for
    the ancestor subreaper.  The service manager that registered as the
    subreaper never gets SIGCHLD for them and cannot wait() on them.

  - If is_child_subreaper is lost, the prctl returned 0 but the process
    is not a subreaper: orphans of its descendants skip it and are
    reparented to the next subreaper up or to init.
    PR_GET_CHILD_SUBREAPER reads back 0.

  - If timer_create_restore_ids is lost, the prctl returned 0 but
    timer_create() does not restore the requested timer ids (or keeps
    restoring them after _OFF), and _GET reads back the old value.

Documentation/memory-barriers.txt requires all fields of one bitfield
to be protected by one lock for exactly this reason.  No lock is taken
or documented for these flags, and the prctls need no privilege, so any
process can hit the race against its own ancestors or descendants, or
between two of its own threads.

Make is_child_subreaper and has_child_subreaper separate bool members.
Each store then has its own memory location and no read-modify-write
of a neighbour.  The readers and their locking are unchanged.

autoreap stays a bit-field and so shares a memory location only with
timer_create_restore_ids.  That pair is race-free: autoreap is stored
exactly once, in copy_process() before the new signal_struct is
reachable by any other task, and timer_create_restore_ids is only
stored to the signal_struct of an already running process.

Fixes: 749860ce2427 ("prctl: propagate has_child_subreaper flag to every descendant")
Assisted-by: LLM
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
Changes in v2:
  - keep autoreap as a bit-field (Oleg); it is only stored before the
    signal_struct is published, so sharing a memory location with
    timer_create_restore_ids is fine.
  - mention timer_create_restore_ids, which shares the memory location
    with the subreaper bits today and is a third unlocked writer.
v1: https://lore.kernel.org/lkml/20261009222404.760787-1-nickolai@csail.mit.edu/

 include/linux/sched/signal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index d45a5476b97de..efa1e2d6c7a6b 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -130,8 +130,8 @@ struct signal_struct {
 	 * process will inherit a flag if they should look for a
 	 * child_subreaper process at exit.
 	 */
-	unsigned int		is_child_subreaper:1;
-	unsigned int		has_child_subreaper:1;
+	bool			is_child_subreaper;
+	bool			has_child_subreaper;
 	unsigned int		autoreap:1;
 
 #ifdef CONFIG_POSIX_TIMERS

base-commit: fc1c25014ff8e416d75e8a5ba3b9c0ba5eb8e877
-- 
2.55.0

Re: [PATCH v2] exit: fix lost-update race on the child_subreaper bitfields

[Oleg Nesterov]

On 10/11, Nickolai Zeldovich wrote:
>
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -130,8 +130,8 @@ struct signal_struct {
>  	 * process will inherit a flag if they should look for a
>  	 * child_subreaper process at exit.
>  	 */
> -	unsigned int		is_child_subreaper:1;
> -	unsigned int		has_child_subreaper:1;
> +	bool			is_child_subreaper;
> +	bool			has_child_subreaper;
>  	unsigned int		autoreap:1;

Thanks,

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

perhaps it makes sense to add something like signal_struct::atomic_flags
later...

Oleg.

Re: [PATCH v2] exit: fix lost-update race on the child_subreaper bitfields

[Christian Brauner]

On Sun, 11 Oct 2026 08:34:39 -0400, Nickolai Zeldovich wrote:
> is_child_subreaper, has_child_subreaper, autoreap and, with
> CONFIG_POSIX_TIMERS=y, timer_create_restore_ids are 1-bit fields that
> share one memory location in struct signal_struct.  A store to any of
> them is a read-modify-write of the whole word, and three of the stores
> run without a common lock:
> 
>   - PR_SET_CHILD_SUBREAPER writes me->signal->is_child_subreaper with
>     no lock held.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] exit: fix lost-update race on the child_subreaper bitfields
      https://git.kernel.org/vfs/vfs/c/56d5dcb2d6d5