Improve invalidation mechanism and harden security #60

pakmarkthub · 2019-06-05T21:30:23Z

Problems:

Issue GPU buffer invalidation does not tear-down CPU mappings obtained via gdr_map() #54: GPU buffer invalidation does not tear-down CPU mappings obtained via gdr_map()
Issue Support for fork #45: Support for fork
Issue fd to /dev/gdrdrv should not be sharable with other processes #62: fd to /dev/gdrdrv should not be sharable with other processes
Issue gdr_close does not unmap nor clean up internally alloced gdr_memh_t #65: gdr_close does not unmap nor clean up internally alloced gdr_memh_t
gdr_mh_t.h is generated by random number and is used as mmap offset. Sometimes, the mapping range may overlap another already-mapped range -- imagine the first gdr_mh_t.h is 0x0000 and the map size is 0x0100, then the second gdr_mh_t.h is 0x0020. Although it rarely happens, this bug can lead to undesirable and hard-to-detect behaviors.

This change:

fixes the bug that causes unmap_mapping_range to do nothing. We change the address passed to that API to vm_pgoff instead of vm_start.
prevents copy of vma when fork. Hence, the child processes cannot access the parent's mappings anymore. This behavior aligns with CUDA since CUDA contexts are not transferred to the child processes.
checks "struct pid *" to ensure that the caller is the same as the process that opens gdrdrv (calls to gdr_open). Hence, the use of opened gdrdrv fd is local to the creator process.
separates the mapping address space between each process. Two processes map to different GPU memory even with the same mmap offset.
makes the current gdr_mh_t.h sit next to the previous mapped range. It also guarantees that no two ranges will be overlapped due to the offsets.

Presubmit testings:

gc04:
- 4.15.0-50-generic GPU buffer invalidation does not tear-down CPU mappings obtained via gdr_map() #54-Ubuntu SMP Mon May 6 18:46:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
- P100 GPU
- Intel(R) Xeon(R) CPU E5-2687W v4 @ 3.00GHz

pakmarkthub · 2019-06-10T17:25:35Z

@Akshay-Venkatesh Can I ask you to review this PR as well? I would like to make sure that it does not break UCX.

pakmarkthub · 2019-06-13T00:10:51Z

@drossetti As discussed offline, unmap_mapping_range unmaps all mappings based on the specified file offset and range (see https://elixir.bootlin.com/linux/v4.15.18/source/mm/memory.c#L2811). If we have two mappings that have overlapped file-offset range, unmap_mapping_range will remove both of them.

An example undesirable situation:

Do cuMemAlloc(&d_A, ...) and cuMemAlloc(&d_B, ...).
Do gdr_pin_buffer for both d_A and d_B. The handle numbers returned from gdrdrv are very close. This situation rarely occurs but random_get_entropy() cannot guarantee that it will never occur.
Do gdr_map(..., &bar_ptr_A, ...) on d_A, and gdr_map(..., &bar_ptr_B, ...) on d_B.
Call cuMemFree(d_A).
This API triggers call back from nvidia_p2p_get_pages to destroy the bar_ptr_A mapping.
gdrdrv calls unmap_mapping_range with the offset based on handle_A.
unmap_mapping_range removes bar_ptr_B mapping as well because the offset based on handle_B is between handle_A and handle_A + size_A, which is a bug.

gdrdrv/gdrdrv.c

drossetti · 2019-06-13T21:10:00Z

gdrdrv/gdrdrv.c

    mutex_lock(&info->lock);
    list_add(&mr->node, &info->mr_list);
+
+    // The user treats handle as the offset value for mmap. To guarantee no


What about something like:
// the user-space library passes the memory handle as the mmap offset,
// and offsets are used to determine the VMAs to delete during invalidation.
// Hence, we need [handle,handle+size-1] to correspond to a unique VMA.
// Note that size here must match the original mmap size

drossetti · 2019-06-13T21:14:42Z

gdrdrv/gdrdrv.c

@@ -811,7 +856,7 @@ static int gdrdrv_mmap(struct file *filp, struct vm_area_struct *vma)
    // this also works as the mapped flag for this mr
    mr->cpu_mapping_type = GDR_MR_CACHING;
    vma->vm_ops = &gdrdrv_vm_ops;
-    gdr_dbg("overwriting vma->vm_private_data=%px with mr=%px\n", vma->vm_private_data, mr);
+    gdr_dbg("overwriting vma->vm_private_data=0x%p with mr=0x%p\n", vma->vm_private_data, mr);


note that %px is not the same as 0x%p, check the kernel doc

drossetti · 2019-06-13T21:16:01Z

test_invalidation.cpp

@@ -0,0 +1,1138 @@
+/*
+ * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.


drossetti · 2019-06-13T21:23:48Z

test_invalidation.cpp

+        read_fd = filedes_1[0];
+        write_fd = filedes_0[1];
+
+        srand(rand());


Why is srand() needed?
Also, I'm not sure srand(rand()) makes sense.

test_invalidation.cpp

gdrdrv/gdrdrv.c

pakmarkthub · 2019-06-14T18:45:52Z

@drossetti Please review

drossetti · 2019-06-14T19:12:09Z

gdrdrv/gdrdrv.c

+    // The end of this range is wrapped around. The calculation uses PAGE_SHIFT
+    // because handle is equal vm_pgoff (aka file offset << PAGE_SHIFT). We
+    // indicate this case as -ENOMEM error.
+    if ((next_handle & ((gdr_hnd_t)(-1) >> PAGE_SHIFT)) < info->next_handle)


I don't like failing eagerly. It should fail when next handle is going to be used.

gdrdrv/gdrdrv.c

pakmarkthub · 2019-06-14T19:45:13Z

@drossetti Thank you. I have fixed the bug.

drossetti · 2019-06-17T17:15:10Z

Makefile

@@ -29,7 +29,7 @@ endif

 LIBOBJS := $(LIBSRCS:.c=.o)

-SRCS := basic.cpp validate.cpp copybw.cpp
+SRCS := basic.cpp validate.cpp copybw.cpp test_invalidation.cpp


Can we use this new test to have all sanity tests? in that case, can we rename it as sanity.cpp ?
Also drop test_ prefix as it does not match the other test names.
We can add a proper prefix during the make install phase.

drossetti · 2019-06-17T17:19:57Z

Makefile

@@ -86,6 +87,9 @@ validate: validate.o $(LIB)
 copybw: copybw.o $(LIB)
 	$(LINK.cc)  -o $@ $^ $(LIBS)

+test_invalidation: test_invalidation.o $(LIB)
+	$(LINK.cc)  -o $@ $^ -lcheck -lcudart -lcuda -lpthread -lrt -lsubunit


Are you really using subunit?
In case https://libcheck.github.io/check/doc/check_html/check_4.html#Subunit-Support shows you'd need to use:

srunner_run_all (sr, CK_SUBUNIT);

At least on Ubuntu 18.04 with libcheck installed from "apt install check", I cannot compile without "-lsubunit". This seems to be a dependency from libcheck.a itself. The error is as below:

g++ -O2 -I /usr/local/cuda/include -I gdrdrv/ -I /usr/local/cuda/include -D GDRAPI_ARCH=X86 -L /usr/local/cuda/lib64 -L /usr/local/cuda/lib -L /usr/lib64/nvidia -L /usr/lib/nvidia -L /usr/local/cuda/lib64 -o sanity sanity.o libgdrapi.so.1.2 -lcheck -lcudart -lcuda -lpthread -lrt /usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/libcheck.a(check_log.o): In function `subunit_lfun': (.text+0x5f4): undefined reference to `subunit_test_start' (.text+0x6bf): undefined reference to `subunit_test_fail' (.text+0x6d4): undefined reference to `subunit_test_pass' (.text+0x6ef): undefined reference to `subunit_test_error' collect2: error: ld returned 1 exit status

shouldn't we use pkg-config to get the right libraries ?

gdrdrv/gdrdrv.c

drossetti · 2019-06-17T17:22:27Z

gdrdrv/gdrdrv.c

+    // because filp->f_mapping usually points to inode->i_mapping.
+    struct address_space    mapping;
+
+    // The handle number of mmap's offset are equivalent. However, the mmap


this sentence sounds weird. maybe you meant "and" instead of "of"

drossetti · 2019-06-17T17:26:46Z

gdrdrv/gdrdrv.c

@@ -273,6 +303,19 @@ static int gdrdrv_open(struct inode *inode, struct file *filp)
    INIT_LIST_HEAD(&info->mr_list);
    mutex_init(&info->lock);

+    info->pid = task_pid(current);
+
+    // Create mapping per opened file. By preventing sharing of this file, the


can you move this comment one line above.
also drop "create mapping per opened file" and reword similar to:
"GPU driver does not support sharing GPU allocations at fork time. Hence here we track the process owning the driver fd and prevent other process to use it."
Here we store the current pid to prevent

gdrdrv/gdrdrv.c

drossetti · 2019-06-18T20:49:02Z

@pakmarkthub
is the comment in gdrapi.h copied below still true ?

// Note that altough BAR mappings of GPU memory are destroyed, user-space                                                                                                                                                                                                       
// mappings are not. So therefore user code is responsible of calling                                                                                                                                                                                                           
// gdr_unmap on all mappings before calling gdr_close.                                                                                                                                                                                                                          
int gdr_close(gdr_t g);

pakmarkthub · 2019-06-19T00:01:02Z

@drossetti Thanks Davide. This revision is ready for review.

pakmarkthub · 2019-06-19T00:07:55Z

@e-ago Could you also take a look? I want to make sure that this PR does not break what you are working on.

pakmarkthub · 2019-07-13T00:35:05Z

Please help reviewing this PR. It is one of the blockers for the automated test.

drossetti · 2019-07-15T16:04:06Z

Makefile

@@ -86,6 +87,9 @@ validate: validate.o $(LIB)
 copybw: copybw.o $(LIB)
 	$(LINK.cc)  -o $@ $^ $(LIBS)

+test_invalidation: test_invalidation.o $(LIB)
+	$(LINK.cc)  -o $@ $^ -lcheck -lcudart -lcuda -lpthread -lrt -lsubunit


shouldn't we use pkg-config to get the right libraries ?

drossetti · 2019-07-15T16:04:56Z

Makefile

@@ -96,7 +92,7 @@ drv_install:


 clean:
-	rm -f *.o $(EXES) lib*.{a,so}* *~ core.* && \
+	rm -f *.o $(EXES) lib*.{a,so}* *~ core.* gdrdrv/.cache.mk libgdrapi.so* && \


should we defer to the gdrdrv/ Makefile for cleanup there?

drossetti · 2019-07-15T16:47:08Z

gdrdrv/gdrdrv.c

+    // numerical pid here to avoid issues from pid reuse.
+    struct pid             *pid;
+
+    // Address space uniqued to this opened file. We need to create a new one


'uniqued' should be 'unique'

gdrdrv/gdrdrv.c

drossetti · 2019-07-15T16:54:48Z

gdrdrv/gdrdrv.c

+
+    // Disallow mmapped VMA to propagate to child processes
+    vma->vm_flags |= VM_DONTCOPY;
+
 #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,6,9)


I suggest to simply drop support of kernel versions earlier than 2.6.9 here, mainly because the GPU driver has a more stringent requirement.

drossetti · 2019-07-15T17:00:12Z

gdrdrv/gdrdrv.c

+            mr->vma = NULL;
+            mr->mapping = NULL;
+            mr->cpu_mapping_type = GDR_MR_NONE;
+            // TODO: tear down stale partial mappings


This comment might have never been true, i.e. in case of error, the vma is destroyed and all partial mappings are automatically removed.
Still we should verify that.

Confirmed that if gdrdrv_mmap returns an error, Linux kernel will clean up any partial mappings for us. See https://elixir.bootlin.com/linux/latest/source/mm/mmap.c#L1788 for reference.

drossetti · 2019-07-15T17:19:20Z

sanity.cpp

+    return fd;
+}
+
+typedef struct gdr_memh_t { 


rather than duplicating these type definitions here, I'd rather have a shared header file.

drossetti · 2019-07-15T17:20:15Z

sanity.cpp

+
+    gdr_mh_t mh = {0};
+    if (mh == null_mh) 
+		print_dbg("miao\n");


this looks like stale code

drossetti · 2019-07-15T17:20:29Z

sanity.cpp

+    gdr_mh_t mh = {0};
+    if (mh == null_mh) 
+		print_dbg("miao\n");
+	CUdeviceptr d_ptr = d_A;


indentation problem

sanity.cpp

pakmarkthub · 2019-07-15T21:03:34Z

@drossetti Thank you for the comments. I have fixed the issues. Please re-review the PR.

drossetti · 2019-07-16T12:04:48Z

gdrapi_internal.h

@@ -0,0 +1,12 @@
+typedef struct gdr_memh_t { 


3 not strictly necessary but missing things:

license header

protection from multiple inclusions

extern "C"

drossetti · 2019-07-16T12:13:07Z

The sanity test is a fairly large piece of code. Would it be possible to move that to a separate PR?
I would rename the current validation test in this PR.

Or at least fix the PR cover letter which still refers to "test_validation.cpp"

… parent process by way of fork

…ernel versions

…gned handle is starting to wrap around

…appings are destroyed by Linux kernel immediately after our mmap function returns

pakmarkthub · 2019-07-18T00:50:02Z

@drossetti As requested, I separated out the sanity test suite to PR #71. This PR now focuses on the invalidation issues only. Please re-review.

drossetti

LGTM
just a couple of nits, no need to rereview

drossetti · 2019-07-18T12:52:51Z

gdrdrv/gdrdrv.c

    mutex_unlock(&info->lock);

+    params.handle = mr->handle;


purely for style, I'd rather access mr->handle when it has a sane value only, i.e. when there are no errors so after list_add().

drossetti · 2019-07-18T12:56:08Z

gdrdrv/gdrdrv.c

-    }
-#else
+
+    // Disallow mmapped VMA to propagate to child processes


'child' -> 'children'

…dle = mr->handle in gdrdrv/gdrdrv.c

drossetti · 2019-07-30T11:28:37Z

merged

pakmarkthub requested review from drossetti and Akshay-Venkatesh June 5, 2019 21:30

drossetti requested changes Jun 13, 2019

View reviewed changes

pakmarkthub requested a review from drossetti June 14, 2019 18:45

drossetti requested changes Jun 14, 2019

View reviewed changes

pakmarkthub requested a review from drossetti June 14, 2019 19:44

drossetti requested changes Jun 17, 2019

View reviewed changes

pakmarkthub requested review from e-ago and drossetti June 19, 2019 00:06

pakmarkthub requested a review from spotluri July 13, 2019 00:30

drossetti requested changes Jul 15, 2019

View reviewed changes

pakmarkthub requested a review from drossetti July 15, 2019 21:02

drossetti requested changes Jul 16, 2019

View reviewed changes

pakmarkthub added 6 commits July 17, 2019 16:34

Fixed the bug that causes unmap_mapping_range to not perform unmapping

a6b0d3e

Fixed the bug allowing the child process the shared gdr_map with the…

d394e2a

… parent process by way of fork

Prevented sharing gdrcopy fd between processes

e82a4c5

Separated mapping address space of Process A from Process B

835cc12

Added comments to gdrdrv.c

490a109

Changed %p to %px in gdrdrv

094ea6b

pakmarkthub added 12 commits July 17, 2019 16:51

Added pid checking to gdrdrv_release

2d95c64

Created gdr_generate_mr_handle and fixed the wrap-around bug

e452d9c

Handled the mismatch of the address_space struct in different Linux k…

df02139

…ernel versions

Removed unused #include <linux/sched/mm.h> from gdrdrv/gdrdrv.c

34daeb2

Fixed the bug that makes gdrdrv_pin_buffer eagerly fail when the assi…

d1a266c

…gned handle is starting to wrap around

Fixed the bug that adds mr to info->mr_list when creating the mr fail

58575d5

Fixed comments and checked for info->lock in gdrdrv.c

a25c4fe

gdr_close() now automatically cleans up resources

b93ad9c

Removed deprecated gds_close comment

11079f5

Dropped support for Linux kernel version <= 2.6.9, and fixed a typo

00b947e

Fixed the indentation issue in gdrapi.h

9124188

Removed "// TODO: tear down stale partial mappings" because partial m…

d91d76b

…appings are destroyed by Linux kernel immediately after our mmap function returns

pakmarkthub force-pushed the invalidate branch from 4087948 to d91d76b Compare July 18, 2019 00:07

pakmarkthub mentioned this pull request Jul 18, 2019

Sanity test suite #71

Closed

pakmarkthub requested a review from drossetti July 18, 2019 00:49

drossetti approved these changes Jul 18, 2019

View reviewed changes

Changed child -> children and checked ret before assigning params.han…

3786215

…dle = mr->handle in gdrdrv/gdrdrv.c

drossetti merged commit 8a6985b into NVIDIA:invalidate Jul 30, 2019

		@@ -0,0 +1,1138 @@
		/*
		* Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.

Improve invalidation mechanism and harden security #60

Improve invalidation mechanism and harden security #60

Conversation

pakmarkthub commented Jun 5, 2019 • edited Loading

pakmarkthub commented Jun 10, 2019

pakmarkthub commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drossetti Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

pakmarkthub commented Jun 14, 2019

Choose a reason for hiding this comment

pakmarkthub commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drossetti commented Jun 18, 2019

pakmarkthub commented Jun 19, 2019

pakmarkthub commented Jun 19, 2019

pakmarkthub commented Jul 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakmarkthub commented Jul 15, 2019

drossetti Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

drossetti commented Jul 16, 2019 • edited Loading

pakmarkthub commented Jul 18, 2019

drossetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drossetti commented Jul 30, 2019

pakmarkthub commented Jun 5, 2019 •

edited

Loading

drossetti Jun 13, 2019 •

edited

Loading

drossetti Jul 16, 2019 •

edited

Loading

drossetti commented Jul 16, 2019 •

edited

Loading