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@xxxxxxxxxxxxxxx
> 
> And therefore we don't, unless I'm mistaken.

You're right.

> >> Signed-off-by: Qing Wang <wangqing7171@xxxxxxxxx>
> >> ---
> >>  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