linux-kernel - Re: [PATCH] mm: khugepaged: fix call hpage_collapse_scan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9f223f07-e1e5-4401-89bf-33f065b2ca0f@linux.alibaba.com>
Date: Fri, 10 Jan 2025 10:08:31 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Yang Shi <yang@...amperecomputing.com>, Liu Shixin
 <liushixin2@...wei.com>, Andrew Morton <akpm@...ux-foundation.org>,
 Chengming Zhou <chengming.zhou@...ux.dev>,
 Matthew Wilcox <willy@...radead.org>,
 Kefeng Wang <wangkefeng.wang@...wei.com>, Nanyong Sun
 <sunnanyong@...wei.com>, Muchun Song <muchun.song@...ux.dev>,
 Qi Zheng <zhengqi.arch@...edance.com>, Johannes Weiner <hannes@...xchg.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: khugepaged: fix call hpage_collapse_scan_file() for
 anonymous vma



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@...wei.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.