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

[Lance Yang <lance.yang@linux.dev>]

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