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

[Mike Rapoport <rppt@kernel.org>]

Hi Andrew,

On Tue, Mar 31, 2026 at 08:01:48PM -0700, Andrew Morton wrote:
> On Tue, 31 Mar 2026 14:41:58 +0100 David Carlier <devnexen@gmail.com> wrote:
> 
> > In mfill_copy_folio_retry(), all locks are dropped to retry
> > copy_from_user() with page faults enabled. During this window, the VMA
> > can be replaced entirely (e.g. munmap + mmap + UFFDIO_REGISTER by
> > another thread), but the caller proceeds with a folio allocated from the
> > original VMA's backing store.

What does "folio allocated from the original VMA's backing store" exactly
mean? Why is this a problem?
 
> > Checking ops alone is insufficient: the replacement VMA could be the
> > same type (e.g. shmem -> shmem) with identical flags but a different
> > backing inode. Take a snapshot of the VMA's file and flags before
> > dropping locks, and compare after re-acquiring them. If anything
> > changed, bail out with -EINVAL.
> > 
> > Use get_file()/fput() rather than ihold()/iput() to hold the file
> > reference across the lock-dropped window, avoiding potential deadlocks
> > from filesystem eviction under mmap_lock.
> 
> Thanks, I've queued this as a squashable fix against mm-unstable's
> "shmem, userfaultfd: implement shmem uffd operations using vm_uffd_ops
> ongoing".

First, this a pre-existing and TBH quite theoretical bug and it was there
since the very beginning, so it should not be added as a fixup for the
uffd+guestmemfd series.

Second, I have reservations about vma_snapshot implementation. What
invariant does it exactly enforce? 
 
> I've fumbled the ball on your [2/2] unlikely() fix ;).  Please resend that
> after -rc1.

This one should go the same route IMO. 

-- 
Sincerely yours,
Mike.