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/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 ;)

> What you are currently doing is just ensuring that if slot == NULL, then
> we get a legal value of mm_slot, and since the code then operates on
> the basis of mm_slot, everything will work fine. I really think this
> is a workaround to the real issue that you got slot == NULL :)

Anyway, an actual fix like "if (!slot) return" for khugepaged would
be better than this workaround, as Dev mentioned.

Cheers,
Lance

> 
>>
>> Current code works since mm_slot is the first element, but make sure it
>> won't be disturbed.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>   mm/khugepaged.c | 5 ++++-
>>   mm/ksm.c        | 5 ++++-
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index af5f5c80fe4e..668e74ad33b7 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -108,7 +108,7 @@ struct collapse_control {
>>    * @slot: hash lookup from mm to mm_slot
>>    */
>>   struct khugepaged_mm_slot {
>> -     struct mm_slot slot;
>> +     struct mm_slot slot;    /* keep it the first element */
>>   };
>>
>>   /**
>> @@ -2382,6 +2382,9 @@ static unsigned int 
>> khugepaged_scan_mm_slot(unsigned int pages, int *result,
>>       struct vm_area_struct *vma;
>>       int progress = 0;
>>
>> +     BUILD_BUG_ON_MSG(mm_slot_entry(NULL, struct khugepaged_mm_slot, 
>> slot),
>> +                     "slot should be the first element");
>> +
>>       VM_BUG_ON(!pages);
>>       lockdep_assert_held(&khugepaged_mm_lock);
>>       *result = SCAN_FAIL;
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 2ef29802a49b..0d486dbdf7d3 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -124,7 +124,7 @@ typedef u8 rmap_age_t;
>>    * @rmap_list: head for this mm_slot's singly-linked list of rmap_items
>>    */
>>   struct ksm_mm_slot {
>> -     struct mm_slot slot;
>> +     struct mm_slot slot;    /* keep it the first element */
>>       struct ksm_rmap_item *rmap_list;
>>   };
>>
>> @@ -3842,6 +3842,9 @@ static int __init ksm_init(void)
>>       struct task_struct *ksm_thread;
>>       int err;
>>
>> +     BUILD_BUG_ON_MSG(mm_slot_entry(NULL, struct ksm_mm_slot, slot),
>> +                     "slot should be the first element");
>> +
>>       /* The correct value depends on page size and endianness */
>>       zero_checksum = calc_checksum(ZERO_PAGE(0));
>>       /* Default to false for backwards compatibility */
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose the 
> contents to any other person, use it for any purpose, or store or copy 
> the information in any medium. Thank you.