Re: [PATCH v2] mm: khugepaged: fix call hpage_collapse_scan_file() for anonymous vma

[David Hildenbrand <david@redhat.com>]

On 14.01.25 19:00, Yang Shi wrote:
> 
> 
> 
> On 1/14/25 8:56 AM, David Hildenbrand wrote:
>> On 11.01.25 04:45, 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>
>>> ---
>>> v1->v2: Remove the redundant vm_file check, suggested by Matthew Wilcox.
>>>
>>>    mm/khugepaged.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index 653dbb1ff05c..bad1e130eda8 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -2422,7 +2422,7 @@ 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_is_anonymous(vma)) {
>>>                    struct file *file = get_file(vma->vm_file);
>>>                    pgoff_t pgoff = linear_page_index(vma,
>>>                            khugepaged_scan.address);
>>> @@ -2768,7 +2768,7 @@ 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_is_anonymous(vma)) {
>>>                struct file *file = get_file(vma->vm_file);
>>>                pgoff_t pgoff = linear_page_index(vma, addr);
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>>
>> The whole CONFIG_SHMEM is all weird and needs to be cleaned up ... at
>> some point.
>>
>> BTW, do we correctly handle MAP_PRIVATE of these files, where we would
>> have vma->vm_file and !vma_is_anonymous(vma), but could end up having
>> anonymous pages in there?
> 
> 
> I think so. There are a couple of cases if I don't miss something:
> 
> 1. non-shmem: khugepaged just supports collapse read-only opened regular
> file at this point, there should be no anonymous pages in this case
> 2. Shmem:
>       2.1. small folios in page cache: it should be fine to collapse page
> cache itself even though there are anonymous pages
>       2.2. large folios in page cache: khugepaged should bail out from
> collapse_pte_mapped_thp() if CoW'ed anonymous folio is met
> 

Great, thanks!

-- 
Cheers,

David / dhildenb