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