futex: scheduling-while-atomic because nested vfork can break guard(private_hash)

Introduction
============
The futex private hash implementation was written with the assumption that any MM used by more than one running task either has a private hash or has irrevocably opted out of having a private hash; but this isn't actually the case.

Commit ee9dce44362b ("futex: Drop CLONE_THREAD requirement for private default hash alloc") tried to fix cases where processes can share an MM without triggering private hash allocation, but overlooked that nested vfork can also result in concurrently running processes sharing an MM. After that was discovered, commit bde023808364 ("futex: Fix race on the initial mm->futex.phash.ref allocation") instead changed futex_hash_allocate() to not rely on this assumption anymore; but other places in the futex code still make this assumption.

Issue description
============
exit_pi_state_list() contains this code to handle waiters on PI futexes held by the current task, which is exiting or going through execve:

```
 /*
* Ensure the hash remains stable (no resize) during the while loop
* below. The hb pointer is acquired under the pi_lock so we can't block
* on the mutex.
*/
[...]
guard(private_hash)(current->mm);
[...]
raw_spin_lock_irq(&curr->pi_lock);
while (!list_empty(head)) {
  [...]
  if (1) {
    CLASS(hbr, hbr)(&key);
```

CLASS(hbr, hbr)(&key) calls futex_hash(), which (if the MM is in the middle of switching between two futex_private_hash instances) can call futex_pivot_hash(), which can block on scoped_guard(mutex, &mm->futex.phash.lock). guard(private_hash) is supposed to have taken a reference on the futex_private_hash, which would prevent switching to another futex_private_hash instance; but guard(private_hash) will have been a no-op if the MM didn't have a futex_private_hash yet at that time.

So it is possible to race as follows:

First, use nested vfork to create three processes P1, P2, and P3 that share one MM and can run concurrently. That can be done like this:

- P1 vforks to create P2'
- P2' vforks to create P2
- P2 sends SIGKILL to P2' to unblock P1
- P1 vforks to create P3'
- P3' vforks to create P3
- P3 sends SIGKILL to P3' to unblock P1

Then race like this (view this in monospace):

P1         P2                   P3          RCU
==         ==                   ==          ===
           futex(FUTEX_LOCK_PI|FUTEX_PRIVATE_FLAG)
             [success]
                                futex(FUTEX_LOCK_PI|FUTEX_PRIVATE_FLAG)
                                  [blocks]
           do_exit
             exit_mm
               mm_exit_exec_release
                 futex_exit_exec_release
                   futex_cleanup
                     exit_pi_state_list
                       guard(private_hash)(current->mm)
futex_hash_prctl(PR_FUTEX_HASH, PR_FUTEX_HASH_SET_SLOTS)
futex_hash_prctl(PR_FUTEX_HASH, PR_FUTEX_HASH_SET_SLOTS)
  futex_hash_allocate
    scoped_guard(mutex, &mm->futex.phash.lock)
    futex_ref_drop
      __futex_ref_atomic_begin
        call_rcu_hurry(..., futex_ref_rcu)
                                            futex_ref_rcu
                                              __futex_ref_atomic_end
                       raw_spin_lock_irq(&curr->pi_lock)
                       CLASS(hbr, hbr)(&key)
                         futex_hash
                           futex_private_hash_get [fails]
                           futex_pivot_hash
                             scoped_guard(mutex, &mm->futex.phash.lock)
                               *** scheduling-while-atomic ***
    __futex_pivot_hash

Impact, related issues
==========
futex_wait_multiple_setup() follows the same pattern of using guard(private_hash)(current->mm) to ensure that a later CLASS(hbr, hbr)(&q->key) won't sleep and is probably affected by the same issue.

__futex_unlock_pi() and requeue_pi_wake_futex() also use futex_private_hash(), but there the race seems benign.

This issue also means that different futex users could disagree about the hash bucket that a private futex belongs in (older futex_hash() calls returning a pointer into the global hash, newer futex_hash() calls returning a pointer into the private hash). For most users of CLASS(hbr, hbr)(...), that probably just leads to futex wake/wait operations on the same userspace address not interacting with each other, without affecting the kernel. But an interesting aspect of exit_pi_state_list() is that it does spin_lock(&hb->lock) further down without any visible access to the corresponding hash bucket; I can't tell if that actually protects against something, but if yes, this locking might be broken if the corresponding locking in another task is holding the hb->lock of a global hash entry instead?

Sidenote
========
Maybe we should block CLONE_VFORK when current->vfork_done!=NULL ? It is clearly bad to allow nested vfork() such that a SIGKILL can cause two threads to run concurrently on the same userspace stack.

But I'm not sure if that's a good fix for these futex issues - a fix that doesn't rely on such distant assumptions might be nicer...

Reproducer
==========
Tested at mainline commit 50d05c7c76c96b90462f24debacca971d2e86713, with a build with options:

CONFIG_PREEMPT=y
CONFIG_RCU_STRICT_GRACE_PERIOD=y
CONFIG_DEBUG_ATOMIC_SLEEP=y

To test, patch delays into the kernel like this (also attached as kernel-delay-patch.diff):

```
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index a061f54b606d..94ebd2c25b2a 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -47,6 +47,7 @@
 #include <linux/vmalloc.h>
 #include <linux/kmemleak.h>
 #include <linux/wait_bit.h>
+#include <linux/delay.h>

 #include <vdso/futex.h>

@@ -1359,6 +1360,7 @@ static void exit_pi_state_list(struct task_struct *curr)
         */
        WARN_ON(curr != current);
        guard(private_hash)(current->mm);
+       if (strcmp(current->comm, "SLOW-1") == 0) mdelay(1000);
        /*
         * We are a ZOMBIE and nobody can enqueue itself on
         * pi_state_list anymore, but we have to be careful
@@ -1945,6 +1947,7 @@ static int futex_hash_allocate(unsigned int
hash_slots, unsigned int flags)
                                 * reference on the existing hash.
                                 */
                                futex_ref_drop(cur);
+                               if (strcmp(current->comm, "SLOW-2") ==
0) mdelay(10000);
                        }

                        if (new) {
```

And run the following testcase:

```
#define _GNU_SOURCE
#include <err.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/prctl.h>
#include <sys/syscall.h>
#include <linux/futex.h>

static void *alloc_stack(void) {
  size_t stack_size = 1024*1024;
  void *p = mmap(NULL, stack_size, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
  if (p == MAP_FAILED)
    err(1, "stack mmap");
  if (madvise(p, 0x1000, MADV_GUARD_INSTALL))
    err(1, "stack madvise");
  return p + stack_size;
}

static pid_t child1, child1b;

static unsigned int futex_var;

int child_fn2(void *arg) {
  printf("child_fn2 entry, will kill %d\n", child1);

  syscall(__NR_futex, &futex_var, FUTEX_LOCK_PI|FUTEX_PRIVATE_FLAG, 0, NULL);
  if (kill(child1, SIGKILL))
    err(1, "kill");

  usleep(100000);
  prctl(PR_SET_NAME, "SLOW-1", 0, 0, 0);
  exit(0);
}
int child_fn1(void *arg) {
  child1 = getpid();
  if (clone(child_fn2, alloc_stack(),
CLONE_VM|CLONE_VFORK|CLONE_PARENT|SIGCHLD, NULL) == -1)
    err(1, "second clone");
  errx(1, "child_fn1 running after clone");
}

int child_fn2b(void *arg) {
  printf("child_fn2b entry, will kill %d\n", child1b);
  if (kill(child1b, SIGKILL))
    err(1, "kill");

  // this will block until the bug has been hit
  syscall(__NR_futex, &futex_var, FUTEX_LOCK_PI|FUTEX_PRIVATE_FLAG, 0, NULL);
  exit(0);
}

int child_fn1b(void *arg) {
  child1b = getpid();
  if (clone(child_fn2b, alloc_stack(),
CLONE_VM|CLONE_VFORK|CLONE_PARENT|SIGCHLD, NULL) == -1)
    err(1, "second clone");
  errx(1, "child_fn1b running after clone");
}

int main(void) {
  setbuf(stdout, NULL);
  if (clone(child_fn1, alloc_stack(), CLONE_VM|CLONE_VFORK|SIGCHLD, NULL) == -1)
    err(1, "first clone");
  if (clone(child_fn1b, alloc_stack(), CLONE_VM|CLONE_VFORK|SIGCHLD,
NULL) == -1)
    err(1, "first clone");
  printf("parent running after clone\n");
  usleep(500000);
  prctl(PR_FUTEX_HASH, PR_FUTEX_HASH_SET_SLOTS, 4, 0, 0);
  prctl(PR_SET_NAME, "SLOW-2", 0, 0, 0);
  prctl(PR_FUTEX_HASH, PR_FUTEX_HASH_SET_SLOTS, 8, 0, 0);
  return 0;
}
```

This gives me a "sleeping function called from invalid context" warning, a "Invalid wait context" warning, and a scheduling-while-atomic BUG:

```
sh-5.3# /host/h/test/nested_vfork/nested_vfork
child_fn2 entry, will kill 82
[...]
child_fn2b entry, will kill 84
parent running after clone
 BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:623
 in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 83, name: SLOW-1
 preempt_count: 1, expected: 0
 RCU nest depth: 0, expected: 0
 locks held by SLOW-1/83: 2, last CPU#0:
  #0: ffff888101e73100 (&tsk->futex.exit_mutex){+.+.}-{4:4}, at:
futex_exit_exec_release+0x27/0x160
  #1: ffff888101e726f0 (&p->pi_lock){-.-.}-{2:2}, at:
exit_pi_state_list+0xd7/0x5a0
 irq event stamp: 3306
 hardirqs last  enabled at (3305): [<ffffffff838780bb>] __schedule+0x13eb/0x18d0
 hardirqs last disabled at (3306): [<ffffffff83886c47>]
_raw_spin_lock_irq+0x17/0x50
 softirqs last  enabled at (3294): [<ffffffff814b251d>]
__irq_exit_rcu+0xad/0x180
 softirqs last disabled at (3283): [<ffffffff814b251d>]
__irq_exit_rcu+0xad/0x180
 CPU: 0 UID: 0 PID: 83 Comm: SLOW-1 Tainted: G                 N
7.3.0-rc2-00099-g50d05c7c76c9-dirty #77 PREEMPT
 Tainted: [N]=TEST
 Call Trace:
  <TASK>
[...]
  __might_resched+0x290/0x2b0
  __might_sleep+0x54/0x90
[...]
  __mutex_lock+0x69/0x1180
[...]
  mutex_lock_nested+0x16/0x20
  futex_hash+0x2a4/0x310
[...]
  exit_pi_state_list+0x110/0x5a0
  futex_exit_exec_release+0x159/0x160
  mm_exit_exec_release+0x1e/0x1b0
  exit_mm+0x3b/0x200
  do_exit+0x43c/0x1390
  do_group_exit+0x139/0x140
[...]
  __x64_sys_exit_group+0x26/0x30
[...]
  </TASK>

 =============================
 [ BUG: Invalid wait context ]
 7.3.0-rc2-00099-g50d05c7c76c9-dirty #77 Tainted: G        W        N
 -----------------------------
 SLOW-1/83 is trying to lock:
 ffff888100098488 (&fd->phash.lock){+.+.}-{4:4}, at: futex_hash+0x2a4/0x310
 other info that might help us debug this:
 context-{5:5}
 locks held by SLOW-1/83: 2, last CPU#0:
  #0: ffff888101e73100 (&tsk->futex.exit_mutex){+.+.}-{4:4}, at:
futex_exit_exec_release+0x27/0x160
  #1: ffff888101e726f0 (&p->pi_lock){-.-.}-{2:2}, at:
exit_pi_state_list+0xd7/0x5a0
 stack backtrace:
 CPU: 0 UID: 0 PID: 83 Comm: SLOW-1 Tainted: G        W        N
7.3.0-rc2-00099-g50d05c7c76c9-dirty #77 PREEMPT
 Tainted: [W]=WARN, [N]=TEST
 Call Trace:
  <TASK>
[...]
  __lock_acquire+0xade/0x2c30
  lock_acquire+0xf1/0x210
[...]
  __mutex_lock+0xb7/0x1180
[...]
  mutex_lock_nested+0x16/0x20
  futex_hash+0x2a4/0x310
[...]
  exit_pi_state_list+0x110/0x5a0
[...]
  </TASK>
 BUG: scheduling while atomic: SLOW-1/83/0x00000002
 INFO: lockdep is turned off.
 irq event stamp: 3306
 hardirqs last  enabled at (3305): [<ffffffff838780bb>] __schedule+0x13eb/0x18d0
 hardirqs last disabled at (3306): [<ffffffff83886c47>]
_raw_spin_lock_irq+0x17/0x50
 softirqs last  enabled at (3294): [<ffffffff814b251d>]
__irq_exit_rcu+0xad/0x180
 softirqs last disabled at (3283): [<ffffffff814b251d>]
__irq_exit_rcu+0xad/0x180
 CPU: 0 UID: 0 PID: 83 Comm: SLOW-1 Tainted: G        W        N
7.3.0-rc2-00099-g50d05c7c76c9-dirty #77 PREEMPT
 Tainted: [W]=WARN, [N]=TEST
 Call Trace:
  <TASK>
  __dump_stack+0x21/0x30
  dump_stack_lvl+0x76/0xa0
  dump_stack+0x15/0x20
  __schedule_bug+0x83/0xa0
  __schedule+0x12a7/0x18d0
[...]
  schedule+0xf9/0x160
[...]
  schedule_preempt_disabled+0x11/0x20
  __mutex_lock+0x2ba/0x1180
[...]
  mutex_lock_nested+0x16/0x20
  futex_hash+0x2a4/0x310
[...]
  exit_pi_state_list+0x110/0x5a0
[...]
  </TASK>
```

index a061f54b606d..94ebd2c25b2a 100644
@@ -47,6 +47,7 @@
 #include <linux/vmalloc.h>
 #include <linux/kmemleak.h>
 #include <linux/wait_bit.h>
+#include <linux/delay.h>
 
 #include <vdso/futex.h>
 

@@ -1359,6 +1360,7 @@ static void exit_pi_state_list(struct task_struct *curr)
 	 */
 	WARN_ON(curr != current);
 	guard(private_hash)(current->mm);
+	if (strcmp(current->comm, "SLOW-1") == 0) mdelay(1000);
 	/*
 	 * We are a ZOMBIE and nobody can enqueue itself on
 	 * pi_state_list anymore, but we have to be careful

@@ -1945,6 +1947,7 @@ static int futex_hash_allocate(unsigned int hash_slots, unsigned int flags)
 				 * reference on the existing hash.
 				 */
 				futex_ref_drop(cur);
+				if (strcmp(current->comm, "SLOW-2") == 0) mdelay(10000);
 			}
 
 			if (new) {

What's with the markdown tags? The email is text/plain, so plain it should be.

*groan*.

What a crazy ass world we live in that this has to be stated :-( Of course you should view text/plain in monospace, anything else would be absolutely insane. (And yes, I know outlook exists, but people using that deserve all the pain that piece of shit gets them.)

Right, that transition is not supposed to be possible. Notably a single thread cannot have (private) futex waiters, and we allocate the private hash on cloning the second thread.

So need_futex_hash_allocate_default() is explicitly excluding vfork from causing a private hash to be allocated. Perhaps we should fix that.

I need more thinking (and wake-up juice) to see if that might perhaps bring other problems with it.

Thanks, I'll go poke at this a bit.

So I can confirm that the below does in fact cure your testcase.

Per commit: ee9dce44362b ("futex: Drop CLONE_THREAD requirement for private default hash alloc") the reason for excluding vfork() was performance and thinking this
would/could not matter, which you've proven to be clearly false.

Thomas?

---
 kernel/fork.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 416758c8a3d4..bc32ea19099e 100644
@@ -1996,9 +1996,9 @@ static bool need_futex_hash_allocate_default(u64 clone_flags)
 {
 	/*
 	 * Allocate a default futex hash for any sibling that will
-	 * share the parent's mm, except vfork.
+	 * share the parent's mm.
 	 */
-	return (clone_flags & (CLONE_VM | CLONE_VFORK)) == CLONE_VM;
+	return clone_flags & CLONE_VM;
 }
 
 /*

On Fri, Sep 11 2026 at 11:04, Peter Zijlstra wrote:

I don't remember why we excluded VFORK in the first place. Sebastian?

That was introduced in commit ee9dce44362b ("futex: Drop CLONE_THREAD requirement for private default hash alloc"), and the commit message says:

| Loosen the check to cover any CLONE_VM clone, except vfork().  Excluding
| vfork keeps the existing paths untouched (no overhead), and we can't
| race in the first place: either the parent is suspended and the child
| runs alone, or mm->futex_ref is already allocated from an earlier
| CLONE_VM.

So I think that was purely supposed to be a performance optimization for single-threaded processes that do vfork()+exec()?

Right, but didn't really measure anything and am certainly fine with Peter's fixlet. Sorry for breaking things.

Thanks,
Davidlohr

in v2 I moved everything struct signal because the private hash should live there as per review. This explains the original check in need_futex_hash_allocate_default(). In v3 I moved to mm while the check remained unchanged.

The mm is cloned on CLONE_VM. Kernel threads don't get a private hash due to mm == NULL check in futex_hash_allocate_default() which was added in v4 with auto-resize.

To get the whole magic to work, we need to create the private hash once the first "parallel" user is created, the first thread. Skipping it for CLONE_VFORK violates this. I don't see why we shouldn't do what you just suggested.

Thanks, I'll go write up a Changelog or something and stick it in locking/urgent.

I like having clear delimiters around code blocks in the middle of text, I feel it can otherwise be annoying to scan for where cited code ends. I used to put <<< and >>> around them in the past, but that was kinda weird and ~nobody else uses that syntax...

(As another example, my employer's webmailer is also not particularly polished when it comes to text/plain mails.)