Re: [PATCH v3 02/15] userfaultfd: introduce struct mfill_state

["Harry Yoo (Oracle)" <harry@kernel.org>]

On Mon, Mar 30, 2026 at 01:11:03PM +0300, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> 
> mfill_atomic() passes a lot of parameters down to its callees.
> 
> Aggregate them all into mfill_state structure and pass this structure to
> functions that implement various UFFDIO_ commands.
> 
> Tracking the state in a structure will allow moving the code that retries
> copying of data for UFFDIO_COPY into mfill_atomic_pte_copy() and make the
> loop in mfill_atomic() identical for all UFFDIO operations on PTE-mapped
> memory.
> 
> The mfill_state definition is deliberately local to mm/userfaultfd.c,
> hence shmem_mfill_atomic_pte() is not updated.
> 
> [harry.yoo@oracle.com: properly initialize mfill_state.len to fix
>                        folio_add_new_anon_rmap() WARN]
> Link: https://lkml.kernel.org/r/abehBY7QakYF9bK4@hyeyoo
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
> ---
>  mm/userfaultfd.c | 148 ++++++++++++++++++++++++++---------------------
>  1 file changed, 82 insertions(+), 66 deletions(-)
> 
> @@ -790,12 +804,14 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>  	    uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE))
>  		goto out_unlock;
>  
> -	while (src_addr < src_start + len) {
> -		pmd_t dst_pmdval;
> +	state.vma = dst_vma;

Oh wait, the lock leak was introduced in patch 2.
If there's an error between uffd_mfill_lock() and `state.vma = dst_vma`,
it remains unlocked.

Probably should have been fixed in 2, not patch 4...
Sorry didn't realize it earlier.

> -		VM_WARN_ON_ONCE(dst_addr >= dst_start + len);
> +	while (state.src_addr < src_start + len) {
> +		VM_WARN_ON_ONCE(state.dst_addr >= dst_start + len);
> +
> +		pmd_t dst_pmdval;
>  
> -		dst_pmd = mm_alloc_pmd(dst_mm, dst_addr);
> +		dst_pmd = mm_alloc_pmd(dst_mm, state.dst_addr);
>  		if (unlikely(!dst_pmd)) {
>  			err = -ENOMEM;
>  			break;
> @@ -866,10 +882,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>  
>  out_unlock:
>  	up_read(&ctx->map_changing_lock);
> -	uffd_mfill_unlock(dst_vma);
> +	uffd_mfill_unlock(state.vma);
>  out:
> -	if (folio)
> -		folio_put(folio);
> +	if (state.folio)
> +		folio_put(state.folio);

Sashiko raised a concern [2] that it the VMA might be unmapped and
a new mapping created as a uffd hugetlb vma and leak the folio by
going through

`if (is_vm_hugetlb_page(dst_vma))
        return mfill_atomic_hugetlb(ctx, dst_vma, dst_start,
                                    src_start, len, flags);`

but it appears to be a false positive (to me) because

`if (atomic_read(&ctx->mmap_changing))` check should have detected unmapping
and free the folio?

[2] https://sashiko.dev/#/patchset/20260330101116.1117699-1-rppt%40kernel.org?patch=13671

>  	VM_WARN_ON_ONCE(copied < 0);
>  	VM_WARN_ON_ONCE(err > 0);
>  	VM_WARN_ON_ONCE(!copied && !err);

Otherwise looks correct to me.

-- 
Cheers,
Harry / Hyeonggon