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:46, Wei Yang wrote:
> On Mon, Sep 15, 2025 at 09:19:55AM +0530, Dev Jain wrote:
>>
>> 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.
>>
> 
> You mean sth like this?
> 
> @@ -2940,7 +2940,7 @@ void __ksm_exit(struct mm_struct *mm)
>          spin_lock(&ksm_mmlist_lock);
>          slot = mm_slot_lookup(mm_slots_hash, mm);
>          mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
> -       if (mm_slot && ksm_scan.mm_slot != mm_slot) {
> +       if (WARN_ON_ONCE(slot) && ksm_scan.mm_slot != mm_slot) {
>                  if (!mm_slot->rmap_list) {
>                          hash_del(&slot->hash);
>                          list_del(&slot->mm_node);

See my other reply, I think this can happen if mm_slot_alloc() fails.

-- 
Cheers

David / dhildenb