[PATCH] slab: fix memory leak when refill_sheaf() fails

[PATCH] slab: fix memory leak when refill_sheaf() fails

[Qing Wang]

When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
but need to fill 10), it will update sheaf->size and return -ENOMEM.
However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
causing the partially allocated objects memory in sheaf->objects[] leaked.

Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
free objects of sheaf->objects[]. And also add a WARN_ON() in
free_empty_sheaf() to catch any future cases where a non-empty sheaf is
being freed.

Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")
Signed-off-by: Qing Wang <wangqing7171@gmail.com>
---
 mm/slub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 20cb4f3b636d..73b2cfd0e123 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2797,6 +2797,7 @@ static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
 	if (s->flags & SLAB_KMALLOC)
 		mark_obj_codetag_empty(sheaf);
 
+	WARN_ON(sheaf->size > 0);
 	kfree(sheaf);
 
 	stat(s, SHEAF_FREE);
@@ -2828,6 +2829,7 @@ static int refill_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf,
 	return 0;
 }
 
+static void sheaf_flush_unused(struct kmem_cache *s, struct slab_sheaf *sheaf);
 
 static struct slab_sheaf *alloc_full_sheaf(struct kmem_cache *s, gfp_t gfp)
 {
@@ -2837,6 +2839,7 @@ static struct slab_sheaf *alloc_full_sheaf(struct kmem_cache *s, gfp_t gfp)
 		return NULL;
 
 	if (refill_sheaf(s, sheaf, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
+		sheaf_flush_unused(s, sheaf);
 		free_empty_sheaf(s, sheaf);
 		return NULL;
 	}
@@ -4623,6 +4626,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 			 * we must be very low on memory so don't bother
 			 * with the barn
 			 */
+			sheaf_flush_unused(s, empty);
 			free_empty_sheaf(s, empty);
 		}
 	} else {
-- 
2.34.1

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Harry Yoo]

On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
> When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
> but need to fill 10), it will update sheaf->size and return -ENOMEM.
> However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
> directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
> causing the partially allocated objects memory in sheaf->objects[] leaked.

Nice catch, thanks!
Probably the need to fail new_slab() made it quite hard to trigger and notice.

> Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
> free objects of sheaf->objects[]. And also add a WARN_ON() in
> free_empty_sheaf() to catch any future cases where a non-empty sheaf is
> being freed.
> 
> Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")

I think we need to add Cc: stable@vger.kernel.org

> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> ---
>  mm/slub.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 20cb4f3b636d..73b2cfd0e123 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2797,6 +2797,7 @@ static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
>  	if (s->flags & SLAB_KMALLOC)
>  		mark_obj_codetag_empty(sheaf);
>  
> +	WARN_ON(sheaf->size > 0);

nit: perhaps VM_WARN_ON_ONCE(); will be enough?

Otherwise looks good to me, so:
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Harry Yoo]

On Wed, Mar 11, 2026 at 08:16:46PM +0900, Harry Yoo wrote:
> On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
> > When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
> > but need to fill 10), it will update sheaf->size and return -ENOMEM.
> > However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
> > directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
> > causing the partially allocated objects memory in sheaf->objects[] leaked.
> 
> Nice catch, thanks!
> Probably the need to fail new_slab() made it quite hard to trigger and notice.

Just out of curiosity, could you please tell us how you discovered
this and confirmed that it's fixed, given that kmemleak won't detect it?

> > Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
> > free objects of sheaf->objects[]. And also add a WARN_ON() in
> > free_empty_sheaf() to catch any future cases where a non-empty sheaf is
> > being freed.

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Qing Wang]

On Wed, 11 Mar 2026 at 19:48, Harry Yoo <harry.yoo@oracle.com> wrote:
> Just out of curiosity, could you please tell us how you discovered
> this and confirmed that it's fixed, given that kmemleak won't detect it?

Just discovered it by code reading (while learing this featch):). And there
is no good way to reproduce it, so confirmed it by code logic.

--
cheers,
Qing

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Harry Yoo]

On Thu, Mar 12, 2026 at 10:21:42AM +0800, Qing Wang wrote:
> On Wed, 11 Mar 2026 at 19:48, Harry Yoo <harry.yoo@oracle.com> wrote:
> > Just out of curiosity, could you please tell us how you discovered
> > this and confirmed that it's fixed, given that kmemleak won't detect it?
> 
> Just discovered it by code reading (while learing this featch):). And there
> is no good way to reproduce it, so confirmed it by code logic.

Ah, that makes sense.
Thanks!

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Vlastimil Babka]

On 3/11/26 12:16, Harry Yoo wrote:
> On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
>> When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
>> but need to fill 10), it will update sheaf->size and return -ENOMEM.
>> However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
>> directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
>> causing the partially allocated objects memory in sheaf->objects[] leaked.
> 
> Nice catch, thanks!

Indeed, thanks!

> Probably the need to fail new_slab() made it quite hard to trigger and notice.

Agreed.

>> Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
>> free objects of sheaf->objects[]. And also add a WARN_ON() in
>> free_empty_sheaf() to catch any future cases where a non-empty sheaf is
>> being freed.
>> 
>> Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")

Actually I think that commit was fine as it was using bulk alloc to refill
and that was undoing any partial successes. I think this one is correct and
replaced it so:

Fixes: ed30c4adfc2b ("slab: add optimized sheaf refill from partial list")

> I think we need to add Cc: stable@vger.kernel.org

And therefore we don't, unless I'm mistaken.

>> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
>> ---
>>  mm/slub.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 20cb4f3b636d..73b2cfd0e123 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2797,6 +2797,7 @@ static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
>>  	if (s->flags & SLAB_KMALLOC)
>>  		mark_obj_codetag_empty(sheaf);
>>  
>> +	WARN_ON(sheaf->size > 0);
> 
> nit: perhaps VM_WARN_ON_ONCE(); will be enough?

Yep replaced it too.

Added to slab/for-next-fixes, thanks!

> Otherwise looks good to me, so:
> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> 

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Harry Yoo]

On Wed, Mar 11, 2026 at 05:59:03PM +0100, Vlastimil Babka wrote:
> On 3/11/26 12:16, Harry Yoo wrote:
> > On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
> >> When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
> >> but need to fill 10), it will update sheaf->size and return -ENOMEM.
> >> However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
> >> directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
> >> causing the partially allocated objects memory in sheaf->objects[] leaked.
> > 
> > Nice catch, thanks!
> 
> Indeed, thanks!
> 
> > Probably the need to fail new_slab() made it quite hard to trigger and notice.
> 
> Agreed.
> 
> >> Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
> >> free objects of sheaf->objects[]. And also add a WARN_ON() in
> >> free_empty_sheaf() to catch any future cases where a non-empty sheaf is
> >> being freed.
> >> 
> >> Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")
> 
> Actually I think that commit was fine as it was using bulk alloc to refill
> and that was undoing any partial successes. I think this one is correct and
> replaced it so:

Oops, I missed that.

Yeah, at first look I wondered "Well, refill_sheaf()" doesn't partially
fill sheaves!" Then I looked at the code right now and it did :/

In 2d517aa09bbc did not partially refill sheaves as it used
__kmem_cache_alloc_bulk() to refill sheaves,

> Fixes: ed30c4adfc2b ("slab: add optimized sheaf refill from partial list")

and this changed the behavior by replacing it with __refill_objects().
So this is the right Fixes: tag.

> > I think we need to add Cc: stable@vger.kernel.org
> 
> And therefore we don't, unless I'm mistaken.

You're right.

> >> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> >> ---
> >>  mm/slub.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 20cb4f3b636d..73b2cfd0e123 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -2797,6 +2797,7 @@ static void free_empty_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf)
> >>  	if (s->flags & SLAB_KMALLOC)
> >>  		mark_obj_codetag_empty(sheaf);
> >>  
> >> +	WARN_ON(sheaf->size > 0);
> > 
> > nit: perhaps VM_WARN_ON_ONCE(); will be enough?
> 
> Yep replaced it too.
> 
> Added to slab/for-next-fixes, thanks!

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Hao Li]

On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
> When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
> but need to fill 10), it will update sheaf->size and return -ENOMEM.
> However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
> directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
> causing the partially allocated objects memory in sheaf->objects[] leaked.
> 
> Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
> free objects of sheaf->objects[]. And also add a WARN_ON() in
> free_empty_sheaf() to catch any future cases where a non-empty sheaf is
> being freed.
> 
> Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")
> Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> ---

This fix looks good to me.
So feel free to add:

Reviewed-by: Hao Li <hao.li@linux.dev>

I also want to bring up another point here, although it may be outside the
scope of the current fix.

When I looked into the refill_sheaf() path, I found a refill failure does not
guarantee that the sheaf remains intact: refill_sheaf() can partially fill the
sheaf before failing. This non-intact behavior propagates to its caller,
__prefill_sheaf_pfmemalloc(), which therefore also cannot assume that the sheaf
is still intact after a refill failure.

However, the comment for kmem_cache_refill_sheaf() says that "if the refill
fails (returning -ENOMEM), the existing sheaf is left intact." That means the
behavior of __prefill_sheaf_pfmemalloc() - where the sheaf may be left
partially filled on refill failure - contradicts the API contract of
kmem_cache_refill_sheaf().

Maybe we can add rollback logic to __prefill_sheaf_pfmemalloc() so that it
provides intact semantics, preventing the non-intact behavior of refill_sheaf()
from propagating up to kmem_cache_refill_sheaf().

-- 
Thanks,
Hao

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Hao Li]

On Wed, Mar 11, 2026 at 10:46:23PM +0800, Hao Li wrote:
> On Wed, Mar 11, 2026 at 05:36:17PM +0800, Qing Wang wrote:
> > When refill_sheaf() partially fills one sheaf (e.g., fills 5 objects
> > but need to fill 10), it will update sheaf->size and return -ENOMEM.
> > However, the callers (alloc_full_sheaf() and __pcs_replace_empty_main())
> > directly call free_empty_sheaf() on failure, which only does kfree(sheaf),
> > causing the partially allocated objects memory in sheaf->objects[] leaked.
> > 
> > Fix this by calling sheaf_flush_unused() before free_empty_sheaf() to
> > free objects of sheaf->objects[]. And also add a WARN_ON() in
> > free_empty_sheaf() to catch any future cases where a non-empty sheaf is
> > being freed.
> > 
> > Fixes: 2d517aa09bbc ("slab: add opt-in caching layer of percpu sheaves")
> > Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> > ---
> 
> This fix looks good to me.
> So feel free to add:
> 
> Reviewed-by: Hao Li <hao.li@linux.dev>
> 
> I also want to bring up another point here, although it may be outside the
> scope of the current fix.
> 
> When I looked into the refill_sheaf() path, I found a refill failure does not
> guarantee that the sheaf remains intact: refill_sheaf() can partially fill the
> sheaf before failing. This non-intact behavior propagates to its caller,
> __prefill_sheaf_pfmemalloc(), which therefore also cannot assume that the sheaf
> is still intact after a refill failure.
> 
> However, the comment for kmem_cache_refill_sheaf() says that "if the refill
> fails (returning -ENOMEM), the existing sheaf is left intact." That means the
> behavior of __prefill_sheaf_pfmemalloc() - where the sheaf may be left
> partially filled on refill failure - contradicts the API contract of
> kmem_cache_refill_sheaf().
> 
> Maybe we can add rollback logic to __prefill_sheaf_pfmemalloc() so that it
> provides intact semantics, preventing the non-intact behavior of refill_sheaf()
> from propagating up to kmem_cache_refill_sheaf().

Looking at this a bit more, after checking the current callers, it seems that
the existing callers of kmem_cache_refill_sheaf() are not relying on the sheaf
remaining intact on refill failure.

If so, then another possible option might be to update the comment for
kmem_cache_refill_sheaf() to match the current behavior, rather than adding
rollback logic.

So it may just come down to whether we want to preserve the documented
semantics in the implementation, or adjust the comment to reflect what the code
already does.

I may be missing some intended dependency here, though.

-- 
Thanks,
Hao

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Vlastimil Babka]

On 3/11/26 17:30, Hao Li wrote:
>> 
>> I also want to bring up another point here, although it may be outside the
>> scope of the current fix.
>> 
>> When I looked into the refill_sheaf() path, I found a refill failure does not
>> guarantee that the sheaf remains intact: refill_sheaf() can partially fill the
>> sheaf before failing. This non-intact behavior propagates to its caller,
>> __prefill_sheaf_pfmemalloc(), which therefore also cannot assume that the sheaf
>> is still intact after a refill failure.
>> 
>> However, the comment for kmem_cache_refill_sheaf() says that "if the refill
>> fails (returning -ENOMEM), the existing sheaf is left intact." That means the
>> behavior of __prefill_sheaf_pfmemalloc() - where the sheaf may be left
>> partially filled on refill failure - contradicts the API contract of
>> kmem_cache_refill_sheaf().
>> 
>> Maybe we can add rollback logic to __prefill_sheaf_pfmemalloc() so that it
>> provides intact semantics, preventing the non-intact behavior of refill_sheaf()
>> from propagating up to kmem_cache_refill_sheaf().
> 
> Looking at this a bit more, after checking the current callers, it seems that
> the existing callers of kmem_cache_refill_sheaf() are not relying on the sheaf
> remaining intact on refill failure.
> 
> If so, then another possible option might be to update the comment for
> kmem_cache_refill_sheaf() to match the current behavior, rather than adding
> rollback logic.

I agree with this option. Having possibly more objects than before the call
shouldn't be an issue for the callers.

> So it may just come down to whether we want to preserve the documented
> semantics in the implementation, or adjust the comment to reflect what the code
> already does.
> 
> I may be missing some intended dependency here, though.
> 

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Harry Yoo]

On Wed, Mar 11, 2026 at 05:54:40PM +0100, Vlastimil Babka wrote:
> On 3/11/26 17:30, Hao Li wrote:
> >> 
> >> I also want to bring up another point here, although it may be outside the
> >> scope of the current fix.
> >> 
> >> When I looked into the refill_sheaf() path, I found a refill failure does not
> >> guarantee that the sheaf remains intact: refill_sheaf() can partially fill the
> >> sheaf before failing. This non-intact behavior propagates to its caller,
> >> __prefill_sheaf_pfmemalloc(), which therefore also cannot assume that the sheaf
> >> is still intact after a refill failure.
> >> 
> >> However, the comment for kmem_cache_refill_sheaf() says that "if the refill
> >> fails (returning -ENOMEM), the existing sheaf is left intact." That means the
> >> behavior of __prefill_sheaf_pfmemalloc() - where the sheaf may be left
> >> partially filled on refill failure - contradicts the API contract of
> >> kmem_cache_refill_sheaf().
> >> 
> >> Maybe we can add rollback logic to __prefill_sheaf_pfmemalloc() so that it
> >> provides intact semantics, preventing the non-intact behavior of refill_sheaf()
> >> from propagating up to kmem_cache_refill_sheaf().
> > 
> > Looking at this a bit more, after checking the current callers, it seems that
> > the existing callers of kmem_cache_refill_sheaf() are not relying on the sheaf
> > remaining intact on refill failure.
> > 
> > If so, then another possible option might be to update the comment for
> > kmem_cache_refill_sheaf() to match the current behavior, rather than adding
> > rollback logic.
> 
> I agree with this option. Having possibly more objects than before the call
> shouldn't be an issue for the callers.

+1 for this!

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] slab: fix memory leak when refill_sheaf() fails

[Hao Li]

On Thu, Mar 12, 2026 at 01:40:01PM +0900, Harry Yoo wrote:
> On Wed, Mar 11, 2026 at 05:54:40PM +0100, Vlastimil Babka wrote:
> > On 3/11/26 17:30, Hao Li wrote:
> > >> 
> > >> I also want to bring up another point here, although it may be outside the
> > >> scope of the current fix.
> > >> 
> > >> When I looked into the refill_sheaf() path, I found a refill failure does not
> > >> guarantee that the sheaf remains intact: refill_sheaf() can partially fill the
> > >> sheaf before failing. This non-intact behavior propagates to its caller,
> > >> __prefill_sheaf_pfmemalloc(), which therefore also cannot assume that the sheaf
> > >> is still intact after a refill failure.
> > >> 
> > >> However, the comment for kmem_cache_refill_sheaf() says that "if the refill
> > >> fails (returning -ENOMEM), the existing sheaf is left intact." That means the
> > >> behavior of __prefill_sheaf_pfmemalloc() - where the sheaf may be left
> > >> partially filled on refill failure - contradicts the API contract of
> > >> kmem_cache_refill_sheaf().
> > >> 
> > >> Maybe we can add rollback logic to __prefill_sheaf_pfmemalloc() so that it
> > >> provides intact semantics, preventing the non-intact behavior of refill_sheaf()
> > >> from propagating up to kmem_cache_refill_sheaf().
> > > 
> > > Looking at this a bit more, after checking the current callers, it seems that
> > > the existing callers of kmem_cache_refill_sheaf() are not relying on the sheaf
> > > remaining intact on refill failure.
> > > 
> > > If so, then another possible option might be to update the comment for
> > > kmem_cache_refill_sheaf() to match the current behavior, rather than adding
> > > rollback logic.
> > 
> > I agree with this option. Having possibly more objects than before the call
> > shouldn't be an issue for the callers.
> 
> +1 for this!

Great! Thanks to both of you for confirming!

-- 
Thanks,
Hao