Re: [PATCH] userfaultfd: hugetlbfs: add UFFDIO_COPY support for shared mappings

[Andrea Arcangeli]

On Thu, Feb 16, 2017 at 04:18:51PM -0800, Mike Kravetz wrote:
> Thanks Andrea, I incorporated your suggestions into a new version of the patch. 
> While changing (dst_vma->vm_flags & VM_SHARED) to integers, I noticed an issue
> in the error path of __mcopy_atomic_hugetlb().

Indeed good point!

> +	int vm_alloc_shared = dst_vma->vm_flags & VM_SHARED;
> +	int vm_shared = dst_vma->vm_flags & VM_SHARED;

Other minor nitpick, this could have been:

     int vm_shared = vm_alloc_shared;

But I'm sure gcc will emit the same asm.

For greppability (if such word exist) calling it vm_shared_alloc would
have been preferable. We can clean it up post upstream merge or it
should be diffed against mm latest or it may cause more rejects.

Reviewed-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>


The patches were not against latest -mm so I solved the rejects during
merge in my tree. Then I looked at the result of all my merges after
everything is applied and I think I spotted a merge gone wrong in this
patch:

https://ozlabs.org/~akpm/mmots/broken-out/userfaultfd-mcopy_atomic-return-enoent-when-no-compatible-vma-found.patch

Below is a hand edited git diff that shows the only meaningful
difference.

The below should be included in
userfaultfd-mcopy_atomic-return-enoent-when-no-compatible-vma-found.patch
or as -fix2 at the end.

Everything else is identical which is great. Mike Rapoport could you
verify the below hunk is missing in mm?

Once it'll all be merged upstream then there will be less merge crunch
as we've been working somewhat in parallel on the same files, so this
is resulting in more merge rejects than ideal :).

diff --git a/../mm/mm/userfaultfd.c b/mm/userfaultfd.c
index 830bed7..3ec9aad 100644
--- a/../mm/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -199,6 +201,12 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
 		dst_vma = find_vma(dst_mm, dst_start);
 		if (!dst_vma || !is_vm_hugetlb_page(dst_vma))
 			goto out_unlock;
+		/*
+		 * Only allow __mcopy_atomic_hugetlb on userfaultfd
+		 * registered ranges.
+		 */
+		if (!dst_vma->vm_userfaultfd_ctx.ctx)
+			goto out_unlock;
 
 		if (dst_start < dst_vma->vm_start ||
 		    dst_start + len > dst_vma->vm_end)
@@ -214,16 +224,10 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
 		goto out_unlock;
 
 	/*
-	 * Only allow __mcopy_atomic_hugetlb on userfaultfd registered ranges.
-	 */
-	if (!dst_vma->vm_userfaultfd_ctx.ctx)
-		goto out_unlock;
-
-	/*
 	 * If not shared, ensure the dst_vma has a anon_vma.
 	 */
 	err = -ENOMEM;

Thanks,
Andrea