Re: [PATCH] mm/mm_slot: make sure slot is the first element of its wrapper structure

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Sep 15, 2025 at 09:56:34AM +0200, David Hildenbrand wrote:
>On 14.09.25 09:29, Dev Jain wrote:
>> 
>> On 14/09/25 12:33 pm, Lance Yang wrote:
>> > 
>> > 
>> > On 2025/9/14 14:21, Dev Jain wrote:
>> > > 
>> > > On 14/09/25 5:30 am, Wei Yang wrote:
>> > > > When using mm_slot in ksm/khugepaged, there is code snip like:
>> > > > 
>> > > >        slot = mm_slot_lookup(mm_slots_hash, mm);
>> > > >        mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
>> > > >        if (mm_slot && ..) {
>> > > >        }
>> > > > 
>> > > > This is only valid when mm_slot is the first element of its wrapper
>> > > > structure, otherwise a NULL slot would converted to a mm_slot with
>> > > > negative value. And current code thinks it is valid and continue.
>> > > 
>> > > Shouldn't you fix the code for the case when you can't find the slot
>> > > in the hashtable, i.e slot == NULL? Like, if (!slot) return.
>> > 
>> > Right. For khugepaged specifically, the slot == NULL case in
>> > __khugepaged_exit() (only user of mm_slot_lookup) should probably
>> > be treated as a kernel BUG for new.
>> > 
>> > But I'm not sure if the same logic applies to KSM ;)
>> 
>> I haven't seen the KSM analogue, but restricting the position of an element
>> 
>> in a struct to make the code work should imply that the code is wrong in
>> 
>> the first place :)
>
>I agree. If mm_slot_lookup() returns NULL we just just handle that cleanly like
>
>diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>index 6b40bdfd224c3..70a32d59d7d2f 100644
>--- a/mm/khugepaged.c
>+++ b/mm/khugepaged.c
>@@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>        spin_lock(&khugepaged_mm_lock);
>        slot = mm_slot_lookup(mm_slots_hash, mm);
>-       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>-       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>-               hash_del(&slot->hash);
>-               list_del(&slot->mm_node);
>-               free = 1;
>+       if (slot) {
>+               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>+               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {

Ok I got the plan.

One nit, we don't need to check mm_slot here, right?

>+                       hash_del(&slot->hash);
>+                       list_del(&slot->mm_node);
>+                       free = 1;
>+               }
>        }
>        spin_unlock(&khugepaged_mm_lock);
>
>If mm_slot_lookup() is not expected to ever return NULL, then a VM_WARN_ON_ONCE
>might be sufficient to document that this is guaranteed.
>
>IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
>
>(a) test_and_set_bit() succeeds
>
>but
>
>(b) mm_slot_alloc() fails
>
>In that case we could get NULL.
>
>
>It is rather weird to leave the flag set in case mm_slot_alloc() failed ...

Yes...

>
>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me