* Re: [PATCH v16 07/16] mm, arm64: untag user pointers in get_vaddr_frames
From: Kees Cook @ 2019-06-08 4:00 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Catalin Marinas,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig, Mauro Carvalho Chehab,
Jens Wiklander
In-Reply-To: <da1d0e0f6d69c15a12987379e372182f416cbc02.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:09PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> get_vaddr_frames uses provided user pointers for vma lookups, which can
> only by done with untagged pointers. Instead of locating and changing
> all callers of this function, perform untagging in it.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/frame_vector.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> index c64dca6e27c2..c431ca81dad5 100644
> --- a/mm/frame_vector.c
> +++ b/mm/frame_vector.c
> @@ -46,6 +46,8 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> if (WARN_ON_ONCE(nr_frames > vec->nr_allocated))
> nr_frames = vec->nr_allocated;
>
> + start = untagged_addr(start);
> +
> down_read(&mm->mmap_sem);
> locked = 1;
> vma = find_vma_intersection(mm, start, start + 1);
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 06/16] mm, arm64: untag user pointers in mm/gup.c
From: Kees Cook @ 2019-06-08 3:59 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Catalin Marinas,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig, Mauro Carvalho Chehab,
Jens Wiklander
In-Reply-To: <e1f6d268135f683fd70c2af27e75f694d7ffaf48.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:08PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> mm/gup.c provides a kernel interface that accepts user addresses and
> manipulates user pages directly (for example get_user_pages, that is used
> by the futex syscall). Since a user can provided tagged addresses, we need
> to handle this case.
>
> Add untagging to gup.c functions that use user addresses for vma lookups.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/gup.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e4..c37df3d455a2 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -802,6 +802,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> if (!nr_pages)
> return 0;
>
> + start = untagged_addr(start);
> +
> VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
>
> /*
> @@ -964,6 +966,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
> struct vm_area_struct *vma;
> vm_fault_t ret, major = 0;
>
> + address = untagged_addr(address);
> +
> if (unlocked)
> fault_flags |= FAULT_FLAG_ALLOW_RETRY;
>
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 15/16] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
From: Kees Cook @ 2019-06-08 3:58 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Catalin Marinas,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig, Mauro Carvalho Chehab,
Jens Wiklander
In-Reply-To: <c529e1eeea7700beff197c4456da6a882ce2efb7.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:17PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> vaddr_get_pfn() uses provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> drivers/vfio/vfio_iommu_type1.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 3ddc375e7063..528e39a1c2dd 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -384,6 +384,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
>
> down_read(&mm->mmap_sem);
>
> + vaddr = untagged_addr(vaddr);
> +
> vma = find_vma_intersection(mm, vaddr, vaddr + 1);
>
> if (vma && vma->vm_flags & VM_PFNMAP) {
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 16/16] selftests, arm64: add a selftest for passing tagged pointers to kernel
From: Kees Cook @ 2019-06-08 3:56 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Catalin Marinas,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig, Mauro Carvalho Chehab,
Jens Wiklander
In-Reply-To: <9e1b5998a28f82b16076fc85ab4f88af5381cf74.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:18PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
I'm adding Shuah to CC in case she has some suggestions about the new
selftest.
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> tools/testing/selftests/arm64/.gitignore | 1 +
> tools/testing/selftests/arm64/Makefile | 22 ++++++++++
> .../testing/selftests/arm64/run_tags_test.sh | 12 ++++++
> tools/testing/selftests/arm64/tags_lib.c | 42 +++++++++++++++++++
> tools/testing/selftests/arm64/tags_test.c | 18 ++++++++
> 5 files changed, 95 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/.gitignore
> create mode 100644 tools/testing/selftests/arm64/Makefile
> create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> create mode 100644 tools/testing/selftests/arm64/tags_lib.c
> create mode 100644 tools/testing/selftests/arm64/tags_test.c
>
> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> new file mode 100644
> index 000000000000..e8fae8d61ed6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/.gitignore
> @@ -0,0 +1 @@
> +tags_test
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> new file mode 100644
> index 000000000000..9dee18727923
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +include ../lib.mk
> +
> +# ARCH can be overridden by the user for cross compiling
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> +
> +TEST_CUSTOM_PROGS := $(OUTPUT)/tags_test
> +
> +$(OUTPUT)/tags_test: tags_test.c $(OUTPUT)/tags_lib.so
> + $(CC) -o $@ $(CFLAGS) $(LDFLAGS) $<
> +
> +$(OUTPUT)/tags_lib.so: tags_lib.c
> + $(CC) -o $@ -shared $(CFLAGS) $(LDFLAGS) $^
> +
> +TEST_PROGS := run_tags_test.sh
> +
> +all: $(TEST_CUSTOM_PROGS)
> +
> +endif
> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> new file mode 100755
> index 000000000000..2bbe0cd4220b
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +echo "--------------------"
> +echo "running tags test"
> +echo "--------------------"
> +LD_PRELOAD=./tags_lib.so ./tags_test
> +if [ $? -ne 0 ]; then
> + echo "[FAIL]"
> +else
> + echo "[PASS]"
> +fi
> diff --git a/tools/testing/selftests/arm64/tags_lib.c b/tools/testing/selftests/arm64/tags_lib.c
> new file mode 100644
> index 000000000000..8a674509216e
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_lib.c
> @@ -0,0 +1,42 @@
> +#include <stdlib.h>
> +
> +#define TAG_SHIFT (56)
> +#define TAG_MASK (0xffUL << TAG_SHIFT)
> +
> +void *__libc_malloc(size_t size);
> +void __libc_free(void *ptr);
> +void *__libc_realloc(void *ptr, size_t size);
> +void *__libc_calloc(size_t nmemb, size_t size);
> +
> +static void *tag_ptr(void *ptr)
> +{
> + unsigned long tag = rand() & 0xff;
> + if (!ptr)
> + return ptr;
> + return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +}
> +
> +static void *untag_ptr(void *ptr)
> +{
> + return (void *)((unsigned long)ptr & ~TAG_MASK);
> +}
> +
> +void *malloc(size_t size)
> +{
> + return tag_ptr(__libc_malloc(size));
> +}
> +
> +void free(void *ptr)
> +{
> + __libc_free(untag_ptr(ptr));
> +}
> +
> +void *realloc(void *ptr, size_t size)
> +{
> + return tag_ptr(__libc_realloc(untag_ptr(ptr), size));
> +}
> +
> +void *calloc(size_t nmemb, size_t size)
> +{
> + return tag_ptr(__libc_calloc(nmemb, size));
> +}
> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> new file mode 100644
> index 000000000000..263b302874ed
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <sys/utsname.h>
> +
> +int main(void)
> +{
> + struct utsname *ptr;
> + int err;
> +
> + ptr = (struct utsname *)malloc(sizeof(*ptr));
> + err = uname(ptr);
> + free(ptr);
> + return err;
> +}
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 13/16] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get
From: Kees Cook @ 2019-06-08 3:52 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Mark Rutland, kvm, Szabolcs Nagy, Catalin Marinas, Will Deacon,
dri-devel, linux-mm, Khalid Aziz, linux-kselftest,
Mauro Carvalho Chehab, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, linux-rdma, amd-gfx,
Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov, Dave Martin,
Evgeniy Stepanov, linux-media, Kevin Brodsky, Ruben Ayrapetyan,
Ramana
In-Reply-To: <31821f3538ddacb7e57e0248e86a3d28f9789d2f.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:15PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> videobuf_dma_contig_user_get() uses provided user pointers for vma
> lookups, which can only by done with untagged pointers.
>
> Untag the pointers in this function.
>
> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> drivers/media/v4l2-core/videobuf-dma-contig.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index e1bf50df4c70..8a1ddd146b17 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -160,6 +160,7 @@ static void videobuf_dma_contig_user_put(struct videobuf_dma_contig_memory *mem)
> static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> struct videobuf_buffer *vb)
> {
> + unsigned long untagged_baddr = untagged_addr(vb->baddr);
> struct mm_struct *mm = current->mm;
> struct vm_area_struct *vma;
> unsigned long prev_pfn, this_pfn;
> @@ -167,22 +168,22 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> unsigned int offset;
> int ret;
>
> - offset = vb->baddr & ~PAGE_MASK;
> + offset = untagged_baddr & ~PAGE_MASK;
> mem->size = PAGE_ALIGN(vb->size + offset);
> ret = -EINVAL;
>
> down_read(&mm->mmap_sem);
>
> - vma = find_vma(mm, vb->baddr);
> + vma = find_vma(mm, untagged_baddr);
> if (!vma)
> goto out_up;
>
> - if ((vb->baddr + mem->size) > vma->vm_end)
> + if ((untagged_baddr + mem->size) > vma->vm_end)
> goto out_up;
>
> pages_done = 0;
> prev_pfn = 0; /* kill warning */
> - user_address = vb->baddr;
> + user_address = untagged_baddr;
>
> while (pages_done < (mem->size >> PAGE_SHIFT)) {
> ret = follow_pfn(vma, user_address, &this_pfn);
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v16 05/16] arm64: untag user pointers passed to memory syscalls
From: Kees Cook @ 2019-06-08 3:51 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Mark Rutland, kvm, Szabolcs Nagy, Catalin Marinas, Will Deacon,
dri-devel, linux-mm, Khalid Aziz, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Dave Martin, Evgeniy Stepanov, linux-media, Kevin Brodsky,
Ruben Ayrapetyan, Ramana Radhakrishnan, Alex
In-Reply-To: <045a94326401693e015bf80c444a4d946a5c68ed.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:07PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch allows tagged pointers to be passed to the following memory
> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
> mremap, msync, munlock.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/madvise.c | 2 ++
> mm/mempolicy.c | 3 +++
> mm/mincore.c | 2 ++
> mm/mlock.c | 4 ++++
> mm/mprotect.c | 2 ++
> mm/mremap.c | 2 ++
> mm/msync.c | 2 ++
> 7 files changed, 17 insertions(+)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 628022e674a7..39b82f8a698f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> size_t len;
> struct blk_plug plug;
>
> + start = untagged_addr(start);
> +
> if (!madvise_behavior_valid(behavior))
> return error;
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d80ae01..78e0a88b2680 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned long len,
> int err;
> unsigned short mode_flags;
>
> + start = untagged_addr(start);
> mode_flags = mode & MPOL_MODE_FLAGS;
> mode &= ~MPOL_MODE_FLAGS;
> if (mode >= MPOL_MAX)
> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
> int uninitialized_var(pval);
> nodemask_t nodes;
>
> + addr = untagged_addr(addr);
> +
> if (nmask != NULL && maxnode < nr_node_ids)
> return -EINVAL;
>
> diff --git a/mm/mincore.c b/mm/mincore.c
> index c3f058bd0faf..64c322ed845c 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
> unsigned long pages;
> unsigned char *tmp;
>
> + start = untagged_addr(start);
> +
> /* Check the start address: needs to be page-aligned.. */
> if (start & ~PAGE_MASK)
> return -EINVAL;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 080f3b36415b..e82609eaa428 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
> unsigned long lock_limit;
> int error = -ENOMEM;
>
> + start = untagged_addr(start);
> +
> if (!can_do_mlock())
> return -EPERM;
>
> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
> {
> int ret;
>
> + start = untagged_addr(start);
> +
> len = PAGE_ALIGN(len + (offset_in_page(start)));
> start &= PAGE_MASK;
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index bf38dfbbb4b4..19f981b733bc 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
> (prot & PROT_READ);
>
> + start = untagged_addr(start);
> +
> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
> return -EINVAL;
> diff --git a/mm/mremap.c b/mm/mremap.c
> index fc241d23cd97..1d98281f7204 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -606,6 +606,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> LIST_HEAD(uf_unmap_early);
> LIST_HEAD(uf_unmap);
>
> + addr = untagged_addr(addr);
> +
> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> return ret;
>
> diff --git a/mm/msync.c b/mm/msync.c
> index ef30a429623a..c3bd3e75f687 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
> int unmapped_error = 0;
> int error = -EINVAL;
>
> + start = untagged_addr(start);
> +
> if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> goto out;
> if (offset_in_page(start))
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
From: Kees Cook @ 2019-06-08 3:51 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Mark Rutland, kvm, Szabolcs Nagy, Catalin Marinas, Will Deacon,
dri-devel, linux-mm, Khalid Aziz, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Dave Martin, Evgeniy Stepanov, linux-media, Kevin Brodsky,
Ruben Ayrapetyan, Ramana Radhakrishnan, Alex
In-Reply-To: <4327b260fb17c4776a1e3c844f388e4948cfb747.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> copy_from_user (and a few other similar functions) are used to copy data
> from user memory into the kernel memory or vice versa. Since a user can
> provided a tagged pointer to one of the syscalls that use copy_from_user,
> we need to correctly handle such pointers.
>
> Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr,
> before performing access validity checks.
>
> Note, that this patch only temporarily untags the pointers to perform the
> checks, but then passes them as is into the kernel internals.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> arch/arm64/include/asm/uaccess.h | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index e5d5f31c6d36..9164ecb5feca 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
> return ret;
> }
>
> -#define access_ok(addr, size) __range_ok(addr, size)
> +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size)
> #define user_addr_max get_fs
>
> #define _ASM_EXTABLE(from, to) \
> @@ -226,7 +226,8 @@ static inline void uaccess_enable_not_uao(void)
>
> /*
> * Sanitise a uaccess pointer such that it becomes NULL if above the
> - * current addr_limit.
> + * current addr_limit. In case the pointer is tagged (has the top byte set),
> + * untag the pointer before checking.
> */
> #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
> static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> @@ -234,10 +235,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> void __user *safe_ptr;
>
> asm volatile(
> - " bics xzr, %1, %2\n"
> + " bics xzr, %3, %2\n"
> " csel %0, %1, xzr, eq\n"
> : "=&r" (safe_ptr)
> - : "r" (ptr), "r" (current_thread_info()->addr_limit)
> + : "r" (ptr), "r" (current_thread_info()->addr_limit),
> + "r" (untagged_addr(ptr))
> : "cc");
>
> csdb();
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 04/16] mm: untag user pointers in do_pages_move
From: Kees Cook @ 2019-06-08 3:49 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Catalin Marinas,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig, Mauro Carvalho Chehab,
Jens Wiklander
In-Reply-To: <e410843d00a4ecd7e525a7a949e605ffc6c394c4.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:06PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> do_pages_move() is used in the implementation of the move_pages syscall.
>
> Untag user pointers in this function.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/migrate.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f2ecc2855a12..3930bb6fa656 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1617,6 +1617,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> if (get_user(node, nodes + i))
> goto out_flush;
> addr = (unsigned long)p;
> + addr = untagged_addr(addr);
>
> err = -ENODEV;
> if (node < 0 || node >= MAX_NUMNODES)
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v16 03/16] lib, arm64: untag user pointers in strn*_user
From: Kees Cook @ 2019-06-08 3:48 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Mark Rutland, kvm, Szabolcs Nagy, Catalin Marinas, Will Deacon,
dri-devel, linux-mm, Khalid Aziz, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Dave Martin, Evgeniy Stepanov, linux-media, Kevin Brodsky,
Ruben Ayrapetyan, Ramana Radhakrishnan, Alex
In-Reply-To: <14f17ef1902aa4f07a39f96879394e718a1f5dc1.1559580831.git.andreyknvl@google.com>
On Mon, Jun 03, 2019 at 06:55:05PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> strncpy_from_user and strnlen_user accept user addresses as arguments, and
> do not go through the same path as copy_from_user and others, so here we
> need to handle the case of tagged user addresses separately.
>
> Untag user pointers passed to these functions.
>
> Note, that this patch only temporarily untags the pointers to perform
> validity checks, but then uses them as is to perform user memory accesses.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> lib/strncpy_from_user.c | 3 ++-
> lib/strnlen_user.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 023ba9f3b99f..dccb95af6003 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -6,6 +6,7 @@
> #include <linux/uaccess.h>
> #include <linux/kernel.h>
> #include <linux/errno.h>
> +#include <linux/mm.h>
>
> #include <asm/byteorder.h>
> #include <asm/word-at-a-time.h>
> @@ -108,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)src;
> + src_addr = (unsigned long)untagged_addr(src);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..28ff554a1be8 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,6 +2,7 @@
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/uaccess.h>
> +#include <linux/mm.h>
>
> #include <asm/word-at-a-time.h>
>
> @@ -109,7 +110,7 @@ long strnlen_user(const char __user *str, long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)str;
> + src_addr = (unsigned long)untagged_addr(str);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
From: Jason Gunthorpe @ 2019-06-08 2:12 UTC (permalink / raw)
To: Ralph Campbell
Cc: Andrea Arcangeli, linux-rdma, John Hubbard, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <61ea869d-43d2-d1e5-dc00-cf5e3e139169@nvidia.com>
On Fri, Jun 07, 2019 at 02:37:07PM -0700, Ralph Campbell wrote:
>
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > hmm_release() is called exactly once per hmm. ops->release() cannot
> > accidentally trigger any action that would recurse back onto
> > hmm->mirrors_sem.
> >
> > This fixes a use after-free race of the form:
> >
> > CPU0 CPU1
> > hmm_release()
> > up_write(&hmm->mirrors_sem);
> > hmm_mirror_unregister(mirror)
> > down_write(&hmm->mirrors_sem);
> > up_write(&hmm->mirrors_sem);
> > kfree(mirror)
> > mirror->ops->release(mirror)
> >
> > The only user we have today for ops->release is an empty function, so this
> > is unambiguously safe.
> >
> > As a consequence of plugging this race drivers are not allowed to
> > register/unregister mirrors from within a release op.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
>
> I agree with the analysis above but I'm not sure that release() will
> always be an empty function. It might be more efficient to write back
> all data migrated to a device "in one pass" instead of relying
> on unmap_vmas() calling hmm_start_range_invalidate() per VMA.
Sure, but it should not be allowed to recurse back to
hmm_mirror_unregister.
> I think the bigger issue is potential deadlocks while calling
> sync_cpu_device_pagetables() and tasks calling hmm_mirror_unregister():
>
> Say you have three threads:
> - Thread A is in try_to_unmap(), either without holding mmap_sem or with
> mmap_sem held for read.
> - Thread B has some unrelated driver calling hmm_mirror_unregister().
> This doesn't require mmap_sem.
> - Thread C is about to call migrate_vma().
>
> Thread A Thread B Thread C
> try_to_unmap hmm_mirror_unregister migrate_vma
> hmm_invalidate_range_start
> down_read(mirrors_sem)
> down_write(mirrors_sem)
> // Blocked on A
> device_lock
> device_lock
> // Blocked on C
> migrate_vma()
> hmm_invalidate_range_s
> down_read(mirrors_sem)
> // Blocked on B
> // Deadlock
Oh... you know I didn't know this about rwsems in linux that they have
a fairness policy for writes to block future reads..
Still, at least as things are designed, the driver cannot hold a lock
it obtains under sync_cpu_device_pagetables() and nest other things in
that lock. It certainly can't recurse back into any mmu notifiers
while holding that lock. (as you point out)
The lock in sync_cpu_device_pagetables() needs to be very narrowly
focused on updating device state only.
So, my first reaction is that the driver in thread C is wrong, and
needs a different locking scheme. I think you'd have to make a really
good case that there is no alternative for a driver..
> Perhaps we should consider using SRCU for walking the mirror->list?
It means the driver has to deal with races like in this patch
description. At that point there is almost no reason to insert hmm
here, just use mmu notifiers directly.
Drivers won't get this right, it is too hard.
Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Jason Gunthorpe @ 2019-06-08 1:47 UTC (permalink / raw)
To: Ralph Campbell
Cc: Andrea Arcangeli, linux-rdma, John Hubbard, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <ba55e382-c982-8e50-4ee7-7f05c9f7fafa@nvidia.com>
On Fri, Jun 07, 2019 at 03:13:00PM -0700, Ralph Campbell wrote:
> > Do you see a reason why the find_vma() ever needs to be before the
> > 'again' in my above example? range.vma does not need to be set for
> > range_register.
>
> Yes, for the GPU case, there can be many faults in an event queue
> and the goal is to try to handle more than one page at a time.
> The vma is needed to limit the amount of coalescing and checking
> for pages that could be speculatively migrated or mapped.
I'd need to see an algorithm sketch to see what you are thinking..
But, I guess a driver would have figure out a list of what virtual
pages it wants to fault under the mmap sem (maybe use find_vam, etc),
then drop mmap_sem, and start processing those pages for mirroring
under the hmm side.
ie they are two seperate unrelated tasks.
I looked at the hmm.rst again, and that reference algorithm is already
showing that that upon retry the mmap_sem is released:
take_lock(driver->update);
if (!range.valid) {
release_lock(driver->update);
up_read(&mm->mmap_sem);
goto again;
So a driver cannot assume that any VMA related work done under the
mmap before the hmm 'critical section' can carry into the hmm critical
section as the lock can be released upon retry and invalidate all that
data..
Forcing the hmm_range_start_and_lock() to acquire the mmap_sem is a
rough way to force the driver author to realize there are two locking
domains and lock protected information cannot cross between.
> OK, I understand.
> If you come up with a set of changes, I can try testing them.
Thanks, I make a sketch of the patch, I have to get back to it after
this series is done.
Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: John Hubbard @ 2019-06-08 1:37 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <20190607123432.GB14802@ziepe.ca>
On 6/7/19 5:34 AM, Jason Gunthorpe wrote:
> On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
>> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe <jgg@mellanox.com>
>> ...
>>> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>>>
>>> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>>> {
>>> - struct hmm *hmm = mm_get_hmm(mm);
>>> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>>> struct hmm_mirror *mirror;
>>> struct hmm_range *range;
>>>
>>> + /* hmm is in progress to free */
>>
>> Well, sometimes, yes. :)
>
> It think it is in all cases actually.. The only way we see a 0 kref
> and still reach this code path is if another thread has alreay setup
> the hmm_free in the call_srcu..
>
>> Maybe this wording is clearer (if we need any comment at all):
>
> I always find this hard.. This is a very standard pattern when working
> with RCU - however in my experience few people actually know the RCU
> patterns, and missing the _unless_zero is a common bug I find when
> looking at code.
>
> This is mm/ so I can drop it, what do you think?
>
I forgot to respond to this section, so catching up now:
I think we're talking about slightly different things. I was just
noting that the comment above the "if" statement was only accurate
if the branch is taken, which is why I recommended this combination
of comment and code:
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
As for the actual _unless_zero part, I think that's good to have.
And it's a good reminder if nothing else, even in mm/ code.
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 12/11] mm/hmm: Fix error flows in hmm_invalidate_range_start
From: Jason Gunthorpe @ 2019-06-08 1:35 UTC (permalink / raw)
To: Ralph Campbell
Cc: Andrea Arcangeli, linux-rdma, John Hubbard, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <439b5731-0b7e-b25b-ce1a-74b34e1f9bf5@nvidia.com>
On Fri, Jun 07, 2019 at 04:52:58PM -0700, Ralph Campbell wrote:
> > @@ -141,6 +142,23 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> > hmm_put(hmm);
> > }
> > +static void notifiers_decrement(struct hmm *hmm)
> > +{
> > + lockdep_assert_held(&hmm->ranges_lock);
> > +
> > + hmm->notifiers--;
> > + if (!hmm->notifiers) {
> > + struct hmm_range *range;
> > +
> > + list_for_each_entry(range, &hmm->ranges, list) {
> > + if (range->valid)
> > + continue;
> > + range->valid = true;
> > + }
>
> This just effectively sets all ranges to valid.
> I'm not sure that is best.
This is a trade off, it would be much more expensive to have a precise
'valid = true' - instead this algorithm is precise about 'valid =
false' and lazy about 'valid = true' which is much less costly to
calculate.
> Shouldn't hmm_range_register() start with range.valid = true and
> then hmm_invalidate_range_start() set affected ranges to false?
It kind of does, expect when it doesn't, right? :)
> Then this becomes just wake_up_all() if --notifiers == 0 and
> hmm_range_wait_until_valid() should wait for notifiers == 0.
Almost.. but it is more tricky than that.
This scheme is a collision-retry algorithm. The pagefault side runs to
completion if no parallel invalidate start/end happens.
If a parallel invalidation happens then the pagefault retries.
Seeing notifiers == 0 means there is absolutely no current parallel
invalidation.
Seeing range->valid == true (under the device lock)
means this range doesn't intersect with a parallel invalidate.
So.. hmm_range_wait_until_valid() checks the per-range valid because
it doesn't want to sleep if *this range* is not involved in a parallel
invalidation - but once it becomes involved, then yes, valid == true
implies notifiers == 0.
It is easier/safer to use unlocked variable reads if there is only one
variable, thus the weird construction.
It is unclear to me if this micro optimization is really
worthwhile. It is very expensive to manage all this tracking, and no
other mmu notifier implementation really does something like
this. Eliminating the per-range tracking and using the notifier count
as a global lock would be much simpler...
> Otherwise, range.valid doesn't really mean it's valid.
Right, it doesn't really mean 'valid'
It is tracking possible colliding invalidates such that valid == true
(under the device lock) means that there was no colliding invalidate.
I still think this implementation doesn't quite work, as I described
here:
https://lore.kernel.org/linux-mm/20190527195829.GB18019@mellanox.com/
But the idea is basically sound and matches what other mmu notifier
users do, just using a seqcount like scheme, not a boolean.
Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: John Hubbard @ 2019-06-08 1:32 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <20190607133107.GF14802@ziepe.ca>
On 6/7/19 6:31 AM, Jason Gunthorpe wrote:
> The wait_event_timeout macro already tests the condition as its first
> action, so there is no reason to open code another version of this, all
> that does is skip the might_sleep() debugging in common cases, which is
> not helpful.
>
> Further, based on prior patches, we can now simplify the required condition
> test:
> - If range is valid memory then so is range->hmm
> - If hmm_release() has run then range->valid is set to false
> at the same time as dead, so no reason to check both.
> - A valid hmm has a valid hmm->mm.
>
> Allowing the return value of wait_event_timeout() (along with its internal
> barriers) to compute the result of the function.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
> v3
> - Simplify the wait_event_timeout to not check valid
> ---
> include/linux/hmm.h | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 1d97b6d62c5bcf..26e7c477490c4e 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
> static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> unsigned long timeout)
> {
> - /* Check if mm is dead ? */
> - if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> - range->valid = false;
> - return false;
> - }
> - if (range->valid)
> - return true;
> - wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> - msecs_to_jiffies(timeout));
> - /* Return current valid status just in case we get lucky */
> - return range->valid;
> + return wait_event_timeout(range->hmm->wq, range->valid,
> + msecs_to_jiffies(timeout)) != 0;
> }
>
> /*
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: John Hubbard @ 2019-06-08 1:13 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, Felix.Kuehling,
dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <20190607123432.GB14802@ziepe.ca>
On 6/7/19 5:34 AM, Jason Gunthorpe wrote:
> On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
>> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe <jgg@mellanox.com>
>> ...
>>> diff --git a/mm/hmm.c b/mm/hmm.c
>>> index 8e7403f081f44a..547002f56a163d 100644
>>> +++ b/mm/hmm.c
>> ...
>>> @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
>>> mm->hmm = NULL;
>>> spin_unlock(&mm->page_table_lock);
>>>
>>> - kfree(hmm);
>>> + mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
>>
>>
>> It occurred to me to wonder if it is best to use the MMU notifier's
>> instance of srcu, instead of creating a separate instance for HMM.
>
> It *has* to be the MMU notifier SRCU because we are synchornizing
> against the read side of that SRU inside the mmu notifier code, ie:
>
> int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range)
> id = srcu_read_lock(&srcu);
> hlist_for_each_entry_rcu(mn, &range->mm->mmu_notifier_mm->list, hlist) {
> if (mn->ops->invalidate_range_start) {
> ^^^^^
>
> Here 'mn' is really hmm (hmm = container_of(mn, struct hmm,
> mmu_notifier)), so we must protect the memory against free for the mmu
> notifier core.
>
> Thus we have no choice but to use its SRCU.
Ah right. It's embarassingly obvious when you say it out loud. :)
Thanks for explaining.
>
> CH also pointed out a more elegant solution, which is to get the write
> side of the mmap_sem during hmm_mirror_unregister - no notifier
> callback can be running in this case. Then we delete the kref, srcu
> and so forth.
>
> This is much clearer/saner/better, but.. requries the callers of
> hmm_mirror_unregister to be safe to get the mmap_sem write side.
>
> I think this is true, so maybe this patch should be switched, what do
> you think?
OK, your follow-up notes that we'll leave it as is, got it.
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
From: Ralph Campbell @ 2019-06-08 0:14 UTC (permalink / raw)
To: Jerome Glisse, John Hubbard, Felix.Kuehling, Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, amd-gfx, linux-mm,
dri-devel
HMM defines its own struct hmm_update which is passed to the
sync_cpu_device_pagetables() callback function. This is
sufficient when the only action is to invalidate. However,
a device may want to know the reason for the invalidation and
be able to see the new permissions on a range, update device access
rights or range statistics. Since sync_cpu_device_pagetables()
can be called from try_to_unmap(), the mmap_sem may not be held
and find_vma() is not safe to be called.
Pass the struct mmu_notifier_range to sync_cpu_device_pagetables()
to allow the full invalidation information to be used.
Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
---
I'm sending this out now since we are updating many of the HMM APIs
and I think it will be useful.
drivers/gpu/drm/nouveau/nouveau_svm.c | 4 ++--
include/linux/hmm.h | 27 ++-------------------------
mm/hmm.c | 13 ++++---------
3 files changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 8c92374afcf2..c34b98fafe2f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -252,13 +252,13 @@ nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit)
static int
nouveau_svmm_sync_cpu_device_pagetables(struct hmm_mirror *mirror,
- const struct hmm_update *update)
+ const struct mmu_notifier_range *update)
{
struct nouveau_svmm *svmm = container_of(mirror, typeof(*svmm), mirror);
unsigned long start = update->start;
unsigned long limit = update->end;
- if (!update->blockable)
+ if (!mmu_notifier_range_blockable(update))
return -EAGAIN;
SVMM_DBG(svmm, "invalidate %016lx-%016lx", start, limit);
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 0fa8ea34ccef..07a2d38fde34 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -377,29 +377,6 @@ static inline uint64_t hmm_pfn_from_pfn(const struct hmm_range *range,
struct hmm_mirror;
-/*
- * enum hmm_update_event - type of update
- * @HMM_UPDATE_INVALIDATE: invalidate range (no indication as to why)
- */
-enum hmm_update_event {
- HMM_UPDATE_INVALIDATE,
-};
-
-/*
- * struct hmm_update - HMM update information for callback
- *
- * @start: virtual start address of the range to update
- * @end: virtual end address of the range to update
- * @event: event triggering the update (what is happening)
- * @blockable: can the callback block/sleep ?
- */
-struct hmm_update {
- unsigned long start;
- unsigned long end;
- enum hmm_update_event event;
- bool blockable;
-};
-
/*
* struct hmm_mirror_ops - HMM mirror device operations callback
*
@@ -420,7 +397,7 @@ struct hmm_mirror_ops {
/* sync_cpu_device_pagetables() - synchronize page tables
*
* @mirror: pointer to struct hmm_mirror
- * @update: update information (see struct hmm_update)
+ * @update: update information (see struct mmu_notifier_range)
* Return: -EAGAIN if update.blockable false and callback need to
* block, 0 otherwise.
*
@@ -434,7 +411,7 @@ struct hmm_mirror_ops {
* synchronous call.
*/
int (*sync_cpu_device_pagetables)(struct hmm_mirror *mirror,
- const struct hmm_update *update);
+ const struct mmu_notifier_range *update);
};
/*
diff --git a/mm/hmm.c b/mm/hmm.c
index 9aad3550f2bb..b49a43712554 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -164,7 +164,6 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
- struct hmm_update update;
struct hmm_range *range;
unsigned long flags;
int ret = 0;
@@ -173,15 +172,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
if (!kref_get_unless_zero(&hmm->kref))
return 0;
- update.start = nrange->start;
- update.end = nrange->end;
- update.event = HMM_UPDATE_INVALIDATE;
- update.blockable = mmu_notifier_range_blockable(nrange);
-
spin_lock_irqsave(&hmm->ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, &hmm->ranges, list) {
- if (update.end < range->start || update.start >= range->end)
+ if (nrange->end < range->start || nrange->start >= range->end)
continue;
range->valid = false;
@@ -198,9 +192,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
list_for_each_entry(mirror, &hmm->mirrors, list) {
int rc;
- rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
+ rc = mirror->ops->sync_cpu_device_pagetables(mirror, nrange);
if (rc) {
- if (WARN_ON(update.blockable || rc != -EAGAIN))
+ if (WARN_ON(mmu_notifier_range_blockable(nrange) ||
+ rc != -EAGAIN))
continue;
ret = -EAGAIN;
break;
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-08 0:10 UTC (permalink / raw)
To: Ira Weiny
Cc: Jason Gunthorpe, Theodore Ts'o,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Jérôme Glisse, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Jan Kara, linux-ext4-u79uwXL29TY76Z2rM5mHXA, Andrew Morton
In-Reply-To: <20190607182534.GC14559-J5EW/p2F9lUlb2qzJQmr9q2pdiUAq4bhAL8bYrjMMd8@public.gmane.org>
On Fri, Jun 07, 2019 at 11:25:35AM -0700, Ira Weiny wrote:
> On Fri, Jun 07, 2019 at 01:04:26PM +0200, Jan Kara wrote:
> > On Thu 06-06-19 15:03:30, Ira Weiny wrote:
> > > On Thu, Jun 06, 2019 at 12:42:03PM +0200, Jan Kara wrote:
> > > > On Wed 05-06-19 18:45:33, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> > > > > From: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > >
> > > > So I'd like to actually mandate that you *must* hold the file lease until
> > > > you unpin all pages in the given range (not just that you have an option to
> > > > hold a lease). And I believe the kernel should actually enforce this. That
> > > > way we maintain a sane state that if someone uses a physical location of
> > > > logical file offset on disk, he has a layout lease. Also once this is done,
> > > > sysadmin has a reasonably easy way to discover run-away RDMA application
> > > > and kill it if he wishes so.
> > >
> > > Fair enough.
> > >
> > > I was kind of heading that direction but had not thought this far forward. I
> > > was exploring how to have a lease remain on the file even after a "lease
> > > break". But that is incompatible with the current semantics of a "layout"
> > > lease (as currently defined in the kernel). [In the end I wanted to get an RFC
> > > out to see what people think of this idea so I did not look at keeping the
> > > lease.]
> > >
> > > Also hitch is that currently a lease is forcefully broken after
> > > <sysfs>/lease-break-time. To do what you suggest I think we would need a new
> > > lease type with the semantics you describe.
> >
> > I'd do what Dave suggested - add flag to mark lease as unbreakable by
> > truncate and teach file locking core to handle that. There actually is
> > support for locks that are not broken after given timeout so there
> > shouldn't be too many changes need.
> >
> > > Previously I had thought this would be a good idea (for other reasons). But
> > > what does everyone think about using a "longterm lease" similar to [1] which
> > > has the semantics you proppose? In [1] I was not sure "longterm" was a good
> > > name but with your proposal I think it makes more sense.
> >
> > As I wrote elsewhere in this thread I think FL_LAYOUT name still makes
> > sense and I'd add there FL_UNBREAKABLE to mark unusal behavior with
> > truncate.
>
> Ok I want to make sure I understand what you and Dave are suggesting.
>
> Are you suggesting that we have something like this from user space?
>
> fcntl(fd, F_SETLEASE, F_LAYOUT | F_UNBREAKABLE);
Rather than "unbreakable", perhaps a clearer description of the
policy it entails is "exclusive"?
i.e. what we are talking about here is an exclusive lease that
prevents other processes from changing the layout. i.e. the
mechanism used to guarantee a lease is exclusive is that the layout
becomes "unbreakable" at the filesystem level, but the policy we are
actually presenting to uses is "exclusive access"...
Cheers,
Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [PATCH v2 12/11] mm/hmm: Fix error flows in hmm_invalidate_range_start
From: Ralph Campbell @ 2019-06-07 23:52 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, John Hubbard, Felix.Kuehling
Cc: linux-rdma, linux-mm, Andrea Arcangeli, amd-gfx, dri-devel
In-Reply-To: <20190607160557.GA335@ziepe.ca>
On 6/7/19 9:05 AM, Jason Gunthorpe wrote:
> If the trylock on the hmm->mirrors_sem fails the function will return
> without decrementing the notifiers that were previously incremented. Since
> the caller will not call invalidate_range_end() on EAGAIN this will result
> in notifiers becoming permanently incremented and deadlock.
>
> If the sync_cpu_device_pagetables() required blocking the function will
> not return EAGAIN even though the device continues to touch the
> pages. This is a violation of the mmu notifier contract.
>
> Switch, and rename, the ranges_lock to a spin lock so we can reliably
> obtain it without blocking during error unwind.
>
> The error unwind is necessary since the notifiers count must be held
> incremented across the call to sync_cpu_device_pagetables() as we cannot
> allow the range to become marked valid by a parallel
> invalidate_start/end() pair while doing sync_cpu_device_pagetables().
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> ---
> include/linux/hmm.h | 2 +-
> mm/hmm.c | 77 +++++++++++++++++++++++++++------------------
> 2 files changed, 48 insertions(+), 31 deletions(-)
>
> I almost lost this patch - it is part of the series, hasn't been
> posted before, and wasn't sent with the rest, sorry.
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index bf013e96525771..0fa8ea34ccef6d 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -86,7 +86,7 @@
> struct hmm {
> struct mm_struct *mm;
> struct kref kref;
> - struct mutex lock;
> + spinlock_t ranges_lock;
> struct list_head ranges;
> struct list_head mirrors;
> struct mmu_notifier mmu_notifier;
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 4215edf737ef5b..10103a24e9b7b3 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -68,7 +68,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> init_rwsem(&hmm->mirrors_sem);
> hmm->mmu_notifier.ops = NULL;
> INIT_LIST_HEAD(&hmm->ranges);
> - mutex_init(&hmm->lock);
> + spin_lock_init(&hmm->ranges_lock);
> kref_init(&hmm->kref);
> hmm->notifiers = 0;
> hmm->mm = mm;
> @@ -114,18 +114,19 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> + unsigned long flags;
>
> /* Bail out if hmm is in the process of being freed */
> if (!kref_get_unless_zero(&hmm->kref))
> return;
>
> - mutex_lock(&hmm->lock);
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
> /*
> * Since hmm_range_register() holds the mmget() lock hmm_release() is
> * prevented as long as a range exists.
> */
> WARN_ON(!list_empty(&hmm->ranges));
> - mutex_unlock(&hmm->lock);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
>
> down_read(&hmm->mirrors_sem);
> list_for_each_entry(mirror, &hmm->mirrors, list) {
> @@ -141,6 +142,23 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> hmm_put(hmm);
> }
>
> +static void notifiers_decrement(struct hmm *hmm)
> +{
> + lockdep_assert_held(&hmm->ranges_lock);
> +
> + hmm->notifiers--;
> + if (!hmm->notifiers) {
> + struct hmm_range *range;
> +
> + list_for_each_entry(range, &hmm->ranges, list) {
> + if (range->valid)
> + continue;
> + range->valid = true;
> + }
This just effectively sets all ranges to valid.
I'm not sure that is best.
Shouldn't hmm_range_register() start with range.valid = true and
then hmm_invalidate_range_start() set affected ranges to false?
Then this becomes just wake_up_all() if --notifiers == 0 and
hmm_range_wait_until_valid() should wait for notifiers == 0.
Otherwise, range.valid doesn't really mean it's valid.
> + wake_up_all(&hmm->wq);
> + }
> +}
> +
> static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> @@ -148,6 +166,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> struct hmm_mirror *mirror;
> struct hmm_update update;
> struct hmm_range *range;
> + unsigned long flags;
> int ret = 0;
>
> if (!kref_get_unless_zero(&hmm->kref))
> @@ -158,12 +177,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> update.event = HMM_UPDATE_INVALIDATE;
> update.blockable = mmu_notifier_range_blockable(nrange);
>
> - if (mmu_notifier_range_blockable(nrange))
> - mutex_lock(&hmm->lock);
> - else if (!mutex_trylock(&hmm->lock)) {
> - ret = -EAGAIN;
> - goto out;
> - }
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
> hmm->notifiers++;
> list_for_each_entry(range, &hmm->ranges, list) {
> if (update.end < range->start || update.start >= range->end)
> @@ -171,7 +185,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
>
> range->valid = false;
> }
> - mutex_unlock(&hmm->lock);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
>
> if (mmu_notifier_range_blockable(nrange))
> down_read(&hmm->mirrors_sem);
> @@ -179,16 +193,26 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> ret = -EAGAIN;
> goto out;
> }
> +
> list_for_each_entry(mirror, &hmm->mirrors, list) {
> - int ret;
> + int rc;
>
> - ret = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
> - if (!update.blockable && ret == -EAGAIN)
> + rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
> + if (rc) {
> + if (WARN_ON(update.blockable || rc != -EAGAIN))
> + continue;
> + ret = -EAGAIN;
> break;
> + }
> }
> up_read(&hmm->mirrors_sem);
>
> out:
> + if (ret) {
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
> + notifiers_decrement(hmm);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
> + }
> hmm_put(hmm);
> return ret;
> }
> @@ -197,23 +221,14 @@ static void hmm_invalidate_range_end(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> + unsigned long flags;
>
> if (!kref_get_unless_zero(&hmm->kref))
> return;
>
> - mutex_lock(&hmm->lock);
> - hmm->notifiers--;
> - if (!hmm->notifiers) {
> - struct hmm_range *range;
> -
> - list_for_each_entry(range, &hmm->ranges, list) {
> - if (range->valid)
> - continue;
> - range->valid = true;
> - }
> - wake_up_all(&hmm->wq);
> - }
> - mutex_unlock(&hmm->lock);
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
> + notifiers_decrement(hmm);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
>
> hmm_put(hmm);
> }
> @@ -866,6 +881,7 @@ int hmm_range_register(struct hmm_range *range,
> {
> unsigned long mask = ((1UL << page_shift) - 1UL);
> struct hmm *hmm = mirror->hmm;
> + unsigned long flags;
>
> range->valid = false;
> range->hmm = NULL;
> @@ -887,7 +903,7 @@ int hmm_range_register(struct hmm_range *range,
> kref_get(&hmm->kref);
>
> /* Initialize range to track CPU page table updates. */
> - mutex_lock(&hmm->lock);
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
>
> range->hmm = hmm;
> list_add(&range->list, &hmm->ranges);
> @@ -898,7 +914,7 @@ int hmm_range_register(struct hmm_range *range,
> */
> if (!hmm->notifiers)
> range->valid = true;
> - mutex_unlock(&hmm->lock);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
>
> return 0;
> }
> @@ -914,13 +930,14 @@ EXPORT_SYMBOL(hmm_range_register);
> void hmm_range_unregister(struct hmm_range *range)
> {
> struct hmm *hmm = range->hmm;
> + unsigned long flags;
>
> if (WARN_ON(range->end <= range->start))
> return;
>
> - mutex_lock(&hmm->lock);
> + spin_lock_irqsave(&hmm->ranges_lock, flags);
> list_del(&range->list);
> - mutex_unlock(&hmm->lock);
> + spin_unlock_irqrestore(&hmm->ranges_lock, flags);
>
> /* Drop reference taken by hmm_range_register() */
> range->valid = false;
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
From: Ira Weiny @ 2019-06-07 23:02 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
Jason Gunthorpe, amd-gfx
In-Reply-To: <20190606184438.31646-11-jgg@ziepe.ca>
On Thu, Jun 06, 2019 at 03:44:37PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> This list is always read and written while holding hmm->lock so there is
> no need for the confusing _rcu annotations.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
Reviewed-by: Ira Weiny <iweiny@intel.com>
> ---
> mm/hmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index c2fecb3ecb11e1..709d138dd49027 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range,
> mutex_lock(&hmm->lock);
>
> range->hmm = hmm;
> - list_add_rcu(&range->list, &hmm->ranges);
> + list_add(&range->list, &hmm->ranges);
>
> /*
> * If there are any concurrent notifiers we have to wait for them for
> @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range)
> return;
>
> mutex_lock(&hmm->lock);
> - list_del_rcu(&range->list);
> + list_del(&range->list);
> mutex_unlock(&hmm->lock);
>
> /* Drop reference taken by hmm_range_register() */
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
From: Ira Weiny @ 2019-06-07 23:01 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
Jason Gunthorpe, amd-gfx
In-Reply-To: <20190606184438.31646-10-jgg@ziepe.ca>
On Thu, Jun 06, 2019 at 03:44:36PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON
> and poison bytes to detect this condition.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> v2
> - Keep range start/end valid after unregistration (Jerome)
> ---
> mm/hmm.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 6802de7080d172..c2fecb3ecb11e1 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range)
> struct hmm *hmm = range->hmm;
>
> /* Sanity check this really should not happen. */
> - if (hmm == NULL || range->end <= range->start)
> + if (WARN_ON(range->end <= range->start))
> return;
>
> mutex_lock(&hmm->lock);
> @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range)
> range->valid = false;
> mmput(hmm->mm);
> hmm_put(hmm);
> - range->hmm = NULL;
> +
> + /* The range is now invalid, leave it poisoned. */
> + range->valid = false;
No need to set valid false again as you just did this 5 lines above.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
> }
> EXPORT_SYMBOL(hmm_range_unregister);
>
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Ira Weiny @ 2019-06-07 22:55 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse, amd-gfx
In-Reply-To: <20190607133107.GF14802@ziepe.ca>
On Fri, Jun 07, 2019 at 10:31:07AM -0300, Jason Gunthorpe wrote:
> The wait_event_timeout macro already tests the condition as its first
> action, so there is no reason to open code another version of this, all
> that does is skip the might_sleep() debugging in common cases, which is
> not helpful.
>
> Further, based on prior patches, we can now simplify the required condition
> test:
> - If range is valid memory then so is range->hmm
> - If hmm_release() has run then range->valid is set to false
> at the same time as dead, so no reason to check both.
> - A valid hmm has a valid hmm->mm.
>
> Allowing the return value of wait_event_timeout() (along with its internal
> barriers) to compute the result of the function.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> v3
> - Simplify the wait_event_timeout to not check valid
> ---
> include/linux/hmm.h | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 1d97b6d62c5bcf..26e7c477490c4e 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
> static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> unsigned long timeout)
> {
> - /* Check if mm is dead ? */
> - if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> - range->valid = false;
> - return false;
> - }
> - if (range->valid)
> - return true;
> - wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> - msecs_to_jiffies(timeout));
> - /* Return current valid status just in case we get lucky */
> - return range->valid;
> + return wait_event_timeout(range->hmm->wq, range->valid,
> + msecs_to_jiffies(timeout)) != 0;
> }
>
> /*
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 04/11] mm/hmm: Simplify hmm_get_or_create and make it reliable
From: Ira Weiny @ 2019-06-07 22:44 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
Jason Gunthorpe, amd-gfx
In-Reply-To: <20190606184438.31646-5-jgg@ziepe.ca>
On Thu, Jun 06, 2019 at 03:44:31PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> As coded this function can false-fail in various racy situations. Make it
> reliable by running only under the write side of the mmap_sem and avoiding
> the false-failing compare/exchange pattern.
>
> Also make the locking very easy to understand by only ever reading or
> writing mm->hmm while holding the write side of the mmap_sem.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> v2:
> - Fix error unwind of mmgrab (Jerome)
> - Use hmm local instead of 2nd container_of (Jerome)
> ---
> mm/hmm.c | 80 ++++++++++++++++++++------------------------------------
> 1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index cc7c26fda3300e..dc30edad9a8a02 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -40,16 +40,6 @@
> #if IS_ENABLED(CONFIG_HMM_MIRROR)
> static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
>
> -static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> -{
> - struct hmm *hmm = READ_ONCE(mm->hmm);
> -
> - if (hmm && kref_get_unless_zero(&hmm->kref))
> - return hmm;
> -
> - return NULL;
> -}
> -
> /**
> * hmm_get_or_create - register HMM against an mm (HMM internal)
> *
> @@ -64,11 +54,20 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> */
> static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> - bool cleanup = false;
> + struct hmm *hmm;
>
> - if (hmm)
> - return hmm;
> + lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> + if (mm->hmm) {
> + if (kref_get_unless_zero(&mm->hmm->kref))
> + return mm->hmm;
> + /*
> + * The hmm is being freed by some other CPU and is pending a
> + * RCU grace period, but this CPU can NULL now it since we
> + * have the mmap_sem.
> + */
> + mm->hmm = NULL;
> + }
>
> hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
> if (!hmm)
> @@ -83,57 +82,36 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> - mmgrab(hmm->mm);
> -
> - spin_lock(&mm->page_table_lock);
> - if (!mm->hmm)
> - mm->hmm = hmm;
> - else
> - cleanup = true;
> - spin_unlock(&mm->page_table_lock);
>
> - if (cleanup)
> - goto error;
> -
> - /*
> - * We should only get here if hold the mmap_sem in write mode ie on
> - * registration of first mirror through hmm_mirror_register()
> - */
> hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
> - if (__mmu_notifier_register(&hmm->mmu_notifier, mm))
> - goto error_mm;
> + if (__mmu_notifier_register(&hmm->mmu_notifier, mm)) {
> + kfree(hmm);
> + return NULL;
> + }
>
> + mmgrab(hmm->mm);
> + mm->hmm = hmm;
> return hmm;
> -
> -error_mm:
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -error:
> - mmdrop(hmm->mm);
> - kfree(hmm);
> - return NULL;
> }
>
> static void hmm_free_rcu(struct rcu_head *rcu)
> {
> - kfree(container_of(rcu, struct hmm, rcu));
> + struct hmm *hmm = container_of(rcu, struct hmm, rcu);
> +
> + down_write(&hmm->mm->mmap_sem);
> + if (hmm->mm->hmm == hmm)
> + hmm->mm->hmm = NULL;
> + up_write(&hmm->mm->mmap_sem);
> + mmdrop(hmm->mm);
> +
> + kfree(hmm);
> }
>
> static void hmm_free(struct kref *kref)
> {
> struct hmm *hmm = container_of(kref, struct hmm, kref);
> - struct mm_struct *mm = hmm->mm;
> -
> - mmu_notifier_unregister_no_release(&hmm->mmu_notifier, mm);
>
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -
> - mmdrop(hmm->mm);
> + mmu_notifier_unregister_no_release(&hmm->mmu_notifier, hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
From: Ralph Campbell @ 2019-06-07 22:39 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <4a391bd4-287c-5f13-3bca-c6a46ff8d08c@nvidia.com>
On 6/7/19 11:24 AM, Ralph Campbell wrote:
>
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>> From: Jason Gunthorpe <jgg@mellanox.com>
>>
>> Ralph observes that hmm_range_register() can only be called by a driver
>> while a mirror is registered. Make this clear in the API by passing in
>> the
>> mirror structure as a parameter.
>>
>> This also simplifies understanding the lifetime model for struct hmm, as
>> the hmm pointer must be valid as part of a registered mirror so all we
>> need in hmm_register_range() is a simple kref_get.
>>
>> Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
>> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
>
> You might CC Ben for the nouveau part.
> CC: Ben Skeggs <bskeggs@redhat.com>
>
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
>
>
>> ---
>> v2
>> - Include the oneline patch to nouveau_svm.c
>> ---
>> drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
>> include/linux/hmm.h | 7 ++++---
>> mm/hmm.c | 15 ++++++---------
>> 3 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
>> b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> index 93ed43c413f0bb..8c92374afcf227 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> @@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
>> range.values = nouveau_svm_pfn_values;
>> range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
>> again:
>> - ret = hmm_vma_fault(&range, true);
>> + ret = hmm_vma_fault(&svmm->mirror, &range, true);
>> if (ret == 0) {
>> mutex_lock(&svmm->mutex);
>> if (!hmm_vma_range_done(&range)) {
>> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
>> index 688c5ca7068795..2d519797cb134a 100644
>> --- a/include/linux/hmm.h
>> +++ b/include/linux/hmm.h
>> @@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct
>> hmm_mirror *mirror)
>> * Please see Documentation/vm/hmm.rst for how to use the range API.
>> */
>> int hmm_range_register(struct hmm_range *range,
>> - struct mm_struct *mm,
>> + struct hmm_mirror *mirror,
>> unsigned long start,
>> unsigned long end,
>> unsigned page_shift);
>> @@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct
>> hmm_range *range)
>> }
>> /* This is a temporary helper to avoid merge conflict between trees. */
>> -static inline int hmm_vma_fault(struct hmm_range *range, bool block)
>> +static inline int hmm_vma_fault(struct hmm_mirror *mirror,
>> + struct hmm_range *range, bool block)
>> {
>> long ret;
>> @@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range
>> *range, bool block)
>> range->default_flags = 0;
>> range->pfn_flags_mask = -1UL;
>> - ret = hmm_range_register(range, range->vma->vm_mm,
>> + ret = hmm_range_register(range, mirror,
>> range->start, range->end,
>> PAGE_SHIFT);
>> if (ret)
>> diff --git a/mm/hmm.c b/mm/hmm.c
>> index 547002f56a163d..8796447299023c 100644
>> --- a/mm/hmm.c
>> +++ b/mm/hmm.c
>> @@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
>> * Track updates to the CPU page table see include/linux/hmm.h
>> */
>> int hmm_range_register(struct hmm_range *range,
>> - struct mm_struct *mm,
>> + struct hmm_mirror *mirror,
>> unsigned long start,
>> unsigned long end,
>> unsigned page_shift)
>> {
>> unsigned long mask = ((1UL << page_shift) - 1UL);
>> - struct hmm *hmm;
>> + struct hmm *hmm = mirror->hmm;
>> range->valid = false;
>> range->hmm = NULL;
>> @@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
>> range->start = start;
>> range->end = end;
>> - hmm = hmm_get_or_create(mm);
>> - if (!hmm)
>> - return -EFAULT;
>> -
>> /* Check if hmm_mm_destroy() was call. */
>> - if (hmm->mm == NULL || hmm->dead) {
>> - hmm_put(hmm);
>> + if (hmm->mm == NULL || hmm->dead)
>> return -EFAULT;
>> - }
>> +
>> + range->hmm = hmm;
>> + kref_get(&hmm->kref);
>> /* Initialize range to track CPU page table updates. */
>> mutex_lock(&hmm->lock);
>>
I forgot to add that I think you can delete the duplicate
"range->hmm = hmm;"
here between the mutex_lock/unlock.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 03/11] mm/hmm: Hold a mmgrab from hmm to mm
From: Ira Weiny @ 2019-06-07 22:38 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
Jason Gunthorpe, amd-gfx
In-Reply-To: <20190606184438.31646-4-jgg@ziepe.ca>
On Thu, Jun 06, 2019 at 03:44:30PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> So long a a struct hmm pointer exists, so should the struct mm it is
> linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
> once the hmm refcount goes to zero.
>
> Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
> mm->hmm delete the hmm_hmm_destroy().
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> v2:
> - Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
> ---
> include/linux/hmm.h | 3 ---
> kernel/fork.c | 1 -
> mm/hmm.c | 22 ++++------------------
> 3 files changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 2d519797cb134a..4ee3acabe5ed22 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> }
>
> /* Below are for HMM internal use only! Not to be used by device driver! */
> -void hmm_mm_destroy(struct mm_struct *mm);
> -
> static inline void hmm_mm_init(struct mm_struct *mm)
> {
> mm->hmm = NULL;
> }
> #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> -static inline void hmm_mm_destroy(struct mm_struct *mm) {}
> static inline void hmm_mm_init(struct mm_struct *mm) {}
> #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b2b87d450b80b5..588c768ae72451 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
> WARN_ON_ONCE(mm == current->active_mm);
> mm_free_pgd(mm);
> destroy_context(mm);
> - hmm_mm_destroy(mm);
> mmu_notifier_mm_destroy(mm);
> check_mm(mm);
> put_user_ns(mm->user_ns);
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 8796447299023c..cc7c26fda3300e 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -29,6 +29,7 @@
> #include <linux/swapops.h>
> #include <linux/hugetlb.h>
> #include <linux/memremap.h>
> +#include <linux/sched/mm.h>
> #include <linux/jump_label.h>
> #include <linux/dma-mapping.h>
> #include <linux/mmu_notifier.h>
> @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> + mmgrab(hmm->mm);
>
> spin_lock(&mm->page_table_lock);
> if (!mm->hmm)
> @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
> error:
> + mmdrop(hmm->mm);
> kfree(hmm);
> return NULL;
> }
> @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> + mmdrop(hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
> @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm)
> kref_put(&hmm->kref, hmm_free);
> }
>
> -void hmm_mm_destroy(struct mm_struct *mm)
> -{
> - struct hmm *hmm;
> -
> - spin_lock(&mm->page_table_lock);
> - hmm = mm_get_hmm(mm);
> - mm->hmm = NULL;
> - if (hmm) {
> - hmm->mm = NULL;
> - hmm->dead = true;
> - spin_unlock(&mm->page_table_lock);
> - hmm_put(hmm);
> - return;
> - }
> -
> - spin_unlock(&mm->page_table_lock);
> -}
> -
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
From: Ira Weiny @ 2019-06-07 22:33 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Ralph Campbell, linux-rdma, John Hubbard,
Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
Jason Gunthorpe, amd-gfx
In-Reply-To: <20190606184438.31646-3-jgg@ziepe.ca>
On Thu, Jun 06, 2019 at 03:44:29PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> Ralph observes that hmm_range_register() can only be called by a driver
> while a mirror is registered. Make this clear in the API by passing in the
> mirror structure as a parameter.
>
> This also simplifies understanding the lifetime model for struct hmm, as
> the hmm pointer must be valid as part of a registered mirror so all we
> need in hmm_register_range() is a simple kref_get.
>
> Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> v2
> - Include the oneline patch to nouveau_svm.c
> ---
> drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
> include/linux/hmm.h | 7 ++++---
> mm/hmm.c | 15 ++++++---------
> 3 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 93ed43c413f0bb..8c92374afcf227 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
> range.values = nouveau_svm_pfn_values;
> range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
> again:
> - ret = hmm_vma_fault(&range, true);
> + ret = hmm_vma_fault(&svmm->mirror, &range, true);
> if (ret == 0) {
> mutex_lock(&svmm->mutex);
> if (!hmm_vma_range_done(&range)) {
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 688c5ca7068795..2d519797cb134a 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
> * Please see Documentation/vm/hmm.rst for how to use the range API.
> */
> int hmm_range_register(struct hmm_range *range,
> - struct mm_struct *mm,
> + struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift);
> @@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct hmm_range *range)
> }
>
> /* This is a temporary helper to avoid merge conflict between trees. */
> -static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> +static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> + struct hmm_range *range, bool block)
> {
> long ret;
>
> @@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> range->default_flags = 0;
> range->pfn_flags_mask = -1UL;
>
> - ret = hmm_range_register(range, range->vma->vm_mm,
> + ret = hmm_range_register(range, mirror,
> range->start, range->end,
> PAGE_SHIFT);
> if (ret)
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 547002f56a163d..8796447299023c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
> * Track updates to the CPU page table see include/linux/hmm.h
> */
> int hmm_range_register(struct hmm_range *range,
> - struct mm_struct *mm,
> + struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift)
> {
> unsigned long mask = ((1UL << page_shift) - 1UL);
> - struct hmm *hmm;
> + struct hmm *hmm = mirror->hmm;
>
> range->valid = false;
> range->hmm = NULL;
> @@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
> range->start = start;
> range->end = end;
>
> - hmm = hmm_get_or_create(mm);
> - if (!hmm)
> - return -EFAULT;
> -
> /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead) {
> - hmm_put(hmm);
> + if (hmm->mm == NULL || hmm->dead)
> return -EFAULT;
> - }
> +
> + range->hmm = hmm;
I don't think you need this assignment here. In the code below (right after
the mutext_lock()) it is set already. And looks like it remains that way after
the end of the series.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> + kref_get(&hmm->kref);
>
> /* Initialize range to track CPU page table updates. */
> mutex_lock(&hmm->lock);
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox