Re: [PATCH v4 05/15] userfaultfd: retry copying with locks dropped in mfill_atomic_pte_copy()

[Harry Yoo (Oracle)]

On Thu, Apr 02, 2026 at 07:11:46AM +0300, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@xxxxxxxxxx>
> 
> Implementation of UFFDIO_COPY for anonymous memory might fail to copy data
> from userspace buffer when the destination VMA is locked (either with
> mm_lock or with per-VMA lock).
> 
> In that case, mfill_atomic() releases the locks, retries copying the data
> with locks dropped and then re-locks the destination VMA and
> re-establishes PMD.
> 
> Since this retry-reget dance is only relevant for UFFDIO_COPY and it never
> happens for other UFFDIO_ operations, make it a part of
> mfill_atomic_pte_copy() that actually implements UFFDIO_COPY for anonymous
> memory.
> 
> As a temporal safety measure to avoid breaking biscection
> mfill_atomic_pte_copy() makes sure to never return -ENOENT so that the
> loop in mfill_atomic() won't retry copiyng outside of mmap_lock.  This is
> removed later when shmem implementation will be updated later and the loop
> in mfill_atomic() will be adjusted.
> 
> [akpm@xxxxxxxxxxxxxxxxxxxx: update mfill_copy_folio_retry()]
>   Link: https://lkml.kernel.org/r/20260316173829.1126728-1-avagin@xxxxxxxxxx
> Link: https://lkml.kernel.org/r/20260306171815.3160826-6-rppt@xxxxxxxxxx
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---

Looks good to me,
Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

>  mm/userfaultfd.c | 75 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 51 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index c6a38db45343..82e1a3255e1e 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -405,35 +405,63 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr)
>  static int mfill_atomic_pte_copy(struct mfill_state *state)
>  {
> -	struct vm_area_struct *dst_vma = state->vma;
>  	unsigned long dst_addr = state->dst_addr;
>  	unsigned long src_addr = state->src_addr;
>  	uffd_flags_t flags = state->flags;
> -	pmd_t *dst_pmd = state->pmd;
>  	struct folio *folio;
>  	int ret;
>  
> -	if (!state->folio) {
> -		ret = -ENOMEM;
> -		folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, dst_vma,
> -					dst_addr);
> -		if (!folio)
> -			goto out;
> +	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, state->vma, dst_addr);
> +	if (!folio)
> +		return -ENOMEM;
>  
> -		ret = mfill_copy_folio_locked(folio, src_addr);
> +	ret = -ENOMEM;
> +	if (mem_cgroup_charge(folio, state->vma->vm_mm, GFP_KERNEL))
> +		goto out_release;
>  
> -		/* fallback to copy_from_user outside mmap_lock */
> -		if (unlikely(ret)) {
> -			ret = -ENOENT;
> -			state->folio = folio;
> -			/* don't free the page */
> -			goto out;
> -		}
> -	} else {
> -		folio = state->folio;
> -		state->folio = NULL;
> +	ret = mfill_copy_folio_locked(folio, src_addr);
> +	if (unlikely(ret)) {
> +		/*
> +		 * Fallback to copy_from_user outside mmap_lock.
> +		 * If retry is successful, mfill_copy_folio_locked() returns
> +		 * with locks retaken by mfill_get_vma().

nit: mfill_copy_folio_locked() -> mfill_copy_folio_retry();

> +		 * If there was an error, we must mfill_put_vma() anyway and it
> +		 * will take care of unlocking if needed.
> +		 */
> +		ret = mfill_copy_folio_retry(state, folio);
> +		if (ret)
> +			goto out_release;
>  	}
>  
>  	/*

-- 
Cheers,
Harry / Hyeonggon