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:11, Dev Jain wrote:
> 
> On 15/09/25 1:26 pm, 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.
>>
>>
>> It is rather weird to leave the flag set in case mm_slot_alloc()
>> failed ...
> 
> Good spot! We should move the slot allocation line before the test_and_set.

You'll have to handle races differently then, and free the allocated 
mm_slot if the mm_flags_test_and_set fails I guess.

Maybe you'd want an early check if the flag is already set first.

-- 
Cheers

David / dhildenb