Re: [PATCH v4] mm/userfaultfd: detect VMA replacement after copy retry in mfill_copy_folio_retry()

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 09, 2026 at 02:31:46PM +0300, Mike Rapoport wrote:
> Hi Peter,

Hi, Mike,

> 
> On Thu, Apr 02, 2026 at 09:42:01AM -0400, Peter Xu wrote:
> > Hi, Mike,
> > 
> > Let me also leave my comments inline just for you to consider.
> > 
> > On Thu, Apr 02, 2026 at 06:58:33AM +0300, Mike Rapoport wrote:
> > > Hi David,
> > > 
> > > It feels that you use an LLM for correspondence. Please tune it down to
> > > produce more laconic and to the point responses.
> > > 
> > > On Wed, Apr 01, 2026 at 09:06:36AM +0100, David CARLIER wrote:
> > > >   On Tue, Apr 01, 2026 at 08:49:00AM +0300, Mike Rapoport wrote:
> > > >   > What does "folio allocated from the original VMA's backing store" exactly
> > > >   > mean? Why is this a problem?
> > > > 
> > > >   Fair point, the commit message was vague here. What I meant is:
> > > > 
> > > >   mfill_atomic_pte_copy() captures ops = vma_uffd_ops(state->vma) and
> > > >   passes it to __mfill_atomic_pte(). There, ops->alloc_folio() allocates
> > > >   a folio for the original VMA's inode (e.g. a shmem folio for that
> > > >   specific shmem inode). 
> > > 
> > > I wouldn't say ->alloc_folio() allocates a folio _for_ the inode, it
> > > allocates it with inode's memory policy. Worst can happen without any
> > > changes is that the allocated folio will end up in a wrong node.
> > 
> > For shmem it's only about mempolicy indeed, but since we're trying to
> > export it as an API in the series, IMHO it would be nice to be generic.  So
> > we shouldn't assume it's only about mempolicy, we should rely on detecting
> > any context change and bail out with -EAGAIN, relying all rest checks to
> > the next UFFDIO_COPY ioctl done on top of the new mapping topology.
> 
> My point was that this is preexisting bug and that we don't need to rush
> with the complete fix that will extensively compare VMA compatibility...

Yes, I fully agree it was pre-existing.  My guess is we only didn't reach a
consensus yet on how to completely fix it, and whether we need an
intermediate fix for "a VMA suddenly changed to hugetlb" only.

>  
> > > This is still a footgun, but I don't see it as a big deal.
> > 
> > IIUC this is a real bug reported.  Actually, if my understanding is
> > correct, we should be able to easily write a reproducer by registering the
> > src addr of UFFDIO_COPY to userfaultfd too, then the ioctl(UFFDIO_COPY)
> > thread will get blocked faulting in the src_addr.  During that, we can
> > change the VMA layout in another thread to test whatever setup we want.
> > 
> > > Let's revisit it after -rc1 and please make sure to cc "MEMORY MAPPING"
> > > folks for insights about how to better track VMA changes or their absence.
> > 
> > No strong feeling here if we want to slightly postpone this fix.  It looks
> > like not easy to happen as it looks to be a bug present for a while, indeed.
> > 
> > It's just that if my understanding is correct, with above reproducer we can
> > crash the kernel easily without a proper fix.
>  
> ... but we do need a more urgent fix for the case when a VMA suddenly
> becomes hugetlb, because that could not happen before the refactoring.

Personally this is least of a concern to me. Hugetlbfs is so specially
managed in userapps, so it is even less likely to trigger the same bug with
VM_SOFTDIRTY changes or other ways.

But I understand your point.  You want to cover what your series changed on
this.  If you think this is the right way to go, I'll follow your decision.

> 
> For that, it would be enough to check that ->ops are the same before and
> after copy_from_user().
> 
> @David, do you mind to send a patch for this without waiting for rc1?
>  
> > > >   The vm_flags comparison was a secondary guard against permission/type
> > > >   changes during the window.
> > > 
> > > Permissions should be fine, they are checked in userfaultfd_register.
> > > Some other flags that don't matter to uffd operation may change during the
> > > window, though and then a comparison of vm_flags will give a false
> > > positive.
> > 
> > IMHO false positive is fine in this case when -EAGAIN will be used (which I
> > still think we should), if it only causes a retry.
> 
> I still disagree, but let's postpone this discussion for later, when David
> resends the patch that compares VMA properties.

Maybe it's because we are having different checks in mind, where you wanted
to check only the "invalid cases" but I am trying to make it fallback for
all detectable changes.

IMHO it's hard to define "invalid case" in this case, and it's also
unnecessary when a EAGAIN will be processed with another UFFDIO_COPY
attempt and we'll simply redo all the checks.  Hence relying on VMA change
would be the simplest, safest to me.

Again, I'm open to any suggestion on replacing the vma snapshot logic as
long as all possible issues got reported will be properly fixed, especially
in the latest mainline. I don't worry much on backporting yet; if this bug
existed for 10 years and nobody yet reported, to me we can always evaluate
the effort to backport or skip.  However, a proper fix in mainline is IMHO
more important.

Thanks,

-- 
Peter Xu