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

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

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;

>Passing this new pointer to collect_mm_slot() then causes a
>use-after-free on the following iteration, IIUC.
>
>Cheers,
>Lance

-- 
Wei Yang
Help you, Help me