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 09:42, Wei Yang wrote:
> On Mon, Sep 15, 2025 at 09:32:27AM +0200, David Hildenbrand wrote:
>> On 15.09.25 03:47, Wei Yang wrote:
>>> 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.
>>>
>>
>> We should probably just use a static_assert right nxt to the struct and make
>> sure that offsetof(struct khugepaged_mm_slot, slot) == 0 ?
>>
> 
> I am ok with this, but Dev suggest to fix in the code.
> 
> Hmm... both works to me.

Right, if it can just be made working without requiring it to be the 
first entry, we should do that instead.

-- 
Cheers

David / dhildenb