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

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

(Thanks to Suren for his reply also, Replying to this mail to make it
easier for me to quote :)

On Mon, Oct 19, 2026 at 09:53:32PM +0100, Matthew Wilcox wrote:
> 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@kernel.org/
> > https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
>
> 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()).

I don't think that changed anything. Actually that paragraph is probably
just a bad example, since previous to a373baed5a9d lock_vma_under_rcu()
did:

	/*
	 * find_mergeable_anon_vma uses adjacent vmas which are not locked.
	 * This check must happen after vma_start_read(); otherwise, a
	 * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA
	 * from its anon_vma.
	 */
	if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma))
		goto inval_end_read;

So the example cited couldn't happen then either.

I think fundamentally the biggest issue is that, for CoW, we update the
_parent_ as well as the child to do wrprotect.

So you can end up with a stale TLB entry, which can result in data loss:

Thread A (Parent)          | Thread B (Parent)       | Thread C (Parent)
---------------------------|-------------------------|----------------------
< Fork >                   |                         | < has TLB entry >
copy_present_ptes()        | < no TLB entry >        |
  wrprotect parent page X  |                         |
  -> 2 mappings, refs      | *ptr = x                |
                           | < Page fault >          |
                           | do_wp_page()            |
                           |   < shared, so CoW >    |
                           |   wp_page_copy()        |
                           |     copy X -> new pg N  |
                           |                         | *ptr = x;
                           |                         | [ OOPS! stale TLB,
                           |                         |   lands in X not N ]
                           |     ptep_clear_flush()  |
                           |       < TLB flush >     |
                           |     parent PTE -> N     |
                           |     [ X now child-only ]|
                           |                         | ASSERT(*ptr == x);
flush_tlb_mm(oldmm)        |                         | // fails! N lacks x
  < far too late >         |                         |


David raised this at:
https://lore.kernel.org/all/7e3f35cc-59b9-bf12-b8b1-4ed78223844a@redhat.com/

So the real hazard here is doing the copying with concurrent faults.

I don't really think there's a sensible way around that, unfortunately.

The above is also likely what the bugzilla observed.

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

Well, no, because the VMA locks are designed such that an mmap write lock
doesn't exclude them, so that would be undermining their intent.

The thing is that we allow VMA read locks without an mmap read lock lock, so if
I hold an mmap write lock, I can have parallel reads of any VMA that is
not explicitly VMA write locked.

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

Sure, but the issue is in the parent. CoW really scuppers things here :)

>
> > 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@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.
>
> Is it that fb49c455323f commit message is insufficient and we need to
> prevent simultaneous fork/page fault for other reasons, then?

Yeah, as above, the example given isn't great.

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

Well, that's a long time to hold that lock, and while it might address the
pmd_none_or_clear_bad() thing, the CoW issue remains the same, since really
the issue there is TLB invalidation.

And the only way to address that is to just now allow faults to happen
during fork.

(I also fear there are other subtle bugs that might emerge from all this,
we do like to make _implicit_ assumptions about things).

Actually there's a pleasing thing you came up with at the time:

	Calling fork() from a multi-threaded program is fraught with
	danger.  It's a rare thing to do, and we don't need to optimise for
	it.  It does, of course, need to not crash.  But we can slow it
	down as much as we want to.  Slowing down single-threaded programs
	calling fork is much less acceptable.

https://lore.kernel.org/all/ZKQZnAfLRPVqnxiO@casper.infradead.org/

:))

It does feel a bit like we're bending over backwards for a broken use-case.

And anyway, even if we did somehow find a way to make all this work, I fear
we'd just move the contention somewhere else.

Perhaps the 'genius' hack of mine isn't so bad after all? :P

--
Cheers, Lorenzo