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

[Harry Yoo]

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? :)

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.

I will double check if the patch is indeed correct.

Thanks!

>>> 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

-- 
Cheers,
Harry / Hyeonggon

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature