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

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

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

VM_BUG_ON() seems too heavy.

-- 
Wei Yang
Help you, Help me