On 2025/1/10 01:00, Yang Shi wrote:
On 1/8/25 11:00 PM, Liu Shixin wrote:
syzkaller reported such a BUG_ON():
------------[ cut here ]------------
kernel BUG at mm/khugepaged.c:1835!
Internal error: Oops - BUG: 00000000f2000800 [#1] SMP
...
CPU: 6 UID: 0 PID: 8009 Comm: syz.15.106 Kdump: loaded Tainted:
G W 6.13.0-rc6 #22
Tainted: [W]=WARN
Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : collapse_file+0xa44/0x1400
lr : collapse_file+0x88/0x1400
sp : ffff80008afe3a60
...
Call trace:
collapse_file+0xa44/0x1400 (P)
hpage_collapse_scan_file+0x278/0x400
madvise_collapse+0x1bc/0x678
madvise_vma_behavior+0x32c/0x448
madvise_walk_vmas.constprop.0+0xbc/0x140
do_madvise.part.0+0xdc/0x2c8
__arm64_sys_madvise+0x68/0x88
invoke_syscall+0x50/0x120
el0_svc_common.constprop.0+0xc8/0xf0
do_el0_svc+0x24/0x38
el0_svc+0x34/0x128
el0t_64_sync_handler+0xc8/0xd0
el0t_64_sync+0x190/0x198
This indicates that the pgoff is unaligned. After analysis, I confirm
the vma is mapped to /dev/zero. Such a vma certainly has vm_file, but
it is set to anonymous by mmap_zero(). So even if it's mmapped by
2m-unaligned, it can pass the check in thp_vma_allowable_order() as it
is an anonymous-mmap, but then be collapsed as a file-mmap.
It seems the problem has existed for a long time, but actually, since
we have khugepaged_max_ptes_none check before, we will skip collapse it
as it is /dev/zero and so has no present page. But commit d8ea7cc8547c
limit the check for only khugepaged, so the BUG_ON() can be triggered
by madvise_collapse().
Add vma_is_anonymous() check to make such vma be processed by
hpage_collapse_scan_pmd().
Fixes: d8ea7cc8547c ("mm/khugepaged: add flag to predicate
khugepaged-only behavior")
Signed-off-by: Liu Shixin <liushixin2@xxxxxxxxxx>
---
mm/khugepaged.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 653dbb1ff05c..eb9d240e42e8 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2422,7 +2422,8 @@ static unsigned int
khugepaged_scan_mm_slot(unsigned int pages, int *result,
VM_BUG_ON(khugepaged_scan.address < hstart ||
khugepaged_scan.address + HPAGE_PMD_SIZE >
hend);
- if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) {
+ if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file &&
+ !vma_is_anonymous(vma)) {
Thanks for catching this. It sounds a little bit weird to have vm_file
for an anonymous VMA. I'm not sure why we should keep such special case.
It seems shared mapping is treated as shmem file mapping. So can we set
vm_file to NULL when mmap'ing /dev/zero for private mapping? Something
like:
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 169eed162a7f..fc332efc5c11 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -527,6 +527,7 @@ static int mmap_zero(struct file *file, struct
vm_area_struct *vma)
if (vma->vm_flags & VM_SHARED)
return shmem_zero_setup(vma);
vma_set_anonymous(vma);
+ vma->vm_file = NULL;
return 0;
}
Yes, this makes more sense to me.
