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

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

On Sun, Sep 14, 2025 at 01:05:48PM +0800, Lance Yang wrote:
>Hi Wei,
>
>On 2025/9/14 08:00, 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.
>> 
>> Current code works since mm_slot is the first element, but make sure it
>> won't be disturbed.
>
>Good catch! That's indeed quite brittle ;)
>
>Just one nit below.
>
>> 
>> 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");
>> +
>
>I wonder if this BUILD_BUG_ON_MSG() in khugepaged_scan_mm_slot() would
>be better placed in khugepaged_init(), like you did in ksm_init()?

khugepaged_mm_slot is defined in khugepaged.c, maybe we don't want to export
it.

>
>Cheers,
>Lance
>
>>   	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 */

-- 
Wei Yang
Help you, Help me