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

[Mike Rapoport <rppt@kernel.org>]

Hi Peter,

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

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.
 
> Thanks,
> -- 
> Peter Xu

-- 
Sincerely yours,
Mike.