| Message ID | 1477928175-11391-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 31, 2016 at 03:36:15PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Verify that the userspace will get told if it changes to what the userptr > object backing store points to, *after* having created the GEM object. > > Two variants are tested: > 1. One where the object is used after it has been invalidated but while > the address space for the backing store is still invalid. > 2. Second one where the backing store becomes valid immediately after the > object has been invalidated. > > In both cases we want the kernel to give us -EFAULT on the buffer object in > question since the userspace is acting strangely so the driver does not want > to take part in corrupting memory. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/gem_userptr_blits.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > index f30e14360727..11a29ff5fdde 100644 > --- a/tests/gem_userptr_blits.c > +++ b/tests/gem_userptr_blits.c > @@ -1151,6 +1151,58 @@ static void test_unmap_cycles(int fd, int expected) > test_unmap(fd, expected); > } > > +/* > + * Verify that the userspace will get told if it changes to what the userptr > + * object backing store points to, *after* having created the GEM object. > + * > + * Two variants are tested: > + * 1. One where the object is used after it has been invalidated but while > + * the address space for the backing store is still invalid. > + * 2. Second one where the backing store becomes valid immediately after the > + * object has been invalidated. We still end up with inconsistent behaviour because we do a deferred gup, and only store ourselves in the mmu tree whilst we have pages. To close that we would have to add ourselves to the mmutree during creation (restructuring it to keep the lazy gup). Current holes would be between creation and execbuf, or swap-out and execbuf. What we currently check is that the location is valid at the time of use, not that the contents match both clients expectations. My concern with using the mmu invalidate notify was that it may kick a legitimate object. I will have to go and check to see if that is a valid concern. > + * In both cases we want the kernel to give us -EFAULT on the buffer object in > + * question since the userspace is acting strangely so the driver does not want > + * to take part in corrupting memory. > + */ > +static void test_remap(int fd, bool midaccess, int expected) > +{ > + char *ptr, *ptr2, *bo_ptr; > + uint32_t bo[2]; > + size_t map_size = sizeof(linear) + (PAGE_SIZE - 1); > + int ret; > + > + ptr = mmap(NULL, map_size, PROT_READ | PROT_WRITE, > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); > + igt_assert(ptr != MAP_FAILED); > + > + bo_ptr = (char *)ALIGN((unsigned long)ptr, PAGE_SIZE); > + gem_userptr(fd, bo_ptr, sizeof(linear), 0, userptr_flags, &bo[0]); > + > + bo[1] = create_bo(fd, 0); > + if (gup) > + copy(fd, bo[1], bo[0], 0); if (force_swap) mlock(all_memory); > + ret = munmap(ptr, map_size); > + igt_assert_eq(ret, 0); > + > + if (midaccess) > + copy(fd, bo[1], bo[0], expected); > + > + ptr2 = mmap(ptr, map_size, PROT_READ | PROT_WRITE, > + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); > + igt_assert(ptr2 != MAP_FAILED); > + igt_assert(ptr2 == ptr); > + > + copy(fd, bo[1], bo[0], expected); > + > + gem_close(fd, bo[1]); > + gem_close(fd, bo[0]); > + > + ret = munmap(ptr, map_size); > + igt_assert_eq(ret, 0); > +} Basically the current attitude is that we didn't care for the client games, our guarantee was that we didn't subvert the kernel. I do think your proposed semantic is safer, we might as well try and catch all the corner cases whilst here and just some pondering over if there are paths that call invalidate range but we don't want to lose the object. There are some fun tricks userspace could do with mmap(), such as sparse textures (mapping the object to the zero page, then populating on the fly), that don't fit well at all with our object-orientated approach and would fall afoul of the above scheme. Oh well, we need to treat this with all the care of an abi change and see if we can find more exotic users. -Chris
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c index f30e14360727..11a29ff5fdde 100644 --- a/tests/gem_userptr_blits.c +++ b/tests/gem_userptr_blits.c @@ -1151,6 +1151,58 @@ static void test_unmap_cycles(int fd, int expected) test_unmap(fd, expected); } +/* + * Verify that the userspace will get told if it changes to what the userptr + * object backing store points to, *after* having created the GEM object. + * + * Two variants are tested: + * 1. One where the object is used after it has been invalidated but while + * the address space for the backing store is still invalid. + * 2. Second one where the backing store becomes valid immediately after the + * object has been invalidated. + * + * In both cases we want the kernel to give us -EFAULT on the buffer object in + * question since the userspace is acting strangely so the driver does not want + * to take part in corrupting memory. + */ +static void test_remap(int fd, bool midaccess, int expected) +{ + char *ptr, *ptr2, *bo_ptr; + uint32_t bo[2]; + size_t map_size = sizeof(linear) + (PAGE_SIZE - 1); + int ret; + + ptr = mmap(NULL, map_size, PROT_READ | PROT_WRITE, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + igt_assert(ptr != MAP_FAILED); + + bo_ptr = (char *)ALIGN((unsigned long)ptr, PAGE_SIZE); + gem_userptr(fd, bo_ptr, sizeof(linear), 0, userptr_flags, &bo[0]); + + bo[1] = create_bo(fd, 0); + + copy(fd, bo[1], bo[0], 0); + + ret = munmap(ptr, map_size); + igt_assert_eq(ret, 0); + + if (midaccess) + copy(fd, bo[1], bo[0], expected); + + ptr2 = mmap(ptr, map_size, PROT_READ | PROT_WRITE, + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); + igt_assert(ptr2 != MAP_FAILED); + igt_assert(ptr2 == ptr); + + copy(fd, bo[1], bo[0], expected); + + gem_close(fd, bo[1]); + gem_close(fd, bo[0]); + + ret = munmap(ptr, map_size); + igt_assert_eq(ret, 0); +} + #define MM_STRESS_LOOPS 100000 struct stress_thread_data { @@ -1354,6 +1406,12 @@ int main(int argc, char **argv) igt_subtest("unsync-unmap-after-close") test_unmap_after_close(fd); + igt_subtest("unsync-remap") + test_remap(fd, false, 0); + + igt_subtest("unsync-remap-access") + test_remap(fd, true, 0); + igt_subtest("coherency-unsync") test_coherency(fd, count); @@ -1452,6 +1510,12 @@ int main(int argc, char **argv) igt_subtest("sync-unmap-after-close") test_unmap_after_close(fd); + igt_subtest("sync-remap") + test_remap(fd, false, EFAULT); + + igt_subtest("sync-remap-access") + test_remap(fd, true, EFAULT); + igt_subtest("stress-mm") test_stress_mm(fd);