Re: [PATCH] futex: Fix might_sleep() warning in futex_pivot_pending()

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.

> +
> +		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