* [PATCH] tests/gem_userptr_blits: Race between close and invalidate
@ 2015-02-03 14:39 Michał Winiarski
2015-02-03 15:01 ` Chris Wilson
2015-02-03 19:13 ` [PATCH v2] " Michał Winiarski
0 siblings, 2 replies; 6+ messages in thread
From: Michał Winiarski @ 2015-02-03 14:39 UTC (permalink / raw)
To: intel-gfx
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);
+ 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);
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tests/gem_userptr_blits: Race between close and invalidate
2015-02-03 14:39 [PATCH] tests/gem_userptr_blits: Race between close and invalidate Michał Winiarski
@ 2015-02-03 15:01 ` Chris Wilson
2015-02-03 16:08 ` Daniel Vetter
2015-02-03 19:13 ` [PATCH v2] " Michał Winiarski
1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2015-02-03 15:01 UTC (permalink / raw)
To: Michał Winiarski; +Cc: intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tests/gem_userptr_blits: Race between close and invalidate
2015-02-03 15:01 ` Chris Wilson
@ 2015-02-03 16:08 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-02-03 16:08 UTC (permalink / raw)
To: Chris Wilson, Michał Winiarski, intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] tests/gem_userptr_blits: Race between close and invalidate
2015-02-03 14:39 [PATCH] tests/gem_userptr_blits: Race between close and invalidate Michał Winiarski
2015-02-03 15:01 ` Chris Wilson
@ 2015-02-03 19:13 ` Michał Winiarski
2015-02-03 20:24 ` Chris Wilson
1 sibling, 1 reply; 6+ messages in thread
From: Michał Winiarski @ 2015-02-03 19:13 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
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.
v2: Moved expressions inside igt_assert(), added mem barrier (Chris)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
tests/gem_userptr_blits.c | 71 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index be2fdf9..9217c2a 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,65 @@ 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;
+ pthread_mutex_t mutex;
+};
+
+static void *mm_userptr_close_thread(void *data)
+{
+ struct userptr_close_thread_data *t = (struct userptr_close_thread_data *)data;
+ int num_handles = t->overlap ? 2 : 1;
+
+ uint32_t handle[num_handles];
+
+ /* 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 < num_handles; i++)
+ igt_assert(gem_userptr(t->fd, t->ptr, PAGE_SIZE, 0, &handle[i]) == 0);
+ for (int i = 0; i < num_handles; i++)
+ gem_close(t->fd, handle[i]);
+ pthread_mutex_lock(&t->mutex);
+ }
+ pthread_mutex_unlock(&t->mutex);
+
+ return NULL;
+}
+
+static void test_invalidate_close_race(int fd, bool overlap)
+{
+ 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);
+ pthread_mutex_init(&t_data.mutex, NULL);
+
+ igt_assert(pthread_create(&t, NULL, mm_userptr_close_thread, &t_data) == 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);
+ }
+
+ pthread_mutex_lock(&t_data.mutex);
+ t_data.stop = 1;
+ pthread_mutex_unlock(&t_data.mutex);
+
+ pthread_join(t, NULL);
+
+ pthread_mutex_destroy(&t_data.mutex);
+ free(t_data.ptr);
+}
+
unsigned int total_ram;
uint64_t aperture_size;
int fd, count;
@@ -1407,7 +1468,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);
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] tests/gem_userptr_blits: Race between close and invalidate
2015-02-03 19:13 ` [PATCH v2] " Michał Winiarski
@ 2015-02-03 20:24 ` Chris Wilson
2015-02-04 9:29 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2015-02-03 20:24 UTC (permalink / raw)
To: Michał Winiarski; +Cc: Daniel Vetter, intel-gfx
On Tue, Feb 03, 2015 at 08:13:56PM +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.
>
> v2: Moved expressions inside igt_assert(), added mem barrier (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Lgtm,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tests/gem_userptr_blits: Race between close and invalidate
2015-02-03 20:24 ` Chris Wilson
@ 2015-02-04 9:29 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-02-04 9:29 UTC (permalink / raw)
To: Chris Wilson, Michał Winiarski, intel-gfx, Daniel Vetter
On Tue, Feb 03, 2015 at 08:24:12PM +0000, Chris Wilson wrote:
> On Tue, Feb 03, 2015 at 08:13:56PM +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.
> >
> > v2: Moved expressions inside igt_assert(), added mem barrier (Chris)
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>
> Lgtm,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Applied, thanks.
-Daniel
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-04 9:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-03 14:39 [PATCH] tests/gem_userptr_blits: Race between close and invalidate Michał Winiarski
2015-02-03 15:01 ` Chris Wilson
2015-02-03 16:08 ` Daniel Vetter
2015-02-03 19:13 ` [PATCH v2] " Michał Winiarski
2015-02-03 20:24 ` Chris Wilson
2015-02-04 9:29 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox