Re: [PATCH RFC 01/17] userfaultfd: introduce mfill_copy_folio_locked() helper

[Mike Rapoport <rppt@kernel.org>]

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)" <rppt@kernel.org>
> > 
> > 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) <rppt@kernel.org>
> 
> The movement looks all fine,
> 
> Acked-by: Peter Xu <peterx@redhat.com>

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.