[PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Nickolai Zeldovich]

mempool_resize() shrinks a pool by dropping pool->lock around every
free_fn() call and, once the pool holds no more than new_min_nr
elements, stores new_min_nr into pool->min_nr.  new_min_nr was only
compared against pool->min_nr before the lock was first dropped, so that
store assumes nothing changed meanwhile.  The kernel-doc allows
mempool_alloc()/mempool_free() to run while a resize is in progress and
the grow path also copes with concurrent resizes ("Raced, other resize
will do our work"), but the shrink path does not: while its lock is
dropped another resize may shrink the pool below new_min_nr and a third
one may grow it again to a size between the two, replacing
pool->elements with an array of exactly that size.  Storing the stale
new_min_nr afterwards leaves pool->min_nr larger than the elements
array.

Interleaving on a pool with min_nr = curr_nr = 10:

  CPU A: mempool_resize(pool, 8)
    takes the lock, 8 <= 10 so it shrinks: remove_element(), drops the
    lock, calls free_fn() and is preempted there
  CPU B: mempool_resize(pool, 3)
    completes: curr_nr = 3, min_nr = 3, elements still has 10 slots
  CPU B: mempool_resize(pool, 5)
    completes: elements = kmalloc_objs(5), min_nr = 5, curr_nr = 5
  CPU A: retakes the lock, 8 < curr_nr (5) is false, stores min_nr = 8
    while elements has 5 slots

Every later mempool_free() sees curr_nr < min_nr and add_element()
stores the returned element at elements[5], [6] and [7], past the end
of the 5-slot kmalloc allocation: a heap out-of-bounds write, i.e. slab
corruption, a KASAN report, or a crash in whichever unrelated object
sits next to the array.  The invariant that breaks is that pool->min_nr
never exceeds the size of pool->elements; every other path that changes
min_nr or elements keeps it.

An in-tree caller can hit this: fs/smb/client resizes the global
cifs_req_poolp from cifs_demultiplex_thread() (grow, when a TCP session
is created) and from clean_demultiplex_info() (shrink, when one goes
away).  The two are coordinated only through the atomic
tcpSesAllocCount, so mounts and unmounts racing on one client run the
resizes above concurrently; pool->free is kmem_cache_free() there and
the window is a preemption inside it.  The other callers
(block/blk-zoned.c, drivers/s390/scsi/zfcp_erp.c) serialise their
resizes.

The shrink path has stored new_min_nr unconditionally since
mempool_resize() was introduced, while the grow path has always
re-checked pool->min_nr after reacquiring the lock.

Re-check new_min_nr against pool->min_nr under the lock before storing
it.  If a concurrent resize has shrunk the pool below new_min_nr in the
meantime, keep that resize's min_nr (and whatever array it installed),
as the grow path already does; otherwise the store is unchanged.  The
elements the loop has already freed stay freed, which is right either
way because the pool's limit is now below new_min_nr.

Found by formal verification of mm/mempool.c against the kernel binary.
Reproduced with a userspace build of the affected functions in which the
free_fn() callback drives the interleaving above: before the fix min_nr
ends at 8 with a 5-slot array and the next mempool_free() writes past it
(AddressSanitizer heap-buffer-overflow); after the fix min_nr stays at 5
and the element is handed to free_fn().  Not reproduced on a running
kernel, where the window is a single preemption inside free_fn().

Assisted-by: LLM
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---

 mm/mempool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/mempool.c b/mm/mempool.c
index cb74e718b2c6..95533e2975ce 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -378,7 +378,9 @@ int mempool_resize(struct mempool *pool, int new_min_nr)
 			pool->free(element, pool->pool_data);
 			spin_lock_irqsave(&pool->lock, flags);
 		}
-		pool->min_nr = new_min_nr;
+		/* Raced, the array may now be smaller than new_min_nr */
+		if (new_min_nr <= pool->min_nr)
+			pool->min_nr = new_min_nr;
 		goto out_unlock;
 	}
 	spin_unlock_irqrestore(&pool->lock, flags);

base-commit: 90201351a862182bdabfc1ad1c97b1e1b6ed5931
-- 
2.53.0

Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Harry Yoo]

[+Cc Christoph Hellwig and Vitaly Wool ]

On Fri, Oct 16, 2026 at 09:25:10AM -0400, Nickolai Zeldovich wrote:
> mempool_resize() shrinks a pool by dropping pool->lock around every
> free_fn() call and, once the pool holds no more than new_min_nr
> elements, stores new_min_nr into pool->min_nr.  new_min_nr was only
> compared against pool->min_nr before the lock was first dropped, so that
> store assumes nothing changed meanwhile.  The kernel-doc allows
> mempool_alloc()/mempool_free() to run while a resize is in progress and
> the grow path also copes with concurrent resizes ("Raced, other resize
> will do our work"), but the shrink path does not: while its lock is
> dropped another resize may shrink the pool below new_min_nr and a third
> one may grow it again to a size between the two, replacing
> pool->elements with an array of exactly that size.  Storing the stale
> new_min_nr afterwards leaves pool->min_nr larger than the elements
> array.
> 
> Interleaving on a pool with min_nr = curr_nr = 10:
> 
>   CPU A: mempool_resize(pool, 8)
>     takes the lock, 8 <= 10 so it shrinks: remove_element(), drops the
>     lock, calls free_fn() and is preempted there
>   CPU B: mempool_resize(pool, 3)
>     completes: curr_nr = 3, min_nr = 3, elements still has 10 slots
>   CPU B: mempool_resize(pool, 5)
>     completes: elements = kmalloc_objs(5), min_nr = 5, curr_nr = 5
>   CPU A: retakes the lock, 8 < curr_nr (5) is false, stores min_nr = 8
>     while elements has 5 slots
> 
> Every later mempool_free() sees curr_nr < min_nr and add_element()
> stores the returned element at elements[5], [6] and [7], past the end
> of the 5-slot kmalloc allocation: a heap out-of-bounds write, i.e. slab
> corruption, a KASAN report, or a crash in whichever unrelated object
> sits next to the array.  The invariant that breaks is that pool->min_nr
> never exceeds the size of pool->elements; every other path that changes
> min_nr or elements keeps it.
> 
> An in-tree caller can hit this: fs/smb/client resizes the global
> cifs_req_poolp from cifs_demultiplex_thread() (grow, when a TCP session
> is created) and from clean_demultiplex_info() (shrink, when one goes
> away).  The two are coordinated only through the atomic
> tcpSesAllocCount, so mounts and unmounts racing on one client run the
> resizes above concurrently; pool->free is kmem_cache_free() there and
> the window is a preemption inside it.  The other callers
> (block/blk-zoned.c, drivers/s390/scsi/zfcp_erp.c) serialise their
> resizes.

The bug looks valid in theory, not sure if how easily it can be
reproduced in practice as resizing should be quite rare.

(A bug is a bug and should be fixed, but no need to hurry in this case)

> The shrink path has stored new_min_nr unconditionally since
> mempool_resize() was introduced, while the grow path has always
> re-checked pool->min_nr after reacquiring the lock.

So it's Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") ? 

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=6892188b54d484098b79b0c9813c20e543124e2d

...from a historical commit before git era, 1da177e4c3f4.

> Re-check new_min_nr against pool->min_nr under the lock before storing
> it.  If a concurrent resize has shrunk the pool below new_min_nr in the
> meantime, keep that resize's min_nr (and whatever array it installed),
> as the grow path already does; otherwise the store is unchanged.  The
> elements the loop has already freed stay freed, which is right either
> way because the pool's limit is now below new_min_nr.
> 
> Found by formal verification of mm/mempool.c against the kernel binary.
> Reproduced with a userspace build of the affected functions in which the
> free_fn() callback drives the interleaving above: before the fix min_nr
> ends at 8 with a 5-slot array and the next mempool_free() writes past it
> (AddressSanitizer heap-buffer-overflow); after the fix min_nr stays at 5
> and the element is handed to free_fn().  Not reproduced on a running
> kernel, where the window is a single preemption inside free_fn().
> 
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> ---
>
>  mm/mempool.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempool.c b/mm/mempool.c
> index cb74e718b2c6..95533e2975ce 100644
> --- a/mm/mempool.c
> +++ b/mm/mempool.c
> @@ -378,7 +378,9 @@ int mempool_resize(struct mempool *pool, int new_min_nr)
>  			pool->free(element, pool->pool_data);
>  			spin_lock_irqsave(&pool->lock, flags);
>  		}
> -		pool->min_nr = new_min_nr;
> +		/* Raced, the array may now be smaller than new_min_nr */
> +		if (new_min_nr <= pool->min_nr)
> +			pool->min_nr = new_min_nr;
>  		goto out_unlock;
>  	}
>  	spin_unlock_irqrestore(&pool->lock, flags);
> 
> base-commit: 90201351a862182bdabfc1ad1c97b1e1b6ed5931
> -- 
> 2.53.0
> 

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Nickolai Zeldovich]

On Fri, Oct 16, 2026 at 11:46 AM Harry Yoo <harry@kernel.org> wrote:
> The bug looks valid in theory, not sure if how easily it can be
> reproduced in practice as resizing should be quite rare.
>
> (A bug is a bug and should be fixed, but no need to hurry in this case)

I agree; seems unlikely to happen, but might as well fix it at some point.

> So it's Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") ?

Yes; I didn't think it was useful to include this "catch-all" tag, but
technically, that's correct.

Thanks,

Nickolai.

Re: Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Vitaly Wool]

> [+Cc Christoph Hellwig and Vitaly Wool ]

Thanks for CC'ing me on this. How about we take my patch [1] instead since
it isn't prone to this bug and is a nice optimization to the existing
mempool_resize() at the same time?

[1] https://lore.kernel.org/linux-mm/20261013091408.1231786-1-vitaly.wool@konsulko.se/

~Vitaly

Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Harry Yoo]

On Sun, Oct 18, 2026 at 05:01:52PM +0200, Vitaly Wool wrote:
> > [+Cc Christoph Hellwig and Vitaly Wool ]
> 
> Thanks for CC'ing me on this.

No problem.

> How about we take my patch [1] instead since
> it isn't prone to this bug and is a nice optimization to the existing
> mempool_resize() at the same time?

It is not a good idea to squeeze a bug fix and an optimization
into one patch.

> [1] https://lore.kernel.org/linux-mm/20261013091408.1231786-1-vitaly.wool@konsulko.se/
> 
> ~Vitaly
> 

-- 
Cheers,
Harry / Hyeonggon

Re: Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Vitaly Wool]

> > How about we take my patch [1] instead since
> > it isn't prone to this bug and is a nice optimization to the existing
> > mempool_resize() at the same time?
>
> It is not a good idea to squeeze a bug fix and an optimization
> into one patch.

Generally speaking, yes. But that patch had been submitted before the fix
popped up, so there was no sqeezing per se. It's just a better way of doing
things.

~Vitaly

Re: [PATCH v1] mempool: fix min_nr exceeding the elements array after concurrent resizes

[Harry Yoo]

On Sun, Oct 18, 2026 at 06:03:15PM +0200, Vitaly Wool wrote:
> > > How about we take my patch [1] instead since
> > > it isn't prone to this bug and is a nice optimization to the existing
> > > mempool_resize() at the same time?
> >
> > It is not a good idea to squeeze a bug fix and an optimization
> > into one patch.
> 
> Generally speaking, yes. But that patch had been submitted before the fix
> popped up, so there was no sqeezing per se.

I mean the patch you've submitted does two things (wihch is not good).

It should be split into two patches that 1) fixes the bug and 2)
optimizes it.

(Setting aside the question of whether the optimization is worth the
 complexity)

-- 
Cheers,
Harry / Hyeonggon