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

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
>