Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[David Hildenbrand]

On 14.12.22 21:04, Peter Xu wrote:
> When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
> have it and has pte marker installed.  The warning is improper along with
> the comment.  The right thing is to inherit the pte marker when needed, or
> keep the dst pte empty.
>
> A vague guess is this happened by an accident when there's the prior patch
> to introduce src/dst vma into this helper during the uffd-wp feature got
> developed and I probably messed up in the rebase, since if we replace
> dst_vma with src_vma the warning & comment it all makes sense too.
>
> Hugetlb did exactly the right here (copy_hugetlb_page_range()).  Fix the
> general path.
>
> Reproducer:
>
> https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
>
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.19+
> Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
> Reported-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
>   mm/memory.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index aad226daf41b..032ef700c3e8 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>   			return -EBUSY;
>   		return -ENOENT;
>   	} else if (is_pte_marker_entry(entry)) {
> -		/*
> -		 * We're copying the pgtable should only because dst_vma has
> -		 * uffd-wp enabled, do sanity check.
> -		 */
> -		WARN_ON_ONCE(!userfaultfd_wp(dst_vma));
> -		set_pte_at(dst_mm, addr, dst_pte, pte);
> +		if (userfaultfd_wp(dst_vma))
> +			set_pte_at(dst_mm, addr, dst_pte, pte);
>   		return 0;
>   	}
>   	if (!userfaultfd_wp(dst_vma))

Staring at the code first made me go "what about other PTE markers". I 
then looked into the discussion in patch #2. The fix as is is 
suboptimal, because it

1) Removes the warning which is good, but
2) Silently drops swapin errors now

So it silently breaks something else temporarily ...


I remember, that theoretically we could have multiple markers stored in 
a single PTE marker.

Wouldn't it be cleaner to be able to "clean" specific markers from a PTE 
marker without having to special case on each and everyone? I mean, only 
uffd-wp is really special such that it might disappear for the target.

Something like (pseudocode):

if (!userfaultfd_wp(dst_vma))
	pte_marker_clear_uff_wp(entry);
if (!pte_marker_empty(entry)) {
	pte = make_pte_marker(pte_marker_get(entry));
	set_pte_at(dst_mm, addr, dst_pte, pte);
}

Then this fix would be correct and backport-able even without #2. And it
would work for new types of markers :)


I'd prefer a fix that doesn't break something else temporarily, even if 
the stable backport might require 5 additional minutes to do. So 
squashing #2 into #1 would also work.

--
Thanks,

David / dhildenb