Re: [PATCH mm-hotfixes] mm: do not copy page tables unnecessarily for VM_UFFD_WP

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Jan 14, 2026 at 11:35:08AM +0000, Pedro Falcato wrote:
> On Wed, Jan 14, 2026 at 11:00:06AM +0000, Lorenzo Stoakes wrote:
> > Commit ab04b530e7e8 ("mm: introduce copy-on-fork VMAs and make
> > VM_MAYBE_GUARD one") aggregates flags checks in vma_needs_copy(), including
> > VM_UFFD_WP.
> >
> > However in doing so, it incorrectly performed this check against
> > src_vma. This check was done on the assumption that all relevant flags are
> > copied upon fork.
> >
> > However the userfaultfd logic is very innovative in that it implements
> > custom logic on fork in dup_userfaultfd(), including a rather well hidden
> > case where lacking UFFD_FEATURE_EVENT_FORK causes VM_UFFD_WP to not be
> > propagated to the destination VMA.
> >
> > And indeed, vma_needs_copy(), prior to this patch, did check this property
> > on dst_vma, not src_vma.
> >
> > Since all the other relevant flags are copied on fork, we can simply fix
> > this by checking against dst_vma.
> >
> > While we're here, we fix a comment against VM_COPY_ON_FORK (noting that it
> > did indeed already reference dst_vma) to make it abundantly clear that we
> > must check against the destination VMA.
> >
> > Reported-by: Chris Mason <clm@meta.com>
> > Closes: https://lore.kernel.org/all/20260113231257.3002271-1-clm@meta.com/
> > Fixes: ab04b530e7e8 ("mm: introduce copy-on-fork VMAs and make VM_MAYBE_GUARD one")
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Acked-by: Pedro Falcato <pfalcato@suse.de>

Thanks

>
> > ---
> >  include/linux/mm.h | 6 +++++-
> >  mm/memory.c        | 6 +++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index cb3de0c73d03..44a2a9c0a92f 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -609,7 +609,11 @@ enum {
> >  /*
> >   * Flags which should result in page tables being copied on fork. These are
> >   * flags which indicate that the VMA maps page tables which cannot be
> > - * reconsistuted upon page fault, so necessitate page table copying upon
> > + * reconsistuted upon page fault, so necessitate page table copying upon fork.
> > + *
> > + * Note that these flags should be compared with the DESTINATION VMA not the
> > + * source, as VM_UFFD_WP may not be propagated to destination, while all other
> > + * flags will be.
> >   *
> >   * VM_PFNMAP / VM_MIXEDMAP - These contain kernel-mapped data which cannot be
> >   *                           reasonably reconstructed on page fault.
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 4b0790c8fa48..2839000cd26f 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1465,7 +1465,11 @@ copy_p4d_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> >  static bool
> >  vma_needs_copy(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
> >  {
> > -	if (src_vma->vm_flags & VM_COPY_ON_FORK)
> > +	/*
> > +	 * We check against dst_vma as while sane VMA flags will have been
>
> s/sane/most/?
> I understand you're annoyed but perhaps we should leave this out of the code
> comments themselves :)

No, sane. It's highly surprising that this is a thing that could impact page
table copying in my view, the fact you (+ others) tagged the original patch
suggests to me others also find this surprising, especially given how deeply
hidden the code that does it is.

So 'sane' is the clearest way of saying it.

Also, comments about how I may or may not feel are not really appropriate here,
let's keep the review technical please.

>
> > +	 * copied, VM_UFFD_WP may be set only on dst_vma.
> > +	 */
> > +	if (dst_vma->vm_flags & VM_COPY_ON_FORK)
> >  		return true;
> >  	/*
> >  	 * The presence of an anon_vma indicates an anonymous VMA has page
> > --
> > 2.52.0
>
> --
> Pedro

Cheers, Lorenzo