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

[Dev Jain <dev.jain@arm.com>]

On 15/09/25 7:27 am, Wei Yang wrote:
> On Sun, Sep 14, 2025 at 12:59:33PM +0530, 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 :)
>>
> Ok, if so, I think this should be a patch with Fixes tag and cc stable? And it
> supposed to be two patches, since it fixes two different commit. Am I right?

I don't think this is a fix which needs to be backported, the current code works.
What we need is a VM_BUG_ON() or a WARN_ON_ONCE() when slot == NULL, because that
just shouldn't happen, and then safely exit.

>
> Originally, I though we don't want that bother :-)
>
> And now we decide to fix this, I am wondering why we wrap mm_slot with
> khugepaged_mm_slot. I don't think it is necessary to create a new data
> structure to just wrap another. And this is why the problem comes.
>
>    commit b26e27015e mm: thp: convert to common struct mm_slot

Yeah I always wondered why we had that wrapper, makes things unnecessarily
complicated.

>
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.