Re: [Patch v2 2/2] mm/khugepaged: remove definition of struct khugepaged_mm_slot

[SeongJae Park <sj@kernel.org>]

On Mon, 22 Sep 2025 00:28:34 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> On Mon, Sep 22, 2025 at 12:07:32AM +0800, Lance Yang wrote:
> >Good catch!
> >
> >Looking at the crash report, this seems like a use-after-free bug
> >introduced in khugepaged_scan_mm_slot(). See below please.
> >
> >On 2025/9/20 19:52, SeongJae Park wrote:
> >> Hello,
> >> 
> >> On Fri, 19 Sep 2025 07:12:44 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
> >> 
> >> > Current code is not correct to get struct khugepaged_mm_slot by
> >> > mm_slot_entry() without checking mm_slot is !NULL. There is no problem
> >> > reported since slot is the first element of struct khugepaged_mm_slot.
> >> > 
> >> > While struct khugepaged_mm_slot is just a wrapper of struct mm_slot,
> >> > there is no need to define it.
> >> > 
> >> > Remove the definition of struct khugepaged_mm_slot, so there is not
> >> > chance to miss use mm_slot_entry().
> >> > 
> >> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >> > Cc: Lance Yang <lance.yang@linux.dev>
> >> > Cc: David Hildenbrand <david@redhat.com>
> >> > Cc: Dev Jain <dev.jain@arm.com>
> >> > Cc: Kiryl Shutsemau <kirill@shutemov.name>
> >> > Cc: xu.xin16@zte.com.cn
> >> > ---
> >> >   mm/khugepaged.c | 57 ++++++++++++++++++-------------------------------
> >> >   1 file changed, 21 insertions(+), 36 deletions(-)
> >> > 
> >> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> > index e019ea2cbab0..88ea92c64bf0 100644
> >> > --- a/mm/khugepaged.c
> >> > +++ b/mm/khugepaged.c
> >> [...]
> >> > @@ -2376,7 +2365,6 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
> >> >   	__acquires(&khugepaged_mm_lock)
> >> >   {
> >> >   	struct vma_iterator vmi;
> >> > -	struct khugepaged_mm_slot *mm_slot;
> >> >   	struct mm_slot *slot;
> >> >   	struct mm_struct *mm;
> >> >   	struct vm_area_struct *vma;
> >> > @@ -2387,14 +2375,12 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
> >> >   	*result = SCAN_FAIL;
> >> >   	if (khugepaged_scan.mm_slot) {
> >> > -		mm_slot = khugepaged_scan.mm_slot;
> >> > -		slot = &mm_slot->slot;
> >> > +		slot = khugepaged_scan.mm_slot;
> >> >   	} else {
> >> >   		slot = list_first_entry(&khugepaged_scan.mm_head,
> >> >   				     struct mm_slot, mm_node);
> >> > -		mm_slot = mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
> >> >   		khugepaged_scan.address = 0;
> >> > -		khugepaged_scan.mm_slot = mm_slot;
> >> > +		khugepaged_scan.mm_slot = slot;
> >> >   	}
> >> >   	spin_unlock(&khugepaged_mm_lock);
> >> > @@ -2492,7 +2478,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
> >> >   breakouterloop_mmap_lock:
> >> >   	spin_lock(&khugepaged_mm_lock);
> >> > -	VM_BUG_ON(khugepaged_scan.mm_slot != mm_slot);
> >> > +	VM_BUG_ON(khugepaged_scan.mm_slot != slot);
> >> >   	/*
> >> >   	 * Release the current mm_slot if this mm is about to die, or
> >> >   	 * if we scanned all vmas of this mm.
> >> > @@ -2505,15 +2491,14 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
> >> >   		 */
> >> >   		if (!list_is_last(&slot->mm_node, &khugepaged_scan.mm_head)) {
> >> >   			slot = list_next_entry(slot, mm_node);
> >
> >In the original code, we used two distinct local variables.
> >
> >1) struct khugepaged_mm_slot *mm_slot:
> >mm_slot consistently pointed to the item being processed in the
> >current call.
> >
> >2) struct mm_slot *slot:
> >The local slot pointer could be advanced to the next item.
> >
> >> > -			khugepaged_scan.mm_slot =
> >> > -				mm_slot_entry(slot, struct khugepaged_mm_slot, slot);
> >> > +			khugepaged_scan.mm_slot = slot;
> >> >   			khugepaged_scan.address = 0;
> >> >   		} else {
> >> >   			khugepaged_scan.mm_slot = NULL;
> >> >   			khugepaged_full_scans++;
> >> >   		}
> >> > -		collect_mm_slot(mm_slot);
> >
> >At the end, collect_mm_slot(mm_slot) correctly operated on the
> >original item for that scan.
> >
> >> > +		collect_mm_slot(slot);
> >
> >However, this patch merges these two into a single slot variable.
> >
> >When slot = list_next_entry(slot, mm_node); is called, the slot
> >pointer is updated to the next item.
> >
> 
> Oops, you are right. Thanks for spotting it.
> 
> @SeongJae, would you mind applying this change and try again?
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index d28d1116e83f..fb517b5ad277 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2508,8 +2508,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
>                  * mm_slot not pointing to the exiting mm.
>                  */
>                 if (!list_is_last(&slot->mm_node, &khugepaged_scan.mm_head)) {
> -                       slot = list_next_entry(slot, mm_node);
> -                       khugepaged_scan.mm_slot = slot;
> +                       khugepaged_scan.mm_slot = list_next_entry(slot, mm_node);
>                         khugepaged_scan.address = 0;
>                 } else {
>                         khugepaged_scan.mm_slot = NULL;

Thank you for the investigation and the fix, Lance and Wei!  I just found the
above change makes my repro quiet.  I didn't have time to read the
investigation and your fix thoroughly, and my repro is not stable, though.


Thanks,
SJ

[...]