Re: [PATCH v6] mm: retry page faults once under the per-VMA lock

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Oct 01, 2026 at 10:12:16PM +0100, Matthew Wilcox wrote:
> So what I didn't realise is that fork() waits for page faults to finish.
> I don't think that's necessary, so we can just stop doing that (whitespace
> damaged):
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 4bf26b0f1e6e..e79555247d3a 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1739,9 +1739,6 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>         for_each_vma(vmi, mpnt) {
>                 struct file *file;
>
> -               retval = vma_start_write_killable(mpnt);
> -               if (retval < 0)
> -                       goto loop_out;
>                 if (vma_test(mpnt, VMA_DONTCOPY_BIT)) {
>                         retval = vma_iter_clear_gfp(&vmi, mpnt->vm_start,
>                                                     mpnt->vm_end, GFP_KERNEL);
>
> I think this is safe.  I've booted a kernel with this change, and

I'm afraid it's not safe :)

We already actually did this (by mistake I think?) before. See commit
fb49c455323f ("fork: lock VMAs of the parent process when forking") - we shipped
that in 6.4.0 -> 6.4.2 and it corrupted memory all over the place.

Reports + a consistent reproducer:

https://bugzilla.kernel.org/show_bug.cgi?id=217624
https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/

Also we have asserts for this lock in copy_page_range() and
copy_hugetlb_page_range() which will now fail (and possibly more further down
the stack?)

> My argument for why it's safe is that a thread which takes a page
> fault during fork() might have taken the page fault either before or
> after fork().  The faults will definitely happen in the parent process.
> They may or may not have happened in the child process, which can't
> possibly care whether or not they've happened.

The problem isn't the child it's the parent - copy_page_range() _modifies_ the
_parent_'s page tables in-place, on assumption that nothing else can access
them (for CoW).

And actually nowadays things are _even_ worse, you could also have concurrent
page faults, MADV_DONTNEED, MADV_FREE, MADV_GUARD_INSTALL/REMOVE, UFFDIO_COPY +
UFFDIO_MOVE (yikes) all of which might introduce new issues + would have to be
audited too.

So - it turns out a lot of stuff implicitly assumes VMA write lock now:

1. CoW - deferred TLB flush race

(The issue above)

https://lore.kernel.org/all/20230705171213.2843068-2-surenb@google.com/

This happens because __copy_present_ptes() does wrprotect_ptes() on the parent
without a TLB flush (that's deferred until the end of the fork).

But unfortunately without a VMA write lock something can then replace the page
table entry in the meantime and you can end up with data loss as a result.

Same issue exists with hugetlb wp too. And god only knows with uffd wp :)

2. THP - can destroy parent THP mappings

copy_pmd_range() does:

	if (pmd_is_huge(*src_pmd)) { copy_huge_pmd(); ... }
	if (pmd_none_or_clear_bad(src_pmd))
		continue;

Without the write lock, a concurrent fault can install a huge PMD via
do_huge_pmd_anonymous_page() or filemap_map_pmd()/do_set_pmd().

The issue is that pmd_bad() fires for leaf PMD (ugh) on at least x86 and arm64,
meaning that'll clear the page table entry.

I think Hugh left a comment alluding to this before:

	 * copy_pmd_range()'s prior pmd_none_or_clear_bad(src_pmd), and the
	 * error handling here, assume that exclusive mmap_lock on dst and src
	 * protects anon from unexpected THP transitions; ...

It's like the mess around pmd_trans_unstable() :(

3. copy_pte_range() skips the required pmd_same() recheck assuming write lock

It uses pte_offset_map_rw_nolock() with a dummy pmdval and:

	 * We already hold the exclusive mmap_lock, the copy_pte_range() and
	 * retract_page_tables() are using vma->anon_vma to be exclusive, so
	 * the PTE page is stable, and there is no need to get pmdval and do
	 * pmd_same() check.

(Probably needs updating for VMA write lock...)

4. The asserts

Mentioned above!

I suspect there's more as well.

Some of it could be changed, but it'd all be at the cost of behaving very
strangely (no VMA lock but mmap write lock) while manipulating things.

And I am not really happy with changing page table entries in the parent VMA
(for CoW) without a VMA write lock held.

I also wonder whether it would achieve all that much really - the issue is with
holding the VMA lock across I/O, anything else that tries to grab a write lock
is going to end up being the new thing that's blocked (mprotect, munmap, mremap,
mlock etc.)

I think doing something like this would need a very significant redesign and I'm
not convinced it's one that really achieves what we want given all the other
cases that'd get blocked by the VMA lock being held still.

--
Cheers, Lorenzo