From: Lance Yang <lance.yang@linux.dev>
To: Wei Yang <richard.weiyang@gmail.com>, SeongJae Park <sj@kernel.org>
Cc: akpm@linux-foundation.org, david@redhat.com,
lorenzo.stoakes@oracle.com, ziy@nvidia.com,
baolin.wang@linux.alibaba.com, Liam.Howlett@oracle.com,
npache@redhat.com, ryan.roberts@arm.com, dev.jain@arm.com,
baohua@kernel.org, xu.xin16@zte.com.cn, linux-mm@kvack.org,
Kiryl Shutsemau <kirill@shutemov.name>
Subject: Re: [Patch v2 2/2] mm/khugepaged: remove definition of struct khugepaged_mm_slot
Date: Mon, 22 Sep 2025 00:07:32 +0800 [thread overview]
Message-ID: <c4df52c1-f873-4e2c-ae7d-f66bcf6e2fbb@linux.dev> (raw)
In-Reply-To: <20250920115233.81851-1-sj@kernel.org>
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.
Passing this new pointer to collect_mm_slot() then causes a
use-after-free on the following iteration, IIUC.
Cheers,
Lance
>> }
>>
>> return progress;
>> @@ -2600,7 +2585,7 @@ static void khugepaged_wait_work(void)
>>
>> static int khugepaged(void *none)
>> {
>> - struct khugepaged_mm_slot *mm_slot;
>> + struct mm_slot *slot;
>>
>> set_freezable();
>> set_user_nice(current, MAX_NICE);
>> @@ -2611,10 +2596,10 @@ static int khugepaged(void *none)
>> }
>>
>> spin_lock(&khugepaged_mm_lock);
>> - mm_slot = khugepaged_scan.mm_slot;
>> + slot = khugepaged_scan.mm_slot;
>> khugepaged_scan.mm_slot = NULL;
>> - if (mm_slot)
>> - collect_mm_slot(mm_slot);
>> + if (slot)
>> + collect_mm_slot(slot);
>> spin_unlock(&khugepaged_mm_lock);
>> return 0;
>> }
>> --
>> 2.34.1
>>
>>
>>
>
> On latest mm-new tree, I am getting below error while building UML mode kernel
> for kunit. And 'git bisect' points me this patch. I'm not familiar with this
> code and have no time to dive deep for now, so reporting first.
>
> Oops: general protection fault, probably for non-canonical adI
> [ 356.456907] CPU: 34 UID: 0 PID: 309 Comm: khugepaged Not tainted 6.17.0-rc4+ #370 PREEMPT(voluntary)
> [ 356.457702] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-4.el9 04/01/2014
> [ 356.458484] RIP: 0010:collect_mm_slot (mm/khugepaged.c:1427)
> [ 356.458904] Code: 48 89 df 5b e9 1a 29 f3 ff 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 908
>
> Code starting with the faulting instruction
> ===========================================
> 0: 48 89 df mov %rbx,%rdi
> 3: 5b pop %rbx
> 4: e9 1a 29 f3 ff jmp 0xfffffffffff32923
> 9: 66 2e 0f 1f 84 00 00 cs nopw 0x0(%rax,%rax,1)
> 10: 00 00 00
> 13: 90 nop
> 14: 90 nop
> 15: 90 nop
> 16: 90 nop
> 17: 90 nop
> 18: 90 nop
> 19: 90 nop
> 1a: 90 nop
> 1b: 90 nop
> 1c: 90 nop
> 1d: 08 .byte 0x8
> [ 356.460685] RSP: 0018:ffffb61a46e37df8 EFLAGS: 00010286
> [ 356.461115] RAX: e1bca96613f6fe2b RBX: 0000000000000000 RCX: 8000000000000007
> [ 356.461692] RDX: 0000000000000001 RSI: ffffeba0443b2600 RDI: e1bca96613f6fe2b
> [ 356.462269] RBP: 00000000000000f2 R08: ffff8ea80ec9aa00 R09: 0000000080150001
> [ 356.462842] R10: 000000008015000e R11: 0000000000000000 R12: ffff8ea80ec9aa00
> [ 356.463574] R13: 00000000000001e5 R14: 0000000000000001 R15: ffffb61a46e37e60
> [ 356.464249] FS: 0000000000000000(0000) GS:ffff8eaf13dd1000(0000) knlGS:0000000000000000
> [ 356.465070] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 356.465578] CR2: 00007f8e913e2a1c CR3: 00000008cb022000 CR4: 00000000000006f0
> [ 356.466185] Call Trace:
> [ 356.466398] <TASK>
> [ 356.466576] khugepaged (mm/khugepaged.c:2519 mm/khugepaged.c:2556 mm/khugepaged.c:2612)
> [ 356.466869] ? __pfx_khugepaged (mm/khugepaged.c:2605)
> [ 356.467284] kthread (kernel/kthread.c:463)
> [ 356.467592] ? finish_task_switch.isra.0 (arch/x86/include/asm/paravirt.h:671 kernel/sched/sched.h:1531 kernel/sched/core.c:5105 kernel/sched/core.c:5223)
> [ 356.468068] ? __pfx_kthread (kernel/kthread.c:412)
> [ 356.468480] ret_from_fork (arch/x86/kernel/process.c:154)
> [ 356.468849] ? __pfx_kthread (kernel/kthread.c:412)
> [ 356.469223] ret_from_fork_asm (arch/x86/entry/entry_64.S:258)
> [ 356.469591] </TASK>
> [ 356.469778] Modules linked in: binfmt_misc ppdev parport_pc parport pcspkr evdev joydev button serio_raw sgn
> [ 356.473304] Dumping ftrace buffer:
> [ 356.473618] (ftrace buffer empty)
> [ 356.473966] ---[ end trace 0000000000000000 ]---
> [ 356.474506] RIP: 0010:collect_mm_slot (mm/khugepaged.c:1427)
> [ 356.475142] Code: 48 89 df 5b e9 1a 29 f3 ff 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 908
>
> Code starting with the faulting instruction
> ===========================================
> 0: 48 89 df mov %rbx,%rdi
> 3: 5b pop %rbx
> 4: e9 1a 29 f3 ff jmp 0xfffffffffff32923
> 9: 66 2e 0f 1f 84 00 00 cs nopw 0x0(%rax,%rax,1)
> 10: 00 00 00
> 13: 90 nop
> 14: 90 nop
> 15: 90 nop
> 16: 90 nop
> 17: 90 nop
> 18: 90 nop
> 19: 90 nop
> 1a: 90 nop
> 1b: 90 nop
> 1c: 90 nop
> 1d: 08 .byte 0x8
> [ 356.478405] RSP: 0018:ffffb61a46e37df8 EFLAGS: 00010286
> [ 356.478935] RAX: e1bca96613f6fe2b RBX: 0000000000000000 RCX: 8000000000000007
> [ 356.479763] RDX: 0000000000000001 RSI: ffffeba0443b2600 RDI: e1bca96613f6fe2b
> [ 356.480722] RBP: 00000000000000f2 R08: ffff8ea80ec9aa00 R09: 0000000080150001
> [ 356.481703] R10: 000000008015000e R11: 0000000000000000 R12: ffff8ea80ec9aa00
> [ 356.482402] R13: 00000000000001e5 R14: 0000000000000001 R15: ffffb61a46e37e60
> [ 356.483060] FS: 0000000000000000(0000) GS:ffff8eaf13dd1000(0000) knlGS:0000000000000000
> [ 356.484027] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 356.484861] CR2: 00007f8e913e2a1c CR3: 00000008cb022000 CR4: 00000000000006f0
> [ 356.485559] note: khugepaged[309] exited with preempt_count 1
>
>
> Thanks,
> SJ
next prev parent reply other threads:[~2025-09-21 16:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-19 7:12 [Patch v2 0/2] mm_slot: fix the usage of mm_slot_entry Wei Yang
2025-09-19 7:12 ` [Patch v2 1/2] mm/ksm: get mm_slot by mm_slot_entry() when slot is !NULL Wei Yang
2025-09-19 7:24 ` David Hildenbrand
2025-09-19 7:38 ` Dev Jain
2025-09-19 7:44 ` Lance Yang
2025-09-19 7:12 ` [Patch v2 2/2] mm/khugepaged: remove definition of struct khugepaged_mm_slot Wei Yang
2025-09-19 7:36 ` David Hildenbrand
2025-09-22 13:17 ` Nico Pache
2025-09-20 11:52 ` SeongJae Park
2025-09-20 12:29 ` Wei Yang
2025-09-20 13:41 ` SeongJae Park
2025-09-21 15:08 ` Wei Yang
2025-09-22 9:33 ` SeongJae Park
2025-09-21 16:07 ` Lance Yang [this message]
2025-09-22 0:28 ` Wei Yang
2025-09-22 9:37 ` SeongJae Park
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c4df52c1-f873-4e2c-ae7d-f66bcf6e2fbb@linux.dev \
--to=lance.yang@linux.dev \
--cc=Liam.Howlett@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=baohua@kernel.org \
--cc=baolin.wang@linux.alibaba.com \
--cc=david@redhat.com \
--cc=dev.jain@arm.com \
--cc=kirill@shutemov.name \
--cc=linux-mm@kvack.org \
--cc=lorenzo.stoakes@oracle.com \
--cc=npache@redhat.com \
--cc=richard.weiyang@gmail.com \
--cc=ryan.roberts@arm.com \
--cc=sj@kernel.org \
--cc=xu.xin16@zte.com.cn \
--cc=ziy@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox