| Message ID | 1422974357-25100-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 03, 2015 at 03:39:17PM +0100, Micha? Winiarski wrote: > It was possible for invalidate range start mmu notifier callback to race > with releasing userptr object. If the object is released prior to > taking a spinlock in the callback, we'll encounter a null pointer > dereference. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > --- > tests/gem_userptr_blits.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 2 deletions(-) > > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > index be2fdf9..5864e4f 100644 > --- a/tests/gem_userptr_blits.c > +++ b/tests/gem_userptr_blits.c > @@ -1179,6 +1179,8 @@ static void test_unmap_cycles(int fd, int expected) > test_unmap(fd, expected); > } > > +#define MM_STRESS_LOOPS 100000 > + > struct stress_thread_data { > unsigned int stop; > int exit_code; > @@ -1211,7 +1213,7 @@ static void test_stress_mm(int fd) > { > int ret; > pthread_t t; > - unsigned int loops = 100000; > + unsigned int loops = MM_STRESS_LOOPS; > uint32_t handle; > void *ptr; > struct stress_thread_data stdata; > @@ -1239,6 +1241,62 @@ static void test_stress_mm(int fd) > igt_assert(stdata.exit_code == 0); > } > > +struct userptr_close_thread_data { > + int fd; > + void *ptr; > + bool overlap; > + bool stop; > +}; > + > +static void *mm_userptr_close_thread(void *data) > +{ > + int ret; > + struct userptr_close_thread_data *t_data = (struct userptr_close_thread_data *)data; > + int fd = t_data->fd; > + void *ptr = t_data->ptr; > + int handle_num = t_data->overlap ? 2 : 1; > + > + uint32_t handle[handle_num]; > + > + while (!t_data->stop) { > + for (int i = 0; i < handle_num; i++) > + ret = gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]); > + igt_assert(ret == 0); Whoops. Let's just assert that igt_assert() can't be compiled out (that would make a mockery of igt for starters) and allow us to use expressions with side effects inside igt_assert(). static void userptr_close_thread(void *data) { strct userptr_close_thread *t = data; const int nhandles = t->overlap ? 2 : 1; uint32_t handle[nhandles]; /* Be pedantic and enforce the required memory barriers */ pthread_mutex_lock(&t->mutex); while (!t->stop) { pthread_mutex_unlock(&t->mutex); for (int i = 0; i < nhandles; i++) igt_assert(gem_userptr(t->fd, t->ptr, PAGE_SIZE, 0, &handle[i]) == 0); for (int i = 0; i < nhandles; i++) gem_close(t->fd, handle[i]); pthread_mutex_lock(&t->mutex); } pthread_mutex_unlock(&t->mutex); return NULL; } Nice test! -Chris
On Tue, Feb 03, 2015 at 03:01:38PM +0000, Chris Wilson wrote: > On Tue, Feb 03, 2015 at 03:39:17PM +0100, Micha? Winiarski wrote: > > It was possible for invalidate range start mmu notifier callback to race > > with releasing userptr object. If the object is released prior to > > taking a spinlock in the callback, we'll encounter a null pointer > > dereference. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > > --- > > tests/gem_userptr_blits.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 66 insertions(+), 2 deletions(-) > > > > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > > index be2fdf9..5864e4f 100644 > > --- a/tests/gem_userptr_blits.c > > +++ b/tests/gem_userptr_blits.c > > @@ -1179,6 +1179,8 @@ static void test_unmap_cycles(int fd, int expected) > > test_unmap(fd, expected); > > } > > > > +#define MM_STRESS_LOOPS 100000 > > + > > struct stress_thread_data { > > unsigned int stop; > > int exit_code; > > @@ -1211,7 +1213,7 @@ static void test_stress_mm(int fd) > > { > > int ret; > > pthread_t t; > > - unsigned int loops = 100000; > > + unsigned int loops = MM_STRESS_LOOPS; > > uint32_t handle; > > void *ptr; > > struct stress_thread_data stdata; > > @@ -1239,6 +1241,62 @@ static void test_stress_mm(int fd) > > igt_assert(stdata.exit_code == 0); > > } > > > > +struct userptr_close_thread_data { > > + int fd; > > + void *ptr; > > + bool overlap; > > + bool stop; > > +}; > > + > > +static void *mm_userptr_close_thread(void *data) > > +{ > > + int ret; > > + struct userptr_close_thread_data *t_data = (struct userptr_close_thread_data *)data; > > + int fd = t_data->fd; > > + void *ptr = t_data->ptr; > > + int handle_num = t_data->overlap ? 2 : 1; > > + > > + uint32_t handle[handle_num]; > > + > > + while (!t_data->stop) { > > + for (int i = 0; i < handle_num; i++) > > + ret = gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]); > > + igt_assert(ret == 0); > > Whoops. Let's just assert that igt_assert() can't be compiled out (that > would make a mockery of igt for starters) and allow us to use > expressions with side effects inside igt_assert(). Yeah, igt_assert/require can't be compiled out, and for simpler control flow we use the pattern Chris suggested a lot. -Daniel > > static void userptr_close_thread(void *data) > { > strct userptr_close_thread *t = data; > const int nhandles = t->overlap ? 2 : 1; > uint32_t handle[nhandles]; > > /* Be pedantic and enforce the required memory barriers */ > pthread_mutex_lock(&t->mutex); > while (!t->stop) { > pthread_mutex_unlock(&t->mutex); > > for (int i = 0; i < nhandles; i++) > igt_assert(gem_userptr(t->fd, t->ptr, PAGE_SIZE, 0, &handle[i]) == 0); > for (int i = 0; i < nhandles; i++) > gem_close(t->fd, handle[i]); > > pthread_mutex_lock(&t->mutex); > } > pthread_mutex_unlock(&t->mutex); > > return NULL; > } > > Nice test! > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c index be2fdf9..5864e4f 100644 --- a/tests/gem_userptr_blits.c +++ b/tests/gem_userptr_blits.c @@ -1179,6 +1179,8 @@ static void test_unmap_cycles(int fd, int expected) test_unmap(fd, expected); } +#define MM_STRESS_LOOPS 100000 + struct stress_thread_data { unsigned int stop; int exit_code; @@ -1211,7 +1213,7 @@ static void test_stress_mm(int fd) { int ret; pthread_t t; - unsigned int loops = 100000; + unsigned int loops = MM_STRESS_LOOPS; uint32_t handle; void *ptr; struct stress_thread_data stdata; @@ -1239,6 +1241,62 @@ static void test_stress_mm(int fd) igt_assert(stdata.exit_code == 0); } +struct userptr_close_thread_data { + int fd; + void *ptr; + bool overlap; + bool stop; +}; + +static void *mm_userptr_close_thread(void *data) +{ + int ret; + struct userptr_close_thread_data *t_data = (struct userptr_close_thread_data *)data; + int fd = t_data->fd; + void *ptr = t_data->ptr; + int handle_num = t_data->overlap ? 2 : 1; + + uint32_t handle[handle_num]; + + while (!t_data->stop) { + for (int i = 0; i < handle_num; i++) + ret = gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]); + igt_assert(ret == 0); + for (int i = 0; i < handle_num; i++) { + gem_close(fd, handle[i]); + } + } + + return NULL; +} + +static void test_invalidate_close_race(int fd, bool overlap) +{ + int ret; + pthread_t t; + unsigned int loops = MM_STRESS_LOOPS; + struct userptr_close_thread_data t_data; + + memset(&t_data, 0, sizeof(t_data)); + t_data.fd = fd; + t_data.overlap = overlap; + igt_assert(posix_memalign(&t_data.ptr, PAGE_SIZE, PAGE_SIZE) == 0); + + ret = pthread_create(&t, NULL, mm_userptr_close_thread, &t_data); + igt_assert(ret == 0); + + while (loops--) { + mprotect(t_data.ptr, PAGE_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC); + mprotect(t_data.ptr, PAGE_SIZE, PROT_READ | PROT_WRITE); + } + + t_data.stop = 1; + + pthread_join(t, NULL); + + free(t_data.ptr); +} + unsigned int total_ram; uint64_t aperture_size; int fd, count; @@ -1407,7 +1465,13 @@ int main(int argc, char **argv) test_unmap_after_close(fd); igt_subtest("stress-mm") - test_stress_mm(fd); + test_stress_mm(fd); + + igt_subtest("stress-mm-invalidate-close") + test_invalidate_close_race(fd, false); + + igt_subtest("stress-mm-invalidate-close-overlap") + test_invalidate_close_race(fd, true); igt_subtest("coherency-sync") test_coherency(fd, count);
It was possible for invalidate range start mmu notifier callback to race with releasing userptr object. If the object is released prior to taking a spinlock in the callback, we'll encounter a null pointer dereference. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> --- tests/gem_userptr_blits.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-)