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

[Matthew Wilcox]

On Fri, Oct 02, 2026 at 02:12:35PM +0100, Lorenzo Stoakes (ARM) wrote:
> 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):
> > 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@xxxxxxxxxx/
> https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@xxxxxxxxxxxxxxxxxxxxxx/

I wonder if we still need this.  The commit message says:

    A concurrent page fault on a page newly marked read-only by the page
    copy might trigger wp_page_copy() and a anon_vma_prepare(vma) on the
    source vma, defeating the anon_vma_clone() that wasn't done because the
    parent vma originally didn't have an anon_vma, but we now might end up
    copying a pte entry for a page that has one.

But we're holding the mmap_lock for write here, and we need to hold the
mmap_lock for read in order to change a vma->anon_vma (asserted in
__anon_vma_prepare() with mmap_assert_locked()).

At the time of this commit, lock_vma_under_rcu() contained:

        /* find_mergeable_anon_vma uses adjacent vmas which are not locked */
        if (!vma->anon_vma && !vma_is_tcp(vma))
                goto inval;

so we really did have to wait for read holders to finish the fault.
But commit a373baed5a9d deferred the check for a NULL anon VMA until we
actually needed it, so I think commit fb49c455323f's logic was invalidated
by a373baed5a9d.

Maybe I'm wrong.  This is all a bit out of my normal area.

> 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?)

Possibly we have to adjust some assertions to check that either we have
the VMA locked or we have the MM write locked.  That doesn't seem
unreasonable.

> > 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).

I was perhaps unclear.  What I mean is that the child may or may not
observe them as having happened.

> 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.

Yup.

> 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@xxxxxxxxxx/
> 
> 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.

Is it that fb49c455323f commit message is insufficient and we need to
prevent simultaneous fork/page fault for other reasons, then?

> 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() :(

... could we fix this by taking the ptlock during the copy operation?

> 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