linux-kernel - Re: [PATCH] userfaultfd: hugetlbfs: add UFFDIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Feb 2017 16:52:41 +0100
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Hillf Danton <hillf.zj@...baba-inc.com>,
        Pavel Emelyanov <xemul@...allels.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>
Subject: Re: [PATCH] userfaultfd: hugetlbfs: add UFFDIO_COPY support for
 shared mappings

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@...hat.com>

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