Re: [PATCH] tests/gem_userptr_blits: Race between close and invalidate

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx