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

[David Hildenbrand <david@redhat.com>]

On 15.09.25 10:08, Lance Yang wrote:
> 
> 
> 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?

Clearing might also be nasty with concurrent readers that expect that 
it's setup.

It's all rather nasty here, because we simply swallow allocation errors 
essentially and silently end up not scanning that MM through khugepaged...

-- 
Cheers

David / dhildenb