Hi Peter,
On Tue, Feb 03, 2026 at 12:45:02PM -0500, Peter Xu wrote:
> On Tue, Jan 27, 2026 at 09:29:20PM +0200, Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <[email protected]>
> >
> > Split copying of data when locks held from mfill_atomic_pte_copy() into
> > a helper function mfill_copy_folio_locked().
> >
> > This makes improves code readability and makes complex
> > mfill_atomic_pte_copy() function easier to comprehend.
> >
> > No functional change.
> >
> > Signed-off-by: Mike Rapoport (Microsoft) <[email protected]>
>
> The movement looks all fine,
>
> Acked-by: Peter Xu <[email protected]>
Thanks!
> Just one pure question to ask.
>
> > ---
> > mm/userfaultfd.c | 59 ++++++++++++++++++++++++++++--------------------
> > 1 file changed, 35 insertions(+), 24 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index e6dfd5f28acd..a0885d543f22 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -238,6 +238,40 @@ int mfill_atomic_install_pte(pmd_t *dst_pmd,
> > return ret;
> > }
> >
> > +static int mfill_copy_folio_locked(struct folio *folio, unsigned long
> > src_addr)
> > +{
> > + void *kaddr;
> > + int ret;
> > +
> > + kaddr = kmap_local_folio(folio, 0);
> > + /*
> > + * The read mmap_lock is held here. Despite the
> > + * mmap_lock being read recursive a deadlock is still
> > + * possible if a writer has taken a lock. For example:
> > + *
> > + * process A thread 1 takes read lock on own mmap_lock
> > + * process A thread 2 calls mmap, blocks taking write lock
> > + * process B thread 1 takes page fault, read lock on own mmap lock
> > + * process B thread 2 calls mmap, blocks taking write lock
> > + * process A thread 1 blocks taking read lock on process B
> > + * process B thread 1 blocks taking read lock on process A
>
> While moving, I wonder if we need this complex use case to describe the
> deadlock. Shouldn't this already happen with 1 process only?
>
> process A thread 1 takes read lock (e.g. reaching here but
> before copy_from_user)
> process A thread 2 calls mmap, blocks taking write lock
> process A thread 1 goes on copy_from_user(), trigger page fault,
> then tries to re-take the read lock
>
> IIUC above should already cause deadlock when rwsem prioritize the write
> lock here.
We surely can improve the description here, but it should be a separate
patch with its own changelog and it's out of scope of this series.
> > + *
> > + * Disable page faults to prevent potential deadlock
> > + * and retry the copy outside the mmap_lock.
> > + */
> > + pagefault_disable();
> > + ret = copy_from_user(kaddr, (const void __user *) src_addr,
> > + PAGE_SIZE);
> > + pagefault_enable();
> > + kunmap_local(kaddr);
--
Sincerely yours,
Mike.
