Re: [PATCH] mm/ksm: keep soft-dirty and uffd-wp when merging a page

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/ksm: broken FAULT_FLAG_UNSHARE in break_ksm() on uffd-rwp KSM PTEs
- [Medium] mm/ksm: replace_page() spuriously sets soft-dirty on KSM zero pages
--

commit a0c91d60795a1c981863694b2d5797c97c36486f
Author: Samarth Narang <samarth.colleges@gmail.com>

mm/ksm: keep soft-dirty and uffd-wp when merging a page

This commit preserves the soft-dirty and userfaultfd bits from the
original PTE when replace_page() installs a KSM page, and restores
PAGE_NONE permissions on VM_UFFD_RWP VMAs.

> diff --git a/mm/ksm.c b/mm/ksm.c
> index dbfb8f98b1892..b0d0555aa1b57 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
[ ... ]
> @@ -1463,6 +1464,18 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
>  		dec_mm_counter(mm, MM_ANONPAGES);
>  	}
>  
> +	/*
> +	 * The old PTE may carry soft-dirty and uffd-wp state that tracks writes
> +	 * to this address. Make sure to preserve these bits in the new PTE.
> +	 * On a uffd-rwp VMA, also restore PAGE_NONE.
> +	 */
> +	if (pte_soft_dirty(orig_pte))
> +		newpte = pte_mksoft_dirty(newpte);

[Severity: Medium]
When ksm_use_zero_pages is enabled, the zero-page branch just above in
replace_page() constructs newpte using pte_mkdirty():

mm/ksm.c:replace_page() {
	if (!is_zero_pfn(page_to_pfn(kpage))) {
		...
	} else {
		newpte = pte_mkdirty(pte_mkspecial(pfn_pte(page_to_pfn(kpage), vma->vm_page_prot)));
		...
	}
}

On architectures with CONFIG_HAVE_ARCH_SOFT_DIRTY, pte_mkdirty() sets both
the hardware dirty bit and _PAGE_SOFT_DIRTY:

arch/x86/include/asm/pgtable.h:pte_mkdirty() {
	pte = pte_set_flags(pte, _PAGE_DIRTY | _PAGE_SOFT_DIRTY);

	return pte_mksaveddirty(pte);
}

If pte_soft_dirty(orig_pte) is false when merging with the KSM zero page,
won't _PAGE_SOFT_DIRTY remain set on newpte unless there is an else branch
calling pte_clear_soft_dirty(newpte) (similar to remove_migration_pte() in
mm/migrate.c)?

> +	if (pte_uffd(orig_pte))
> +		newpte = pte_mkuffd(newpte);
> +	if (pte_uffd(orig_pte) && userfaultfd_rwp(vma))
> +		newpte = pte_modify(newpte, PAGE_NONE);

[Severity: High]
How does this interact with KSM unmerging via break_ksm() (called from
madvise(MADV_UNMERGEABLE), __ksm_del_vma(), break_cow(), and
unmerge_and_remove_all_rmap_items())?

When replace_page() installs a KSM page or KSM zero page with
pte_mkuffd(newpte) and PAGE_NONE on a VM_UFFD_RWP VMA, break_ksm()
subsequently tries to unshare it by calling handle_mm_fault() with
FAULT_FLAG_UNSHARE | FAULT_FLAG_REMOTE (and without
FAULT_FLAG_ALLOW_RETRY):

mm/ksm.c:break_ksm() {
	do {
		ksm_page = walk_page_range_vma(vma, addr, end, ops, &addr);
		if (ksm_page <= 0)
			return ksm_page;
		ret = handle_mm_fault(vma, addr,
				      FAULT_FLAG_UNSHARE | FAULT_FLAG_REMOTE,
				      NULL);
	} while (!(ret & (VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | VM_FAULT_OOM)));
	return (ret & VM_FAULT_OOM) ? -ENOMEM : 0;
}

However, handle_pte_fault() checks pte_protnone() and
userfaultfd_pte_rwp() before checking FAULT_FLAG_WRITE |
FAULT_FLAG_UNSHARE:

mm/memory.c:handle_pte_fault() {
	if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma)) {
		if (userfaultfd_pte_rwp(vmf->vma, vmf->orig_pte))
			return do_uffd_rwp(vmf);
		return do_numa_page(vmf);
	}
}

In synchronous RWP mode (!userfaultfd_rwp_async(vma)), do_uffd_rwp() calls
handle_userfault(vmf, VM_UFFD_RWP), which warns under CONFIG_DEBUG_VM
because FAULT_FLAG_ALLOW_RETRY is missing and returns VM_FAULT_SIGBUS.
break_ksm() then terminates its loop and returns 0 without unmerging the
KSM page (leaving KSM pages mapped after clearing VM_MERGEABLE or freeing
rmap_items).

In asynchronous RWP mode (userfaultfd_rwp_async(vma)), the first loop
iteration in break_ksm() runs do_uffd_rwp() and strips _PAGE_UFFD and
PAGE_NONE without unsharing, and the second iteration unshares the page via
do_wp_page() -> wp_page_copy() with RWP protection lost.

Also, in wp_page_copy(), the unshare path preserves pte_mkuffd(entry) on
top of vma->vm_page_prot, but does not restore PAGE_NONE when
userfaultfd_rwp(vma) is true:

mm/memory.c:wp_page_copy() {
		if (unlikely(unshare)) {
			if (pte_soft_dirty(vmf->orig_pte))
				entry = pte_mksoft_dirty(entry);
			if (pte_uffd(vmf->orig_pte))
				entry = pte_mkuffd(entry);
		}
}

Should FAULT_FLAG_UNSHARE bypass do_uffd_rwp() in handle_pte_fault() and
restore PAGE_NONE in wp_page_copy()?

>  
>  	flush_cache_page(vma, addr, pte_pfn(ptep_get(ptep)));
>  	/*
>  	 * No need to notify as we are replacing a read only page with another

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