Re: [PATCH] mm: mempool: fix wake-up edge case bug for zero-minimum pools

[Yadan Fan <ydfan@suse.com>]

Hi Andrew,

On 7/17/25 05:19, Andrew Morton wrote:
> On Wed, 16 Jul 2025 23:37:30 +0800 Yadan Fan <ydfan@suse.com> wrote:
> 
>> The mempool wake-up mechanism has a edge case bug that affects pools
>> created with min_nr=0. When a thread blocks waiting for memory from an
>> empty pool (curr_nr == 0), subsequent mempool_free() calls fail to wake
>> the waiting thread because the condition "curr_nr < min_nr" evaluates
>> to "0 < 0" which is false, this causes threads to sleep indefinitely.
>>
>> There is at least 2 places where the mempool created with min_nr=0:
>>
>> 1. lib/btree.c:191: mempool_create(0, btree_alloc, btree_free, NULL)
>> 2. drivers/md/dm-verity-fec.c:791:
>>     mempool_init_slab_pool(&f->extra_pool, 0, f->cache)
> 
> This is very old code.  Can you suggest why this has taken so long to
> surface?
> 
> Which is a roundabout way of asking "should this be backported into
> -stable kernels".  For that we'd need to know how this issue is
> affecting our users.

There is no real issue yet, I just reviewed the codes here and found this,
I thought it may needs to fix so that I sent this patch.

> 
>> Add an explicit check in mempool_free() to handle the min_nr=0 case:
>> when the pool has zero minimum reserves, is currently empty, and has
>> active waiters, wake them up. The wq_has_sleeper() avoids unnecessary
>> wake-ups when no threads are waiting.
> 
> Do we need the separate test?  What's wrong with the obvious approach
> of replacing the "<" with "<=" in the preceding test?

Simply changing to "<=" has problem since add_element() has 
"BUG_ON(pool->curr_nr >= pool->min_nr);".

> 
> And would the previous (ie, existing) test benefit from the
> wq_has_sleeper() check?

I think it could have benefit for the existing test, wq_has_sleeper() is 
cost cheaper than wake_up().

I will submit a new patch containing it.

> 
>> --- a/mm/mempool.c
>> +++ b/mm/mempool.c
>> @@ -545,6 +545,22 @@ void mempool_free(void *element, mempool_t *pool)
>>                  }
>>                  spin_unlock_irqrestore(&pool->lock, flags);
>>          }
>> +       /*
>> +        * Handle the min_nr = 0 edge case:
>> +        * For zero-minimum pools, curr_nr < min_nr (0 < 0) never succeeds,
>> +        * so waiters sleeping on pool->wait would never be woken by the
>> +        * normal wake-up path. This explicit check ensures that when
>> +        * pool->min_nr == 0 and pool->curr_nr == 0, any active waiters
>> +        * are properly awakened.
>> +        * The wq_has_sleeper() avoids unnecessary wake-ups when no
>> +        * threads are waiting.
>> +        */
>> +       if (unlikely(pool->min_nr == 0 &&
>> +                    READ_ONCE(pool->curr_nr) == 0 &&
>> +                    wq_has_sleeper(&pool->wait))) {
>> +               wake_up(&pool->wait);
>> +       }
>> +
> 
> Something strange is happening with the whitespace here.  I pretty much
> retyped the patch.  Please have a chat with your email client ;)
> 

Sorry for this, I may just messed up somehow my client configuration, will fix it.

Thanks,
Yadan