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

[Lance Yang <lance.yang@linux.dev>]

On 2025/9/15 15:56, 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) {
> +                       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.

Ah, good catch! We could indeed get NULL in that case ;)

> 
> 
> It is rather weird to leave the flag set in case mm_slot_alloc() failed ...

Perhaps the MMF_VM_HUGEPAGE flag should be cleared if mm_slot_alloc() fails?

Cheers,
Lance