Re: [RFC PATCH 2/4] mm/mmu_notifier: add MMU_NOTIFIER_RANGE_PERM_UPGRADE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mm/hugetlb: hugetlb_change_protection() sets PERM_UPGRADE when revoking access
- [High] mm/mmu_notifier: change_protection() sets PERM_UPGRADE when revoking write
- [High] mm: change_protection() revokes write access under MMU_NOTIFIER_RANGE_PERM_UPGRADE
- [Low] mm/mmu_notifier: unused-but-set mn_flags when CONFIG_MMU_NOTIFIER=n
--

commit 22cbc27b0ff3d2d6c7ee718ba2fc52e905cded0f
Author: Carlos López <clopez@suse.de>

mm/mmu_notifier: add MMU_NOTIFIER_RANGE_PERM_UPGRADE

This commit adds the MMU_NOTIFIER_RANGE_PERM_UPGRADE range flag to mark MMU
notifier invalidations that only upgrade permissions without revoking
existing access or changing PFNs. It sets this flag in
hugetlb_change_protection() and change_pud_range() when
mm_cp_is_perm_upgrade(cp_flags) is true.

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index da5d04edf7c10..3a988ca364e6c 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6596,19 +6596,23 @@ long hugetlb_change_protection(struct vm_area_struct *vma,
>  	long pages = 0, psize = huge_page_size(h);
>  	struct mmu_notifier_range range;
>  	unsigned long last_addr_mask;
> +	unsigned int mn_flags = 0;
>  	bool uffd_wp = cp_flags & MM_CP_UFFD_WP;
>  	bool uffd_wp_resolve = cp_flags & MM_CP_UFFD_WP_RESOLVE;
>  	bool uffd_rwp = cp_flags & MM_CP_UFFD_RWP;
>  	bool uffd_rwp_resolve = cp_flags & MM_CP_UFFD_RWP_RESOLVE;
>  	struct mmu_gather tlb;
>  
> +	if (mm_cp_is_perm_upgrade(cp_flags))
> +		mn_flags |= MMU_NOTIFIER_RANGE_PERM_UPGRADE;
> +
>  	/*
>  	 * In the case of shared PMDs, the area to flush could be beyond
>  	 * start/end.  Set range.start/range.end to cover the maximum possible
>  	 * range if PMD sharing is possible.
>  	 */
>  	mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA,
> -				0, mm, start, end);
> +				mn_flags, mm, start, end);

[Severity: Critical]
Can setting MMU_NOTIFIER_RANGE_PERM_UPGRADE in hugetlb_change_protection()
leave dangling mappings in a secondary MMU or leave writable secondary MMU
mappings to pages that the host unmapped or downgraded to read-only?

First, when PMD sharing is active on a MAP_SHARED hugetlbfs mapping (for
example, two processes sharing a PUD_SIZE-aligned PROT_READ mapping of a 2MB
hugetlbfs folio where only one process holds the folio rmap count on the
shared PMD table) and the process with a secondary MMU calls mprotect() to
upgrade to PROT_READ | PROT_WRITE, mprotect_fixup() sets MM_CP_PERM_UPGRADE.
hugetlb_change_protection() then advertises MMU_NOTIFIER_RANGE_PERM_UPGRADE,
telling secondary MMUs such as KVM to keep their existing SPTEs, and proceeds
to unshare and unmap the PUD entry:

mm/hugetlb.c:hugetlb_change_protection()
  -> huge_pmd_unshare()
    -> __huge_pmd_unshare()
         pud_clear(pud);

Because the PUD in this mm is now cleared and detached from the shared PMD
page table without zapping the secondary MMU, when the other process later
unmaps the file, unmap_hugepage_range() only notifies that other mm and drops
the shared folio's mapcount to zero. If the folio is subsequently migrated or
freed in migrate_hugetlb_folio(), folio_mapped(src) is false and
try_to_migrate() is skipped, freeing or reusing the physical page while the
first process's secondary MMU still retains a live mapping to it.

Second, unlike change_present_ptes() and change_huge_pmd(),
hugetlb_change_protection() ignores MM_CP_TRY_CHANGE_WRITABLE and
unconditionally overwrites the huge PTE protection with newprot
(vma->vm_page_prot):

mm/hugetlb.c:hugetlb_change_protection() {
    ...
    old_pte = huge_ptep_modify_prot_start(vma, address, ptep);
    pte = huge_pte_modify(old_pte, newprot);
    pte = arch_make_huge_pte(pte, shift, vma->vm_flags);
    ...
}

For any MAP_PRIVATE hugetlb VMA, and for any MAP_SHARED hugetlb VMA with
VM_UFFD_WP enabled (where vma_wants_writenotify() is true and
vma_set_page_prot() clears VMA_SHARED_BIT from vma->vm_page_prot), newprot is
read-only. When mprotect() upgrades permissions (for example adding
PROT_EXEC with MM_CP_PERM_UPGRADE) or userfaultfd resolves write protection
(MM_CP_UFFD_WP_RESOLVE via uffd_wp_range() or userfaultfd_clear_vma()), any
already-writable huge PTE in the range has its write bit stripped to
read-only while MMU_NOTIFIER_RANGE_PERM_UPGRADE tells secondary MMUs not to
zap their writable mappings.

Similarly, hugetlb_change_protection() unconditionally downgrades writable
migration entries to readable migration entries even when
mm_cp_is_perm_upgrade(cp_flags) is true:

mm/hugetlb.c:hugetlb_change_protection() {
    ...
    if (softleaf_is_migration_write(entry)) {
        if (folio_test_anon(folio))
            entry = make_readable_exclusive_migration_entry(
                        swp_offset(entry));
        else
            entry = make_readable_migration_entry(
                        swp_offset(entry));
        newpte = swp_entry_to_pte(entry);
        pages++;
    }
    ...
}

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 96e32e0159166..8c17875cdcfa6 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -597,8 +597,13 @@ static inline long change_pud_range(struct mmu_gather *tlb,
>  			continue;
>  
>  		if (!range.start) {
> +			unsigned int mn_flags = 0;
> +
> +			if (mm_cp_is_perm_upgrade(cp_flags))
> +				mn_flags |= MMU_NOTIFIER_RANGE_PERM_UPGRADE;
> +
>  			mmu_notifier_range_init(&range,
> -						MMU_NOTIFY_PROTECTION_VMA, 0,
> +						MMU_NOTIFY_PROTECTION_VMA, mn_flags,
>  						vma->vm_mm, addr, end);
>  			mmu_notifier_invalidate_range_start(&range);
>  		}

[Severity: High]
Does change_pud_range() also set MMU_NOTIFIER_RANGE_PERM_UPGRADE in cases
where the underlying page table walk actually revokes write access?

1. In change_softleaf_pte() and change_non_present_huge_pmd() (as well as
hugetlb_change_protection()), cp_flags is not checked when handling writable
non-present entries. Both SOFTLEAF_MIGRATION_WRITE and
SOFTLEAF_DEVICE_PRIVATE_WRITE entries are unconditionally downgraded to
readable entries during permission upgrades:

mm/mprotect.c:change_softleaf_pte() {
    ...
    if (softleaf_is_migration_write(entry)) {
        ...
        if (folio_test_anon(folio))
            entry = make_readable_exclusive_migration_entry(swp_offset(entry));
        else
            entry = make_readable_migration_entry(swp_offset(entry));
        ...
    } else if (softleaf_is_device_private_write(entry)) {
        entry = make_readable_device_private_entry(swp_offset(entry));
        ...
    }
    ...
}

2. In change_huge_pud() (as in hugetlb_change_protection()),
MM_CP_TRY_CHANGE_WRITABLE is not handled:

mm/huge_memory.c:change_huge_pud() {
    ...
    oldpud = pudp_invalidate(vma, addr, pudp);
    entry = pud_modify(oldpud, newprot);
    set_pud_at(mm, addr, pudp, entry);
    ...
}

Whenever vma->vm_page_prot is read-only (such as a MAP_SHARED DAX PUD VMA
where vma_wants_writenotify() is true), pud_modify(oldpud, newprot) strips
write permission from already-writable huge PUDs while
MMU_NOTIFIER_RANGE_PERM_UPGRADE is advertised.

3. In change_present_ptes() and change_huge_pmd(), pte_modify() and
pmd_modify() first apply newprot (which is read-only when !VM_SHARED or
vma_wants_writenotify()), and only restore write permission if
MM_CP_TRY_CHANGE_WRITABLE is set and can_change_pte_writable() or
can_change_pmd_writable() returns true. For already-writable PTEs or PMDs
where can_change_shared_pte_writable() returns false (for example, a writable
shared PTE cleaned by clean_record_pte(), or a clean writable
VM_PFNMAP/VM_MIXEDMAP entry) or where vm_normal_page() is NULL in a
!VM_SHARED VMA, write permission is stripped while secondary MMUs are told
no access was revoked.

[Severity: High]
What happens when change_protection() runs with MM_CP_PERM_UPGRADE,
MM_CP_UFFD_WP_RESOLVE, or MM_CP_UFFD_RWP_RESOLVE on a writable MAP_SHARED VMA
without vm_ops->page_mkwrite/pfn_mkwrite (such as shmem/tmpfs) after
vma->vm_page_prot has been downgraded to PAGE_COPY for userfaultfd-wp or
soft-dirty tracking?

When userfaultfd-wp is registered or soft-dirty is cleared on a writable
MAP_SHARED shmem VMA, vma_set_page_prot() sees vma_wants_writenotify() return
true, clears VMA_SHARED_BIT on its local flags copy, and stores read-only
PAGE_COPY in vma->vm_page_prot while vma->flags retains
VMA_SHARED_BIT | VMA_WRITE_BIT.

Later, when uffd_wp_range(), userfaultfd_clear_vma(), or mprotect_fixup()
checks whether MM_CP_TRY_CHANGE_WRITABLE should be set, it calls
vma_wants_manual_pte_write_upgrade(vma):

mm/vma.h:vma_wants_manual_pte_write_upgrade()
  -> vma_wants_writenotify(vma, vma->vm_page_prot)

In vma_wants_writenotify(), because shmem has no page_mkwrite or pfn_mkwrite
callback, execution reaches:

mm/vma.c:vma_wants_writenotify() {
    ...
    /* The open routine did something to the protections that pgprot_modify
     * won't preserve? */
    if (pgprot_val(vm_page_prot) !=
        pgprot_val(vma_pgprot_modify(vm_page_prot, vma->flags)))
        return false;
    ...
}

Because vm_page_prot is already PAGE_COPY whereas
vma_pgprot_modify(vm_page_prot, vma->flags) computes PAGE_SHARED (and in
mprotect_fixup(), vma_wants_manual_pte_write_upgrade(vma) is also called
after vma_flags_reset_once() updates vma->flags but before
vma_set_page_prot(vma) updates vma->vm_page_prot), vma_wants_writenotify()
returns false. As a result, MM_CP_TRY_CHANGE_WRITABLE is not set, and
change_present_ptes() and change_huge_pmd() apply read-only newprot
(PAGE_COPY), stripping _PAGE_RW from all present writable PTEs and PMDs in
the range while change_pud_range() sets MMU_NOTIFIER_RANGE_PERM_UPGRADE.

[Severity: Low]
When CONFIG_MMU_NOTIFIER=n, does mn_flags in change_pud_range() and
hugetlb_change_protection() trigger a -Wunused-but-set-variable warning under
W=1?

In include/linux/mmu_notifier.h, when CONFIG_MMU_NOTIFIER is disabled,
mmu_notifier_range_init() is a macro that drops the flags argument:

#define mmu_notifier_range_init(range,event,flags,mm,start,end)  \
	_mmu_notifier_range_init(range, start, end)

So mn_flags is initialized and modified with |=, but never read after
preprocessing when CONFIG_MMU_NOTIFIER=n.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029143336.390231-3-clopez@suse.de?part=2