linux-kernel - Re: [PATCH 06/11] mempool: factor out a mempool_alloc_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8a2c740-2208-5692-cfc6-f475ae0f7d45@google.com>
Date: Sat, 22 Nov 2025 19:42:51 -0800 (PST)
From: Hugh Dickins <hughd@...gle.com>
To: Christoph Hellwig <hch@....de>, Vlastimil Babka <vbabka@...e.cz>
cc: Andrew Morton <akpm@...ux-foundation.org>, 
    Christoph Lameter <cl@...two.org>, David Rientjes <rientjes@...gle.com>, 
    Roman Gushchin <roman.gushchin@...ux.dev>, 
    Harry Yoo <harry.yoo@...cle.com>, Suren Baghdasaryan <surenb@...gle.com>, 
    Michal Hocko <mhocko@...e.com>, Brendan Jackman <jackmanb@...gle.com>, 
    Zi Yan <ziy@...dia.com>, Eric Biggers <ebiggers@...nel.org>, 
    linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/11] mempool: factor out a mempool_alloc_from_pool
 helper

On Thu, 13 Nov 2025, Christoph Hellwig wrote:

> Add a helper for the mempool_alloc slowpath to better separate it from the
> fast path, and also use it to implement mempool_alloc_preallocated which
> shares the same logic.
> 
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
...
> @@ -413,8 +457,6 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask)
>  {
>  	gfp_t gfp_temp = mempool_adjust_gfp(&gfp_mask);
>  	void *element;
> -	unsigned long flags;
> -	wait_queue_entry_t wait;
>  
>  	VM_WARN_ON_ONCE(gfp_mask & __GFP_ZERO);
>  	might_alloc(gfp_mask);
> @@ -428,53 +470,22 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask)
>  		element = pool->alloc(gfp_temp, pool->pool_data);
>  	}
>  
> -	if (likely(element))
> -		return element;
> -
> -	spin_lock_irqsave(&pool->lock, flags);
> -	if (likely(pool->curr_nr)) {
> -		element = remove_element(pool);
> -		spin_unlock_irqrestore(&pool->lock, flags);
> -		/* paired with rmb in mempool_free(), read comment there */
> -		smp_wmb();
> +	if (unlikely(!element)) {
>  		/*
> -		 * Update the allocation stack trace as this is more useful
> -		 * for debugging.
> +		 * Try to allocate an element from the pool.
> +		 *
> +		 * The first pass won't have __GFP_DIRECT_RECLAIM and won't
> +		 * sleep in mempool_alloc_from_pool.  Retry the allocation
> +		 * with all flags set in that case.
>  		 */
> -		kmemleak_update_trace(element);
> -		return element;
> -	}
> -
> -	/*
> -	 * We use gfp mask w/o direct reclaim or IO for the first round.  If
> -	 * alloc failed with that and @pool was empty, retry immediately.
> -	 */
> -	if (gfp_temp != gfp_mask) {
> -		spin_unlock_irqrestore(&pool->lock, flags);
> -		gfp_temp = gfp_mask;
> -		goto repeat_alloc;
> -	}
> -
> -	/* We must not sleep if !__GFP_DIRECT_RECLAIM */
> -	if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> -		spin_unlock_irqrestore(&pool->lock, flags);
> -		return NULL;
> +		element = mempool_alloc_from_pool(pool, gfp_mask);
> +		if (!element && gfp_temp != gfp_mask) {

No, that is wrong, it breaks the mempool promise: linux-next oopses
in swap_writepage_bdev_async(), which relies on bio_alloc(,,,GFP_NOIO)
to return a good bio.

The refactoring makes it hard to see, but the old version always used
to go back to repeat_alloc at the end, if __GFP_DIRECT_RECLAIM,
whereas here it only does so the first time, when gfp_temp != gfp_mask.

After bisecting to here, I changed that "gfp_temp != gfp_mask" to
"(gfp & __GFP_DIRECT_RECLAIM)", and it worked again.  But other patches
have come in on top, so below is a patch to the final mm/mempool.c...

> +			gfp_temp = gfp_mask;
> +			goto repeat_alloc;
> +		}
>  	}
>  
> -	/* Let's wait for someone else to return an element to @pool */
> -	init_wait(&wait);
> -	prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);
> -
> -	spin_unlock_irqrestore(&pool->lock, flags);
> -
> -	/*
> -	 * FIXME: this should be io_schedule().  The timeout is there as a
> -	 * workaround for some DM problems in 2.6.18.
> -	 */
> -	io_schedule_timeout(5*HZ);
> -
> -	finish_wait(&pool->wait, &wait);
> -	goto repeat_alloc;
> +	return element;
>  }
>  EXPORT_SYMBOL(mempool_alloc_noprof);
...

[PATCH] mempool: fix NULL from mempool_alloc_noprof()

mempool_alloc_noprof() with __GFP_DIRECT_RECLAIM used to loop until it
had allocated an element, but recently regressed to returning NULL when
pool->alloc and mempool_alloc_from_pool() have both failed twice, causing
oops in __swap_writepage() (and presumably others relying on mempool).

Fixes: 1d091d2c5bf3 ("mempool: factor out a mempool_alloc_from_pool helper")
Signed-off-by: Hugh Dickins <hughd@...gle.com>
---
 mm/mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mempool.c b/mm/mempool.c
index 1558601600ba..dc9f2a1dc35f 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -576,7 +576,7 @@ void *mempool_alloc_noprof(struct mempool *pool, gfp_t gfp_mask)
 		 * with all flags set in that case.
 		 */
 		if (!mempool_alloc_from_pool(pool, &element, 1, 0, gfp_mask) &&
-		    gfp_temp != gfp_mask) {
+		    (gfp_mask & __GFP_DIRECT_RECLAIM)) {
 			gfp_temp = gfp_mask;
 			goto repeat_alloc;
 		}
-- 
2.51.0