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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: double decrement in pmdp_invalidate() via page_table_check_pmd_clear()
--

commit df830550fc83c05eb08f00e78d36d32212821daf
Author: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

mm: clear pmd in pmdp_invalidate()

This commit adds a call to page_table_check_pmd_clear() inside generic
pmdp_invalidate() after calling pmdp_establish() with pmd_mkinvalid(*pmdp).

> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index 6e83ce3801b8a..bbf74c73a1471 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -215,6 +215,7 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>  {
>  	VM_WARN_ON_ONCE(!pmd_present(*pmdp));
>  	pmd_t old = pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmdp));
> +	page_table_check_pmd_clear(vma->vm_mm, address, old);

[Severity: High]
Does calling page_table_check_pmd_clear() here cause a double decrement of
ptc->anon_map_count or ptc->file_map_count when CONFIG_PAGE_TABLE_CHECK is
enabled, triggering a BUG_ON() in page_table_check_clear()?

On architectures that support CONFIG_PAGE_TABLE_CHECK and use the generic
pmdp_invalidate() implementation (x86, arm64, and riscv), pmdp_establish()
already invokes page_table_check_pmd_set() before replacing *pmdp. For
example, in arch/x86/include/asm/pgtable.h:pmdp_establish():

	page_table_check_pmd_set(vma->vm_mm, address, pmdp, pmd);
	if (IS_ENABLED(CONFIG_SMP)) {
		return xchg(pmdp, pmd);

And similarly in arch/arm64/include/asm/pgtable.h:pmdp_establish() and
arch/riscv/include/asm/pgtable.h:pmdp_establish() (as well as
generic_pmdp_establish() via set_pmd_at()).

Inside mm/page_table_check.c:__page_table_check_pmds_set(), the existing
present PMD (*pmdp, which has the same value later returned as old) is
cleared first, and the new invalidated PMD is only accounted if
pmd_user_accessible_page() is true:

	for (i = 0; i < nr; i++)
		__page_table_check_pmd_clear(mm, addr + PMD_SIZE * i, *(pmdp + i));
	if (pmd_user_accessible_page(mm, addr, pmd) &&
	    !page_table_check_huge_zero_pmd(pmd))
		page_table_check_set(pmd_pfn(pmd), stride * nr, pmd_write(pmd));

Because pmd_mkinvalid() clears the present/valid bits (including on riscv
since commit bdda2887cbf3 ("riscv: mm: Fixup compat arch_want_old_prefaulted_pte()
and page_table_check")), pmd_user_accessible_page() returns false for the new
invalidated PMD and page_table_check_set() is not called.

When pmdp_invalidate() then calls page_table_check_pmd_clear() on old a
second time, mm/page_table_check.c:page_table_check_clear() decrements the
counter again:

		if (anon) {
			BUG_ON(atomic_read(&ptc->file_map_count));
			BUG_ON(atomic_dec_return(&ptc->anon_map_count) < 0);
		} else {
			BUG_ON(atomic_read(&ptc->anon_map_count));
			BUG_ON(atomic_dec_return(&ptc->file_map_count) < 0);
		}

Wouldn't any call to pmdp_invalidate() on a present user-accessible PMD (such
as during THP splitting, madvise(), mprotect(), or mremap()) underflow
anon_map_count or file_map_count below zero and hit this BUG_ON()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027113813.1332096-1-trintaeoitogc@gmail.com?part=1