Re: [PATCH] mm: clear pmd in pmdp_invalidate()

[Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>]

Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote:
> "David Hildenbrand (Arm)" <david@kernel.org> wrote:
> > > when attempt to do a madvise in a chunk of memory allocated by mmap, the
> > > folio_split is called to split a portion of memory, and despite the path
> > > leads to pmd_mkinvalid() they do not clear the pmd and clearer wording,
> > > leading to a BUG_ON inside page_table_check_set() because the map count
> > > is more than 1.
> > >
> > > Call the page_table_check_pmd_clear() inside pmdp_invalidate() generic
> > > function to ensure that the pmd will have they map count decrement.
> > >
> >
> > The subject + description is rather confusing: you are not "clearing the pmd",
> > you tell page-table-check that the PMD was cleared.
> >
> > This is all about apge-table-check.
> Yes, you are right, i'm not clearing the pmd, I will improve this.
> 
> >
> > Can you post the actual kernel splat you ran into? Or is this generated by an LLM?
> Of course, the stack trace of error is (already decoded):
> page_table_check_set (mm/page_table_check.c:118)
> __page_table_check_ptes_set (mm/page_table_check.c:223)
> remove_migration_pte (include/linux/page_table_check.h:83 arch/riscv/include/asm/pgtable.h:642 mm/migrate.c:448)
> rmap_walk_anon (mm/rmap.c:3053)
> rmap_walk_locked (mm/rmap.c:3169 mm/rmap.c:3164)
> remove_migration_ptes (mm/migrate.c:483)
> __folio_split (mm/huge_memory.c:3640 mm/huge_memory.c:4289)
> split_folio_to_list (mm/huge_memory.c:4435 include/linux/huge_mm.h:416 mm/huge_memory.c:4499)
> madvise_cold_or_pageout_pte_range (mm/madvise.c:420)
> walk_pgd_range (mm/pagewalk.c:147 mm/pagewalk.c:237 mm/pagewalk.c:278 mm/pagewalk.c:319)
> __walk_page_range (mm/pagewalk.c:427)
> walk_page_range_vma_unsafe (mm/pagewalk.c:737)
> walk_page_range_vma (mm/pagewalk.c:747)
> madvise_cold (mm/madvise.c:594 mm/madvise.c:614)
> madvise_vma_behavior (mm/madvise.c:1364)
> madvise_walk_vmas (mm/madvise.c:1721)
> madvise_do_behavior (mm/madvise.c:1916)
> do_madvise (mm/madvise.c:2014)
> __riscv_sys_madvise (mm/madvise.c:2023 mm/madvise.c:2021 mm/madvise.c:2021)
> syscall_handler (arch/riscv/include/asm/syscall.h:112)
> do_trap_ecall_u (arch/riscv/kernel/traps.c:341)
> handle_exception (arch/riscv/kernel/entry.S:237)
> 
> And the repro.c that I writed to reproduce bug:
> ```
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <errno.h>
> #include <linux/mman.h>
> 
> #ifndef MADV_COLD
> #define MADV_COLD 20
> #endif
> 
> #define HPAGE_PMD_SIZE (2 * 1024 * 1024)  // 2MB huge page on RISC-V
> 
> int main(void)
> {
>         char *addr;
>         size_t len = HPAGE_PMD_SIZE * 2;  // 4MB - two huge pages
>         int ret;
> 
>         printf("Testing page_table_check bug with madvise(MADV_COLD)...\n");
>         printf("Huge page size: %zu MB\n", HPAGE_PMD_SIZE / (1024*1024));
> 
>         // Fallback: try without MAP_HUGETLB (kernel will use THP)
>         printf("starting THP...\n");
>         addr = mmap(NULL, len, PROT_READ | PROT_WRITE,
>                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>         if (addr == MAP_FAILED) {
>                 perror("mmap failed");
>                 return 1;
>         }
>         // Enable THP for this mapping
>         ret = madvise(addr, len, MADV_HUGEPAGE);
>         if (ret) {
>                 perror("madvise(MADV_HUGEPAGE) failed");
>                 return 1;
>         }
> 
>         printf("Allocated memory at %p\n", addr);
> 
>         // Write to the pages to ensure they're mapped
>         memset(addr, 'A', len);
>         memset(addr + HPAGE_PMD_SIZE, 'B', HPAGE_PMD_SIZE);
> 
>         // Touch pages to ensure huge page allocation
>         for (size_t i = 0; i < len; i += HPAGE_PMD_SIZE) {
>                 addr[i] = 'X';
>         }
> 
>         printf("Memory initialized, calling madvise(MADV_COLD) on partial range...\n");
> 
>         // Call madvise(MADV_COLD) on a partial range of the first huge page
>         // This should trigger the huge page split and the bug
>         ret = madvise(addr + 4096, HPAGE_PMD_SIZE - 8192, MADV_COLD);
>         if (ret) {
>                 perror("madvise(MADV_COLD) failed");
>                 // Don't return - the kernel might have crashed
>         }
> 
>         printf("madvise returned: %d (errno: %d)\n", ret, errno);
> 
>         // If we get here without kernel crash, the bug might be fixed
>         // or the conditions weren't met
>         printf("Test completed (if kernel didn't crash, bug may be fixed or not triggered)\n");
> 
>         munmap(addr, len);
>         return 0;
> }
> ```
> 
> I see this BUG here:
> https://lore.kernel.org/all/69851c62.a00a0220.37c87e.002e.GAE@google.com/T/
> 
> Yes, I'd should mencioned "Reported-by: syzbot+227179d5a8a87e9df90d@syzkaller.appspotmail.com", my bad

At first, I suspected of pmd_user_accessible_pages() and I started analyze this
function. In a brief search I found a CVE that fix similar bug:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=21e5eca0ac9046da9918a919bc92b7b5a78d27e7
They do this:
```
-	return pmd_leaf(pmd) && (pmd_user(pmd) || pmd_user_exec(pmd));
+	return pmd_leaf(pmd) && !pmd_present_invalid(pmd) && (pmd_user(pmd) || pmd_user_exec(pmd));
```

but, inside riscv-dependent code (where this fix happen), the pmd_user_accessible_page() is:
```
static inline bool pmd_user_accessible_page(struct mm_struct *mm, unsigned long addr, pmd_t pmd)
{
	return pmd_leaf(pmd) && pmd_user(pmd);
}
```

and the pmd_leaf() do:
```
	return pmd_present(pmd) && (pmd_val(pmd) & _PAGE_LEAF);
```

and pmd_present, do:
```
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
	(pmd_val(pmd) & (_PAGE_PRESENT | _PAGE_PROT_NONE | _PAGE_LEAF))
#else
	(pmd_val(pmd) & (_PAGE_PRESENT | _PAGE_PROT_NONE));
```

So , the pmd_leaf() looks like good enough to check if the pmd is accessible to
page_table_check.

... Too , I suspected of PAGE_PROT_NONE, because the doc above they declaration say:
```
/*
 * _PAGE_PROT_NONE is set on not-present pages (and ignored by the hardware) to
 * distinguish them from swapped out pages
 */
```
And this lead me to think "if set a not-present pages, so maybe this not should
return true inside pmd_user_accessible_page".... 

So I check the `git log` to see when and why this is introduced. But how is
mentioned by git commit message:
```
commit e3613bb8afc2a9474c9214d65c8326c5ac02135e
Author: Stefan O'Rear <sorear2@gmail.com>
Date:   Sun Dec 16 13:03:36 2018 -0500

    riscv: Add pte bit to distinguish swap from invalid
    
    Previously, invalid PTEs and swap PTEs had the same binary
    representation, causing errors when attempting to unmap PROT_NONE
    mappings, including implicit unmap on exit.

    ...
```

So , they is introduced to distinguish between swap and invalid ptes.

So , I concluded that this is not the problem, but maybe the documentation is
not good enought about the PAGE_PROT_NONE flag.

And, after a lot of thoughts, I try to call page_table_check_pmd_clear() ,
inside pmdp_invalidate(). I attempt to do this because, I see any architecture
dependent pmdp_invalidate() calling this function and think that probably this
is the solution... 
I apply , test with my repro.c and , this WORK. 

I implement this inside generic pmdp_invalidate() because, in my mind this is a
generic problem and can happen in any architecture not inside riscv specific.


thanks,
guilherme