Re: [PATCH] mm/slub: fix lost local objects when bulk remote free batch fills

[hu.shengming]

Harry wrote:
> On 7/5/26 10:37 PM, hu.shengming@xxxxxxxxxx wrote:
> > Harry wrote:
> >> Hi Shengming,
> > 
> > Hi Harry,
> > 
> >> On 7/4/26 5:04 PM, hu.shengming@xxxxxxxxxx wrote:
> >>> From: Shengming Hu <hu.shengming@xxxxxxxxxx>
> >>>
> >>> In free_to_pcs_bulk(), when remote_objects[] fills to PCS_BATCH_MAX,
> >>> the code jumps to flush_remote to free the batch. If all remote entries
> >>> have already been compacted out of p[] via tail swaps while local objects
> >>> remain, the flush_remote path returns early since `i < size` no longer
> >>> holds. The leftover local objects are then neither cached in the sheaf 
> >>> nor returned to the slab freelist, causing a memory leak.
> >>
> >> Ouch.
> >>
> >> Interestingly, we fixed a similar memory leak in free_to_pcs_bulk()
> >> during the v6.18 cycle; commit cbcff934fa7d ("mm/slub: fix memory leak
> >> in free_to_pcs_bulk()").
> >>
> >> Hmm, I feel is a signal that the control flow is quite complicated and
> >> worth revisiting if we could simplify the logic there. I'm not saying we
> >> should do that to fix the bug, just mentioning.
> > 
> > Yes, this is also a memory leak. I agree that the control flow in
> > free_to_pcs_bulk() is fairly subtle, especially considering the previous
> > leak fix.
> > 
> > My intention with this patch is only to remove this problematic path with
> > a localized change.
> 
> That's fair!
> 
> >>> For illustration:
> >>>  size = 64, local objects at p[0..31], remote objects at p[32..63]
> >>>  After scanning all remotes: i = 32, size = 32
> >>>  p[0..31] local objects are dropped.
> >>
> >> I can see that could indeed happen.
> >> Interesting. How has it been undiscovered until today?
> >>
> >> Well, for this to trigger, at least PCS_BATCH_MAX (hardcoded to 32)
> >> objects in the sheaf should be from remote nodes.
> >>
> >> Looking at kmem_cache_free_bulk() users:
> >> - maple_node has sheaf_capacity = 32
> >> - skbuff_head_cache has sheaf_capacity = 28
> >> - panthor and msm drivers have sheaf_capacity = 4
> >>
> >> The capacity is (at least for now) purely based on the object size
> >> (with user-requested capacity as a minimum)
> >>
> >> Only maple_node cache (out of four users) has
> >> sheaf_capacity >= PCS_BATCH_MAX.
> >>
> >> However, for this to trigger in maple_node cache, ALL objects in the
> >> sheaf should have been from remote nodes. And that means there is no
> >> local object to leak :)
> >>
> >> So, although the logic has a leak, you cannot trigger this in reality yet.
> >>
> > 
> > Thanks for the detailed analysis!
> > 
> > Yes, this was found by code review rather than from a runtime report, and
> > I can not verified it with an actual triggering workload.
> >
> > I agree that it does not seem triggerable with the current users. This fix
> > is mainly intended to make the code robust against possible future users or
> > changes to PCS_BATCH_MAX  :)
> 
> Could we have these explanations/analysis above
> in the commit message please? :)
> 

Sure, I'll fold this full analysis into the commit message in v2.  :)

> But yeah, we should fix this.
> 
> When fixing a bug usually I prefer to go through the process...
> 
>  1) Confirm the bug exists by reproducing it, and
>  2) Confirm that the patch indeed fixes the bug
> 
> But this one is impossible to reproduce, uh.
> 
> In this case I think it's better not to Cc stable (which is already the
> case). In case there is an error in the analysis, it's still
> straightforward to backport later.

Agreed. No stable Cc for now given no known real-world trigger path; it can
be backported easily later if needed.

> I will double check if the patch is indeed correct.
> 
> Thanks!

Thanks a lot for reviewing and double-checking!

> >>> Replace the goto-based flush with an in-place free of full remote batches
> >>> during the scan, then resume processing the compacted array. All local
> >>> objects now go through the normal fast path, and the tail path only
> >>> handles any leftover partial remote batch. The redundant next_remote_batch
> >>> jump label is removed as well.
> >>>
> >>> Fixes: <989b09b73978>("slab: skip percpu sheaves for remote object freeing")
> >>> Signed-off-by: Shengming Hu <hu.shengming@xxxxxxxxxx

--
With Best Regards,
Shengming