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]> 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. > + * > + * 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); > + > + if (ret) > + return -EFAULT; > + > + flush_dcache_folio(folio); > + return ret; > +} > + > static int mfill_atomic_pte_copy(pmd_t *dst_pmd, > struct vm_area_struct *dst_vma, > unsigned long dst_addr, > @@ -245,7 +279,6 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd, > uffd_flags_t flags, > struct folio **foliop) > { > - void *kaddr; > int ret; > struct folio *folio; > > @@ -256,27 +289,7 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd, > if (!folio) > goto out; > > - 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 > - * > - * 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); > + ret = mfill_copy_folio_locked(folio, src_addr); > > /* fallback to copy_from_user outside mmap_lock */ > if (unlikely(ret)) { > @@ -285,8 +298,6 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd, > /* don't free the page */ > goto out; > } > - > - flush_dcache_folio(folio); > } else { > folio = *foliop; > *foliop = NULL; > -- > 2.51.0 > -- Peter Xu
