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

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

[Wei Yang]

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.

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");
+
 	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 */
-- 
2.34.1

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

[Lance Yang]

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()?

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 */

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

[Dev Jain]

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.
What you are currently doing is just ensuring that if slot == NULL, then
we get a legal value of mm_slot, and since the code then operates on
the basis of mm_slot, everything will work fine. I really think this
is a workaround to the real issue that you got slot == NULL :)

>
> Current code works since mm_slot is the first element, but make sure it
> won't be disturbed.
>
> 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");
> +
>       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 */
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.

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

[Lance Yang]

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

> What you are currently doing is just ensuring that if slot == NULL, then
> we get a legal value of mm_slot, and since the code then operates on
> the basis of mm_slot, everything will work fine. I really think this
> is a workaround to the real issue that you got slot == NULL :)

Anyway, an actual fix like "if (!slot) return" for khugepaged would
be better than this workaround, as Dev mentioned.

Cheers,
Lance

> 
>>
>> Current code works since mm_slot is the first element, but make sure it
>> won't be disturbed.
>>
>> 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");
>> +
>>       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 */
> 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.

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

[Dev Jain]

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


>
>> What you are currently doing is just ensuring that if slot == NULL, then
>> we get a legal value of mm_slot, and since the code then operates on
>> the basis of mm_slot, everything will work fine. I really think this
>> is a workaround to the real issue that you got slot == NULL :)
>
> Anyway, an actual fix like "if (!slot) return" for khugepaged would
> be better than this workaround, as Dev mentioned.
>
> Cheers,
> Lance
>
>>
>>>
>>> Current code works since mm_slot is the first element, but make sure it
>>> won't be disturbed.
>>>
>>> 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");
>>> +
>>>       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 */
>> 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.
>
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.

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

[Lance Yang]

On 2025/9/14 15:29, 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 :)

Yep, you're spot on :)

[...]

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

[xu.xin16]

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

The same to KSM. The slot passed into mm_slot_entry() could not be NULL. Even if NULL,

kernel would panic due to NULL dereference in the subsequent procedure, but it has never

happened yet. 

So the patch seems little significant for KSM.

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

[Wei Yang]

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

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

[Wei Yang]

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?

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

-- 
Wei Yang
Help you, Help me

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

[Dev Jain]

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.

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

[Dev Jain]

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

No idea why this still shows up. Sorry for this, lemme do something.

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.

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

[David Hildenbrand]

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 ?

-- 
Cheers

David / dhildenb

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

[Wei Yang]

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.

>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

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

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

[David Hildenbrand]

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

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

[Wei Yang]

On Mon, Sep 15, 2025 at 09:48:55AM +0200, David Hildenbrand wrote:
>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.
>

Got it, thanks.

>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[David Hildenbrand]

On 14.09.25 09:29, 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 :)

I agree. If mm_slot_lookup() returns NULL we just just handle that cleanly like

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 6b40bdfd224c3..70a32d59d7d2f 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
  
         spin_lock(&khugepaged_mm_lock);
         slot = mm_slot_lookup(mm_slots_hash, mm);
-       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
-       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
-               hash_del(&slot->hash);
-               list_del(&slot->mm_node);
-               free = 1;
+       if (slot) {
+               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
+               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
+                       hash_del(&slot->hash);
+                       list_del(&slot->mm_node);
+                       free = 1;
+               }
         }
         spin_unlock(&khugepaged_mm_lock);
  

If mm_slot_lookup() is not expected to ever return NULL, then a VM_WARN_ON_ONCE
might be sufficient to document that this is guaranteed.

IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case

(a) test_and_set_bit() succeeds

but

(b) mm_slot_alloc() fails

In that case we could get NULL.


It is rather weird to leave the flag set in case mm_slot_alloc() failed ...

-- 
Cheers

David / dhildenb

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

[David Hildenbrand]

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

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

[Lance Yang]

On 2025/9/15 15:56, David Hildenbrand wrote:
> On 14.09.25 09:29, 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 :)
> 
> I agree. If mm_slot_lookup() returns NULL we just just handle that 
> cleanly like
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 6b40bdfd224c3..70a32d59d7d2f 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
> 
>          spin_lock(&khugepaged_mm_lock);
>          slot = mm_slot_lookup(mm_slots_hash, mm);
> -       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
> -       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
> -               hash_del(&slot->hash);
> -               list_del(&slot->mm_node);
> -               free = 1;
> +       if (slot) {
> +               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, 
> slot);
> +               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
> +                       hash_del(&slot->hash);
> +                       list_del(&slot->mm_node);
> +                       free = 1;
> +               }
>          }
>          spin_unlock(&khugepaged_mm_lock);
> 
> 
> If mm_slot_lookup() is not expected to ever return NULL, then a 
> VM_WARN_ON_ONCE
> might be sufficient to document that this is guaranteed.
> 
> IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
> 
> (a) test_and_set_bit() succeeds
> 
> but
> 
> (b) mm_slot_alloc() fails
> 
> In that case we could get NULL.

Ah, good catch! We could indeed get NULL in that case ;)

> 
> 
> It is rather weird to leave the flag set in case mm_slot_alloc() failed ...

Perhaps the MMF_VM_HUGEPAGE flag should be cleared if mm_slot_alloc() fails?

Cheers,
Lance

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

[Dev Jain]

On 15/09/25 1:26 pm, David Hildenbrand wrote:
> On 14.09.25 09:29, 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 :)
>
> I agree. If mm_slot_lookup() returns NULL we just just handle that
> cleanly like
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 6b40bdfd224c3..70a32d59d7d2f 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>
>         spin_lock(&khugepaged_mm_lock);
>         slot = mm_slot_lookup(mm_slots_hash, mm);
> -       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
> -       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
> -               hash_del(&slot->hash);
> -               list_del(&slot->mm_node);
> -               free = 1;
> +       if (slot) {
> +               mm_slot = mm_slot_entry(slot, struct
> khugepaged_mm_slot, slot);
> +               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
> +                       hash_del(&slot->hash);
> +                       list_del(&slot->mm_node);
> +                       free = 1;
> +               }
>         }
>         spin_unlock(&khugepaged_mm_lock);
>
>
> If mm_slot_lookup() is not expected to ever return NULL, then a
> VM_WARN_ON_ONCE
> might be sufficient to document that this is guaranteed.
>
> IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
>
> (a) test_and_set_bit() succeeds
>
> but
>
> (b) mm_slot_alloc() fails
>
> In that case we could get NULL.
>
>
> It is rather weird to leave the flag set in case mm_slot_alloc()
> failed ...

Good spot! We should move the slot allocation line before the test_and_set.


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.

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

[David Hildenbrand]

On 15.09.25 10:11, Dev Jain wrote:
> 
> On 15/09/25 1:26 pm, David Hildenbrand wrote:
>> On 14.09.25 09:29, 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 :)
>>
>> I agree. If mm_slot_lookup() returns NULL we just just handle that
>> cleanly like
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 6b40bdfd224c3..70a32d59d7d2f 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>>
>>          spin_lock(&khugepaged_mm_lock);
>>          slot = mm_slot_lookup(mm_slots_hash, mm);
>> -       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>> -       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>> -               hash_del(&slot->hash);
>> -               list_del(&slot->mm_node);
>> -               free = 1;
>> +       if (slot) {
>> +               mm_slot = mm_slot_entry(slot, struct
>> khugepaged_mm_slot, slot);
>> +               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>> +                       hash_del(&slot->hash);
>> +                       list_del(&slot->mm_node);
>> +                       free = 1;
>> +               }
>>          }
>>          spin_unlock(&khugepaged_mm_lock);
>>
>>
>> If mm_slot_lookup() is not expected to ever return NULL, then a
>> VM_WARN_ON_ONCE
>> might be sufficient to document that this is guaranteed.
>>
>> IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
>>
>> (a) test_and_set_bit() succeeds
>>
>> but
>>
>> (b) mm_slot_alloc() fails
>>
>> In that case we could get NULL.
>>
>>
>> It is rather weird to leave the flag set in case mm_slot_alloc()
>> failed ...
> 
> Good spot! We should move the slot allocation line before the test_and_set.

You'll have to handle races differently then, and free the allocated 
mm_slot if the mm_flags_test_and_set fails I guess.

Maybe you'd want an early check if the flag is already set first.

-- 
Cheers

David / dhildenb

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

[David Hildenbrand]

On 15.09.25 10:08, Lance Yang wrote:
> 
> 
> On 2025/9/15 15:56, David Hildenbrand wrote:
>> On 14.09.25 09:29, 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 :)
>>
>> I agree. If mm_slot_lookup() returns NULL we just just handle that
>> cleanly like
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 6b40bdfd224c3..70a32d59d7d2f 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>>
>>           spin_lock(&khugepaged_mm_lock);
>>           slot = mm_slot_lookup(mm_slots_hash, mm);
>> -       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>> -       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>> -               hash_del(&slot->hash);
>> -               list_del(&slot->mm_node);
>> -               free = 1;
>> +       if (slot) {
>> +               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot,
>> slot);
>> +               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>> +                       hash_del(&slot->hash);
>> +                       list_del(&slot->mm_node);
>> +                       free = 1;
>> +               }
>>           }
>>           spin_unlock(&khugepaged_mm_lock);
>>
>>
>> If mm_slot_lookup() is not expected to ever return NULL, then a
>> VM_WARN_ON_ONCE
>> might be sufficient to document that this is guaranteed.
>>
>> IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
>>
>> (a) test_and_set_bit() succeeds
>>
>> but
>>
>> (b) mm_slot_alloc() fails
>>
>> In that case we could get NULL.
> 
> Ah, good catch! We could indeed get NULL in that case ;)
> 
>>
>>
>> It is rather weird to leave the flag set in case mm_slot_alloc() failed ...
> 
> Perhaps the MMF_VM_HUGEPAGE flag should be cleared if mm_slot_alloc() fails?

Clearing might also be nasty with concurrent readers that expect that 
it's setup.

It's all rather nasty here, because we simply swallow allocation errors 
essentially and silently end up not scanning that MM through khugepaged...

-- 
Cheers

David / dhildenb

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

[Wei Yang]

On Mon, Sep 15, 2025 at 09:56:34AM +0200, David Hildenbrand wrote:
>On 14.09.25 09:29, 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 :)
>
>I agree. If mm_slot_lookup() returns NULL we just just handle that cleanly like
>
>diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>index 6b40bdfd224c3..70a32d59d7d2f 100644
>--- a/mm/khugepaged.c
>+++ b/mm/khugepaged.c
>@@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>        spin_lock(&khugepaged_mm_lock);
>        slot = mm_slot_lookup(mm_slots_hash, mm);
>-       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>-       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>-               hash_del(&slot->hash);
>-               list_del(&slot->mm_node);
>-               free = 1;
>+       if (slot) {
>+               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>+               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {

Ok I got the plan.

One nit, we don't need to check mm_slot here, right?

>+                       hash_del(&slot->hash);
>+                       list_del(&slot->mm_node);
>+                       free = 1;
>+               }
>        }
>        spin_unlock(&khugepaged_mm_lock);
>
>If mm_slot_lookup() is not expected to ever return NULL, then a VM_WARN_ON_ONCE
>might be sufficient to document that this is guaranteed.
>
>IIUC, MMF_VM_HUGEPAGE might be set in __khugepaged_enter() in case
>
>(a) test_and_set_bit() succeeds
>
>but
>
>(b) mm_slot_alloc() fails
>
>In that case we could get NULL.
>
>
>It is rather weird to leave the flag set in case mm_slot_alloc() failed ...

Yes...

>
>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Mon, Sep 15, 2025 at 09:56:34AM +0200, David Hildenbrand wrote:
[...]
>diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>index 6b40bdfd224c3..70a32d59d7d2f 100644
>--- a/mm/khugepaged.c
>+++ b/mm/khugepaged.c
>@@ -488,11 +488,13 @@ void __khugepaged_exit(struct mm_struct *mm)
>        spin_lock(&khugepaged_mm_lock);
>        slot = mm_slot_lookup(mm_slots_hash, mm);
>-       mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>-       if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>-               hash_del(&slot->hash);
>-               list_del(&slot->mm_node);
>-               free = 1;
>+       if (slot) {
>+               mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
>+               if (mm_slot && khugepaged_scan.mm_slot != mm_slot) {
>+                       hash_del(&slot->hash);
>+                       list_del(&slot->mm_node);
>+                       free = 1;
>+               }
>        }

But... to be honest, for khugepaged_mm_slot, I'd prefer to remove the
definition of it. struct khugepaged_mm_slot just wrap struct mm_slot.

One un-convenient thing of this change is we cant use KMEM_CACHE() to define
the slab allocator. Have to convert to the old api.

-- 
Wei Yang
Help you, Help me

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

[Kiryl Shutsemau]

On Sun, Sep 14, 2025 at 12:00:26AM +0000, 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.

Seems excessive for this kind of problem.

Why not check slot in mm_slot_entry?

diff --git a/mm/mm_slot.h b/mm/mm_slot.h
index 83f18ed1c4bd..0de24c490b62 100644
--- a/mm/mm_slot.h
+++ b/mm/mm_slot.h
@@ -19,7 +19,7 @@ struct mm_slot {
 };

 #define mm_slot_entry(ptr, type, member) \
-	container_of(ptr, type, member)
+	(ptr ? container_of(ptr, type, member) : NULL)

 static inline void *mm_slot_alloc(struct kmem_cache *cache)
 {
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Wei Yang]

On Mon, Sep 15, 2025 at 10:39:28AM +0100, Kiryl Shutsemau wrote:
>On Sun, Sep 14, 2025 at 12:00:26AM +0000, 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.
>
>Seems excessive for this kind of problem.
>
>Why not check slot in mm_slot_entry?
>
>diff --git a/mm/mm_slot.h b/mm/mm_slot.h
>index 83f18ed1c4bd..0de24c490b62 100644
>--- a/mm/mm_slot.h
>+++ b/mm/mm_slot.h
>@@ -19,7 +19,7 @@ struct mm_slot {
> };
>
> #define mm_slot_entry(ptr, type, member) \
>-	container_of(ptr, type, member)
>+	(ptr ? container_of(ptr, type, member) : NULL)
>

Hmm... I  have thought about this. But not choose this one without some strong
reason.

I am fine with this too...

> static inline void *mm_slot_alloc(struct kmem_cache *cache)
> {
>-- 
>  Kiryl Shutsemau / Kirill A. Shutemov

-- 
Wei Yang
Help you, Help me