| Message ID | 1435575591-3510-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/29/2015 11:59 AM, Micha? Winiarski wrote: > When the the memory backing the userptr object is freed by the user, it's > possible to trigger recursive deadlock caused by operations done on > different BO mapped in that region, triggering invalidate. > > Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > --- > tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > index 1f2cc96..3fe8f90 100644 > --- a/tests/gem_userptr_blits.c > +++ b/tests/gem_userptr_blits.c > @@ -640,6 +640,80 @@ static void test_forked_access(int fd) > free(ptr2); > } > > +static int test_map_fixed_invalidate(int fd, bool overlap) > +{ > + void *ptr; > + void *map; > + int i; > + int num_handles = overlap ? 2 : 1; > + uint32_t handle[num_handles]; > + uint32_t mmap_handle; > + struct drm_i915_gem_mmap_gtt mmap_arg; > + > + igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); > + for (i=0; i<num_handles; i++) > + igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); > + free(ptr); I am not sure we can rely on free triggering munmap(2) here, I think this is just glibc implementation detail. So I would suggest allocating with mmap and freeing with munmap. > + > + mmap_handle = gem_create(fd, PAGE_SIZE); > + > + memset(&mmap_arg, 0, sizeof(mmap_arg)); > + mmap_arg.handle = mmap_handle; > + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg); > + map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, > + fd, mmap_arg.offset); > + igt_assert(map != MAP_FAILED); > + > + *(uint32_t*)map = 0xdead; > + gem_set_tiling(fd, mmap_handle, 2, 512 * 4); > + munmap(map, PAGE_SIZE); > + > + for (i=0; i<num_handles; i++) > + gem_close(fd, handle[i]); > + > + return 0; > +} > + > +static int test_map_fixed_partial_overlap(int fd) > +{ > + void *ptr; > + void *map; > + uint32_t handle; > + uint32_t mmap_handle; > + struct drm_i915_gem_mmap_gtt mmap_arg; > + struct drm_i915_gem_set_domain set_domain; > + > + igt_assert(posix_memalign(&ptr, PAGE_SIZE, sizeof(linear)) == 0); > + handle = create_userptr(fd, 0, ptr); > + copy(fd, handle, handle, 0); > + > + mmap_handle = gem_create(fd, PAGE_SIZE); > + > + memset(&mmap_arg, 0, sizeof(mmap_arg)); > + mmap_arg.handle = mmap_handle; > + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg); > + map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, > + fd, mmap_arg.offset); > + igt_assert(map != MAP_FAILED); > + > + *(uint32_t*)map = 0xdead; > + > + memset(&set_domain, 0, sizeof(set_domain)); > + set_domain.handle = handle; > + set_domain.read_domains = I915_GEM_DOMAIN_GTT; > + set_domain.write_domain = I915_GEM_DOMAIN_GTT; > + igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) && > + errno == EFAULT); > + > + free(ptr); > + munmap(map, PAGE_SIZE); > + > + gem_close(fd, handle); > + gem_close(fd, mmap_handle); > + > + return 0; > +} > + > static int test_forbidden_ops(int fd) > { > struct drm_i915_gem_pread gem_pread; > @@ -1489,6 +1563,15 @@ int main(int argc, char **argv) > igt_subtest("stress-mm-invalidate-close-overlap") > test_invalidate_close_race(fd, true); > > + igt_subtest("map-fixed-invalidate") > + test_map_fixed_invalidate(fd, false); > + > + igt_subtest("map-fixed-invalidate-overlap") > + test_map_fixed_invalidate(fd, true); > + > + igt_subtest("map-fixed-partial-overlap") > + test_map_fixed_partial_overlap(fd); > + It is a bit confusing how overlap means two different things between subtests. In one it is two userptr objects that are overlapping and in another it is the mmap of a normal GEM bo overlapping the userptr range. I mean, in all subtests mmap always overlap the userptr. Also, should there be a subtest which mmaps the userptr bo itself with MAP_FIXED, to the same or overlapping range? Regards, Tvrtko
On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote: > > On 06/29/2015 11:59 AM, Micha? Winiarski wrote: > >When the the memory backing the userptr object is freed by the user, it's > >possible to trigger recursive deadlock caused by operations done on > >different BO mapped in that region, triggering invalidate. > > > >Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > >--- > > tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > > >diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > >index 1f2cc96..3fe8f90 100644 > >--- a/tests/gem_userptr_blits.c > >+++ b/tests/gem_userptr_blits.c > >@@ -640,6 +640,80 @@ static void test_forked_access(int fd) > > free(ptr2); > > } > > > >+static int test_map_fixed_invalidate(int fd, bool overlap) > >+{ > >+ void *ptr; > >+ void *map; > >+ int i; > >+ int num_handles = overlap ? 2 : 1; > >+ uint32_t handle[num_handles]; > >+ uint32_t mmap_handle; > >+ struct drm_i915_gem_mmap_gtt mmap_arg; > >+ > >+ igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); > >+ for (i=0; i<num_handles; i++) > >+ igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); > >+ free(ptr); > > I am not sure we can rely on free triggering munmap(2) here, I think > this is just glibc implementation detail. So I would suggest > allocating with mmap and freeing with munmap. The MAP_FIXED itself should be sufficient to invalidate any prior vma at that address, so we don't depend upon the free() here to zap userptr->pages. In this instance userptr->pages doesn't get populated before the MAP_FIXED anyway. -Chris
On 06/29/2015 03:07 PM, Chris Wilson wrote: > On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote: >> >> On 06/29/2015 11:59 AM, Micha? Winiarski wrote: >>> When the the memory backing the userptr object is freed by the user, it's >>> possible to trigger recursive deadlock caused by operations done on >>> different BO mapped in that region, triggering invalidate. >>> >>> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> >>> --- >>> tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 83 insertions(+) >>> >>> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c >>> index 1f2cc96..3fe8f90 100644 >>> --- a/tests/gem_userptr_blits.c >>> +++ b/tests/gem_userptr_blits.c >>> @@ -640,6 +640,80 @@ static void test_forked_access(int fd) >>> free(ptr2); >>> } >>> >>> +static int test_map_fixed_invalidate(int fd, bool overlap) >>> +{ >>> + void *ptr; >>> + void *map; >>> + int i; >>> + int num_handles = overlap ? 2 : 1; >>> + uint32_t handle[num_handles]; >>> + uint32_t mmap_handle; >>> + struct drm_i915_gem_mmap_gtt mmap_arg; >>> + >>> + igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); >>> + for (i=0; i<num_handles; i++) >>> + igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); >>> + free(ptr); >> >> I am not sure we can rely on free triggering munmap(2) here, I think >> this is just glibc implementation detail. So I would suggest >> allocating with mmap and freeing with munmap. > > The MAP_FIXED itself should be sufficient to invalidate any prior vma at > that address, so we don't depend upon the free() here to zap Yeah, but does the free trigger an invalidate, or does it not? Or in other words, is the one from MAP_FIXED the first one, or not? Better to be explicit than undefined in the test case, that was my point. > userptr->pages. In this instance userptr->pages doesn't get populated > before the MAP_FIXED anyway. Hmmm... did not even think about that. Why does MAP_FIXED trigger get_pages? Tvrtko
On Mon, Jun 29, 2015 at 03:15:12PM +0100, Tvrtko Ursulin wrote: > > On 06/29/2015 03:07 PM, Chris Wilson wrote: > >On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote: > >> > >>On 06/29/2015 11:59 AM, Micha? Winiarski wrote: > >>>When the the memory backing the userptr object is freed by the user, it's > >>>possible to trigger recursive deadlock caused by operations done on > >>>different BO mapped in that region, triggering invalidate. > >>> > >>>Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > >>>--- > >>> tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 83 insertions(+) > >>> > >>>diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > >>>index 1f2cc96..3fe8f90 100644 > >>>--- a/tests/gem_userptr_blits.c > >>>+++ b/tests/gem_userptr_blits.c > >>>@@ -640,6 +640,80 @@ static void test_forked_access(int fd) > >>> free(ptr2); > >>> } > >>> > >>>+static int test_map_fixed_invalidate(int fd, bool overlap) > >>>+{ > >>>+ void *ptr; > >>>+ void *map; > >>>+ int i; > >>>+ int num_handles = overlap ? 2 : 1; > >>>+ uint32_t handle[num_handles]; > >>>+ uint32_t mmap_handle; > >>>+ struct drm_i915_gem_mmap_gtt mmap_arg; > >>>+ > >>>+ igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); > >>>+ for (i=0; i<num_handles; i++) > >>>+ igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); > >>>+ free(ptr); > >> > >>I am not sure we can rely on free triggering munmap(2) here, I think > >>this is just glibc implementation detail. So I would suggest > >>allocating with mmap and freeing with munmap. > > > >The MAP_FIXED itself should be sufficient to invalidate any prior vma at > >that address, so we don't depend upon the free() here to zap > > Yeah, but does the free trigger an invalidate, or does it not? Or in > other words, is the one from MAP_FIXED the first one, or not? Better > to be explicit than undefined in the test case, that was my point. Yes. But I would prefer MAP_FIXED to be the first invalidate, but that looks like we then need to leak the ptr. > >userptr->pages. In this instance userptr->pages doesn't get populated > >before the MAP_FIXED anyway. > > Hmmm... did not even think about that. Why does MAP_FIXED trigger get_pages? MAP_FIXED doesn't. Part of the tests is to make sure that MAP_FIXED places a GTT object over top of the userptr is that the userptr then reports EFAULT when used. My worrying about obj->pages prior to the invalidation is that there are few paths through cancel_userptr() that depend upon whether the object has been used (i.e. has pages) or whether the object is active on the GPU. That's the purpose of doing a copy or set-domain or nothing prior to the MAP_FIXED. -Chris
On 06/29/2015 03:25 PM, Chris Wilson wrote: > On Mon, Jun 29, 2015 at 03:15:12PM +0100, Tvrtko Ursulin wrote: >> >> On 06/29/2015 03:07 PM, Chris Wilson wrote: >>> On Mon, Jun 29, 2015 at 03:01:12PM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 06/29/2015 11:59 AM, Micha? Winiarski wrote: >>>>> When the the memory backing the userptr object is freed by the user, it's >>>>> possible to trigger recursive deadlock caused by operations done on >>>>> different BO mapped in that region, triggering invalidate. >>>>> >>>>> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> >>>>> --- >>>>> tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 83 insertions(+) >>>>> >>>>> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c >>>>> index 1f2cc96..3fe8f90 100644 >>>>> --- a/tests/gem_userptr_blits.c >>>>> +++ b/tests/gem_userptr_blits.c >>>>> @@ -640,6 +640,80 @@ static void test_forked_access(int fd) >>>>> free(ptr2); >>>>> } >>>>> >>>>> +static int test_map_fixed_invalidate(int fd, bool overlap) >>>>> +{ >>>>> + void *ptr; >>>>> + void *map; >>>>> + int i; >>>>> + int num_handles = overlap ? 2 : 1; >>>>> + uint32_t handle[num_handles]; >>>>> + uint32_t mmap_handle; >>>>> + struct drm_i915_gem_mmap_gtt mmap_arg; >>>>> + >>>>> + igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); >>>>> + for (i=0; i<num_handles; i++) >>>>> + igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); >>>>> + free(ptr); >>>> >>>> I am not sure we can rely on free triggering munmap(2) here, I think >>>> this is just glibc implementation detail. So I would suggest >>>> allocating with mmap and freeing with munmap. >>> >>> The MAP_FIXED itself should be sufficient to invalidate any prior vma at >>> that address, so we don't depend upon the free() here to zap >> >> Yeah, but does the free trigger an invalidate, or does it not? Or in >> other words, is the one from MAP_FIXED the first one, or not? Better >> to be explicit than undefined in the test case, that was my point. > > Yes. But I would prefer MAP_FIXED to be the first invalidate, but that > looks like we then need to leak the ptr. If it used mmap instead of posix_memalign and no free then what would it leak? MAP_FIXED would be guaranteed first invalidate and it would discard the mapping. Tvrtko
On Mon, Jun 29, 2015 at 03:56:07PM +0100, Tvrtko Ursulin wrote: > >Yes. But I would prefer MAP_FIXED to be the first invalidate, but that > >looks like we then need to leak the ptr. > > If it used mmap instead of posix_memalign and no free then what > would it leak? MAP_FIXED would be guaranteed first invalidate and it > would discard the mapping. Nothing, good idea. -Chris
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c index 1f2cc96..3fe8f90 100644 --- a/tests/gem_userptr_blits.c +++ b/tests/gem_userptr_blits.c @@ -640,6 +640,80 @@ static void test_forked_access(int fd) free(ptr2); } +static int test_map_fixed_invalidate(int fd, bool overlap) +{ + void *ptr; + void *map; + int i; + int num_handles = overlap ? 2 : 1; + uint32_t handle[num_handles]; + uint32_t mmap_handle; + struct drm_i915_gem_mmap_gtt mmap_arg; + + igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); + for (i=0; i<num_handles; i++) + igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0); + free(ptr); + + mmap_handle = gem_create(fd, PAGE_SIZE); + + memset(&mmap_arg, 0, sizeof(mmap_arg)); + mmap_arg.handle = mmap_handle; + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg); + map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, + fd, mmap_arg.offset); + igt_assert(map != MAP_FAILED); + + *(uint32_t*)map = 0xdead; + gem_set_tiling(fd, mmap_handle, 2, 512 * 4); + munmap(map, PAGE_SIZE); + + for (i=0; i<num_handles; i++) + gem_close(fd, handle[i]); + + return 0; +} + +static int test_map_fixed_partial_overlap(int fd) +{ + void *ptr; + void *map; + uint32_t handle; + uint32_t mmap_handle; + struct drm_i915_gem_mmap_gtt mmap_arg; + struct drm_i915_gem_set_domain set_domain; + + igt_assert(posix_memalign(&ptr, PAGE_SIZE, sizeof(linear)) == 0); + handle = create_userptr(fd, 0, ptr); + copy(fd, handle, handle, 0); + + mmap_handle = gem_create(fd, PAGE_SIZE); + + memset(&mmap_arg, 0, sizeof(mmap_arg)); + mmap_arg.handle = mmap_handle; + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg); + map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, + fd, mmap_arg.offset); + igt_assert(map != MAP_FAILED); + + *(uint32_t*)map = 0xdead; + + memset(&set_domain, 0, sizeof(set_domain)); + set_domain.handle = handle; + set_domain.read_domains = I915_GEM_DOMAIN_GTT; + set_domain.write_domain = I915_GEM_DOMAIN_GTT; + igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) && + errno == EFAULT); + + free(ptr); + munmap(map, PAGE_SIZE); + + gem_close(fd, handle); + gem_close(fd, mmap_handle); + + return 0; +} + static int test_forbidden_ops(int fd) { struct drm_i915_gem_pread gem_pread; @@ -1489,6 +1563,15 @@ int main(int argc, char **argv) igt_subtest("stress-mm-invalidate-close-overlap") test_invalidate_close_race(fd, true); + igt_subtest("map-fixed-invalidate") + test_map_fixed_invalidate(fd, false); + + igt_subtest("map-fixed-invalidate-overlap") + test_map_fixed_invalidate(fd, true); + + igt_subtest("map-fixed-partial-overlap") + test_map_fixed_partial_overlap(fd); + igt_subtest("coherency-sync") test_coherency(fd, count);
When the the memory backing the userptr object is freed by the user, it's possible to trigger recursive deadlock caused by operations done on different BO mapped in that region, triggering invalidate. Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> --- tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)