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@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
