| Message ID | 20250109070059.369257-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: khugepaged: fix call hpage_collapse_scan_file() for anonymous vma | expand |
On Thu, Jan 09, 2025 at 03:00:59PM +0800, Liu Shixin wrote: > Add vma_is_anonymous() check to make such vma be processed by > hpage_collapse_scan_pmd(). Wouldn't it be better to replace the vm_file check with vma_is_anonymous()? ie: > - if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) { > + if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file && > + !vma_is_anonymous(vma)) { + if (IS_ENABLED(CONFIG_SHMEM) && + !vma_is_anonymous(vma)) {
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@huawei.com> > --- > 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; } > struct file *file = get_file(vma->vm_file); > pgoff_t pgoff = linear_page_index(vma, > khugepaged_scan.address); > @@ -2768,7 +2769,8 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev, > mmap_assert_locked(mm); > memset(cc->node_load, 0, sizeof(cc->node_load)); > nodes_clear(cc->alloc_nmask); > - if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) { > + if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file && > + !vma_is_anonymous(vma)) { > struct file *file = get_file(vma->vm_file); > pgoff_t pgoff = linear_page_index(vma, addr); >
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@huawei.com> >> --- >> 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.
On 2025/1/9 21:40, Matthew Wilcox wrote: > On Thu, Jan 09, 2025 at 03:00:59PM +0800, Liu Shixin wrote: >> Add vma_is_anonymous() check to make such vma be processed by >> hpage_collapse_scan_pmd(). > Wouldn't it be better to replace the vm_file check with > vma_is_anonymous()? ie: > >> - if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) { >> + if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file && >> + !vma_is_anonymous(vma)) { > + if (IS_ENABLED(CONFIG_SHMEM) && > + !vma_is_anonymous(vma)) { > > > . > Thanks, replace is better. Yang Shi suggest me to set vm_file to NULL when mmap /dev/zero as private. It seems only /dev/zero is special which has vm_file but no vm_ops. This way may solve more hidden similar problems, so I prefer it now.
On 2025/1/10 1: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@huawei.com> >> --- >> 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; > } > Thanks for your advise, I'll look at it again.
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)) { struct file *file = get_file(vma->vm_file); pgoff_t pgoff = linear_page_index(vma, khugepaged_scan.address); @@ -2768,7 +2769,8 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev, mmap_assert_locked(mm); memset(cc->node_load, 0, sizeof(cc->node_load)); nodes_clear(cc->alloc_nmask); - if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) { + if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file && + !vma_is_anonymous(vma)) { struct file *file = get_file(vma->vm_file); pgoff_t pgoff = linear_page_index(vma, addr);
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@huawei.com> --- mm/khugepaged.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)