Re: [PATCH] futex: Fix might_sleep() warning in futex_pivot_pending()
From: Yao Kai
Date: Thu Aug 20 2026 - 03:33:11 EST
On 8/18/2026 8:24 PM, Yao Kai wrote:
On 8/18/2026 6:46 PM, Peter Zijlstra wrote:
On Mon, Aug 17, 2026 at 03:29:14PM +0800, Yao Kai wrote:
Thanks! I think there is still a lost-wakeup window:
T1 T2
add_wait_queue()
/* not visible to T2 */
futex_pivot_pending()
futex_ref_is_dead() = false
futex_ref_put() = true
wake_up_var()
waitqueue_active() = false
/* observes empty */
return
wait_woken()
schedule()
Since wake_up_var() uses a lockless waitqueue_active() check, I think
we need to order the waitqueue insertion before the first condition
check:
add_wait_queue(__wq_head, &__wbq_entry.wq_entry);
/*
* Pairs with the fully ordered refcount operation before wake_up_var().
* Ensures either the waker sees this waiter or we see the dead refcount.
*/
smp_mb();
Well, add_wait_queue() has UNLOCK(&wq_head->lock) and
futex_pivot_pending() has LOCK(&mmph->lock), giving an UNLOCK+LOCK
consistency, which IIRC is RCtso if you're on PowerPC and RCsc
everywhere else.
So yeah, this needs more. But I would instead suggest we use:
smp_mb__after_spinlock().
Anyway, for this to matter one way or the other, the other side of this
also needs a barrier. But it looks like futex_ref_put() already implies
enough. When in atomic mode it implies a full smp_mb().
while (!futex_pivot_pending(mm))
wait_woken(&__wbq_entry.wq_entry, TASK_UNINTERRUPTIBLE,
MAX_SCHEDULE_TIMEOUT);
The rc check can be dropped because MAX_SCHEDULE_TIMEOUT does not expire.
Indeed, I had realized this after sending :-)
Something like so then?
---
diff --git a/include/linux/wait.h b/include/linux/wait.h
index dce055e6add3..7e215330199c 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1228,6 +1228,7 @@ long prepare_to_wait_event(struct wait_queue_head *wq_head, struct wait_queue_en
void finish_wait(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry);
long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, long timeout);
int woken_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
+int woken_wake_bit_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
#define DEFINE_WAIT_FUNC(name, function) \
diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
index ace7379d627d..553d7b23e3ad 100644
--- a/include/linux/wait_bit.h
+++ b/include/linux/wait_bit.h
@@ -32,6 +32,7 @@ int out_of_line_wait_on_bit_timeout(unsigned long *word, int, wait_bit_action_f
int out_of_line_wait_on_bit_lock(unsigned long *word, int, wait_bit_action_f *action, unsigned int mode);
struct wait_queue_head *bit_waitqueue(unsigned long *word, int bit);
extern void __init wait_bit_init(void);
+extern struct wait_bit_key *__var_wake_key(struct wait_queue_entry *wq_entry, void *arg);
int wake_bit_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index a7c2a6242718..bd9fb0b17ee6 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -46,6 +46,7 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/kmemleak.h>
+#include <linux/wait_bit.h>
#include <vdso/futex.h>
@@ -1886,11 +1887,34 @@ static int futex_hash_allocate(unsigned int hash_slots, unsigned int flags)
futex_hash_bucket_init(&fph->queues[i]);
if (custom) {
+ struct wait_bit_queue_entry __wbq_entry;
+ struct wait_queue_head *__wq_head;
+
/*
* Only let prctl() wait / retry; don't unduly delay clone().
*/
again:
- wait_var_event(mm, futex_pivot_pending(mm));
+ __wq_head = __var_waitqueue(mm);
+ init_wait_var_entry(&__wbq_entry, mm, 0);
+ __wbq_entry.wq_entry.func = woken_wake_bit_function;
+ add_wait_queue(__wq_head, &__wbq_entry.wq_entry);
+
+ /*
+ * add_wait_queue() futex_ref_put()
+ * MB (this) MB (implied)
+ * futex_pivot_pending() wake_up_var()
+ * waitqueue_active()
+ *
+ * Notably, it must not be possible to see
+ * !futex_pivot_pending() && !waitqueue_active().
+ */
+ smp_mb__after_spinlock();
I still think we should use smp_mb() here, smp_mb__after_spinlock() only
orders accesses preceding the lock acquisition against later accesses. The
waitqueue insertion happens after that acquisition, so I don't think
smp_mb__after_spinlock() covers it here.
On further thought, please disregard my previous objection to
smp_mb__after_spinlock().
I was considering the documented semantics of
smp_mb__after_spinlock() in isolation and overlooked that the full
waiter-side sequence also includes the subsequent mutex acquisition in
futex_pivot_pending():
STORE waitqueue entry
UNLOCK wq_head->lock
smp_mb__after_spinlock()
LOCK mmph->lock
LOAD refcount
On architectures where the UNLOCK+LOCK sequence needs strengthening,
smp_mb__after_spinlock() provides the required full barrier. On
architectures where it is a no-op, the lock acquisition is already
strong enough to provide the required ordering.
So your version looks sufficient. Sorry for the noise.
+
+ while (!futex_pivot_pending(mm) &&
+ wait_woken(&__wbq_entry.wq_entry, TASK_UNINTERRUPTIBLE,
+ MAX_SCHEDULE_TIMEOUT))
+ /* empty */;
Since MAX_SCHEDULE_TIMEOUT never returns zero, so I think this can be:
while (!futex_pivot_pending(mm))
wait_woken(&__wbq_entry.wq_entry, TASK_UNINTERRUPTIBLE,
MAX_SCHEDULE_TIMEOUT));
+ remove_wait_queue(__wq_head, &__wbq_entry.wq_entry);
}
scoped_guard(mutex, &mm->futex.phash.lock) {
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 20f27e2cf7ae..d033f600f48c 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -5,6 +5,7 @@
* (C) 2004 Nadia Yvette Chambers, Oracle
*/
#include "sched.h"
+#include <linux/wait_bit.h>
void __init_waitqueue_head(struct wait_queue_head *wq_head, const char *name, struct lock_class_key *key)
{
@@ -463,3 +464,17 @@ int woken_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sy
return default_wake_function(wq_entry, mode, sync, key);
}
EXPORT_SYMBOL(woken_wake_function);
+
+int woken_wake_bit_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *arg)
+{
+ struct wait_bit_key *key = __var_wake_key(wq_entry, arg);
+ if (!key)
+ return 0;
+
+ /* Pairs with the smp_store_mb() in wait_woken(). */
+ smp_mb(); /* C */
+ wq_entry->flags |= WQ_FLAG_WOKEN;
+
+ return default_wake_function(wq_entry, mode, sync, key);
+}
+EXPORT_SYMBOL(woken_wake_bit_function);
diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
index 1088d3b7012c..e8127e83a48f 100644
--- a/kernel/sched/wait_bit.c
+++ b/kernel/sched/wait_bit.c
@@ -167,9 +167,7 @@ wait_queue_head_t *__var_waitqueue(void *p)
}
EXPORT_SYMBOL(__var_waitqueue);
-static int
-var_wake_function(struct wait_queue_entry *wq_entry, unsigned int mode,
- int sync, void *arg)
+struct wait_bit_key *__var_wake_key(struct wait_queue_entry *wq_entry, void *arg)
{
struct wait_bit_key *key = arg;
struct wait_bit_queue_entry *wbq_entry =
@@ -177,6 +175,17 @@ var_wake_function(struct wait_queue_entry *wq_entry, unsigned int mode,
if (wbq_entry->key.flags != key->flags ||
wbq_entry->key.bit_nr != key->bit_nr)
+ return NULL;
+
+ return key;
+}
+
+static int
+var_wake_function(struct wait_queue_entry *wq_entry, unsigned int mode,
+ int sync, void *arg)
+{
+ struct wait_bit_key *key = __var_wake_key(wq_entry, arg);
+ if (!key)
return 0;
return autoremove_wake_function(wq_entry, mode, sync, key);
Thanks,
Yao Kai