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

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

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.