[PATCH libdrm] intel: Serialize drmPrimeFDToHandle with struct_mutex

[PATCH libdrm] intel: Serialize drmPrimeFDToHandle with struct_mutex

[Michał Winiarski]

From: Rafał Sapała <rafal.a.sapala@intel.com>

It is possible to hit a race condition in create_from_prime, when trying
to import a BO that's currently being freed. In case of prime sharing
we'll succesfully get a handle, but fail on get_tiling call, potentially
confusing the caller (and requiring different locking scheme than with
sharing using flink). Wrap fd_to_handle with struct_mutex to force
a more consistent behaviour between prime/flink, convert fprintf to DBG
when handling errors.

Testcase: igt/drm_import_export/import-close-race-prime
Signed-off-by: Rafał Sapała <rafal.a.sapala@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 intel/intel_bufmgr_gem.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index b1c3b3a..ed4ffd2 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -2728,14 +2728,19 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
 	struct drm_i915_gem_get_tiling get_tiling;
 	drmMMListHead *list;
 
+	pthread_mutex_lock(&bufmgr_gem->lock);
 	ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle);
+	if (ret) {
+		DBG("create_from_prime: failed to obtain handle from fd: %s\n", strerror(errno));
+		pthread_mutex_unlock(&bufmgr_gem->lock);
+		return NULL;
+	}
 
 	/*
 	 * See if the kernel has already returned this buffer to us. Just as
 	 * for named buffers, we must not create two bo's pointing at the same
 	 * kernel object
 	 */
-	pthread_mutex_lock(&bufmgr_gem->lock);
 	for (list = bufmgr_gem->named.next;
 	     list != &bufmgr_gem->named;
 	     list = list->next) {
@@ -2747,12 +2752,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
 		}
 	}
 
-	if (ret) {
-	  fprintf(stderr,"ret is %d %d\n", ret, errno);
-	  pthread_mutex_unlock(&bufmgr_gem->lock);
-		return NULL;
-	}
-
 	bo_gem = calloc(1, sizeof(*bo_gem));
 	if (!bo_gem) {
 		pthread_mutex_unlock(&bufmgr_gem->lock);
@@ -2793,6 +2792,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
 		       DRM_IOCTL_I915_GEM_GET_TILING,
 		       &get_tiling);
 	if (ret != 0) {
+		DBG("create_from_prime: failed to get tiling: %s\n", strerror(errno));
 		drm_intel_gem_bo_unreference(&bo_gem->bo);
 		return NULL;
 	}
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] tests/drm_import_export: Add tests for prime/flink sharing races

[Michał Winiarski]

It is possible to race between unreference of the underlying BO and
importing it from prime_fd/name. Verify that the behaviour of libdrm
is consistent for prime/flink.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 tests/drm_import_export.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/tests/drm_import_export.c b/tests/drm_import_export.c
index 57b13dd..db11c18 100644
--- a/tests/drm_import_export.c
+++ b/tests/drm_import_export.c
@@ -131,6 +131,98 @@ static void * test_thread(void * par)
 	return NULL;
 }
 
+#define IMPORT_RACE_LOOPS 100000
+
+struct import_race_thread_data {
+	int prime_fd;
+	uint32_t flink_name;
+	unsigned int stop;
+	pthread_mutex_t mutex;
+};
+
+static void *import_close_thread(void *data)
+{
+	struct import_race_thread_data *t = (struct import_race_thread_data *)data;
+	drm_intel_bo *bo;
+	pthread_mutex_lock(&t->mutex);
+	while (!t->stop) {
+		pthread_mutex_unlock(&t->mutex);
+		bo = NULL;
+		if (use_flink)
+			bo = drm_intel_bo_gem_create_from_name(bufmgr, "buf-shared", t->flink_name);
+		else {
+			pthread_mutex_lock(&t->mutex);
+			if (t->prime_fd != -1) {
+				bo = drm_intel_bo_gem_create_from_prime(bufmgr, t->prime_fd, 4096);
+				pthread_mutex_unlock(&t->mutex);
+			}
+			else
+				/* We take the lock right after entering the loop */
+				continue;
+		}
+		if (bo == NULL) {
+			/*
+			 * If the bo is NULL it means that we've unreferenced in other
+			 * thread - therefore we should expect ENOENT
+			 */
+			igt_assert_eq(errno, ENOENT);
+			continue;
+		}
+
+		drm_intel_bo_unreference(bo);
+
+		pthread_mutex_lock(&t->mutex);
+	}
+	pthread_mutex_unlock(&t->mutex);
+
+	return NULL;
+}
+
+static void test_import_close_race(void)
+{
+	pthread_t t;
+	unsigned int loops = IMPORT_RACE_LOOPS;
+	drm_intel_bo *bo;
+	struct import_race_thread_data t_data;
+
+	memset(&t_data, 0, sizeof(t_data));
+	pthread_mutex_init(&t_data.mutex, NULL);
+	t_data.prime_fd = -1;
+
+	igt_assert_eq(pthread_create(&t, NULL, import_close_thread , &t_data), 0);
+
+	while (loops--) {
+		bo = drm_intel_bo_alloc(bufmgr, "buf-shared", 4096, 4096);
+		igt_assert(bo != NULL);
+		/*
+		 * We setup the test in such way, that create_from_* can race between
+		 * unreference. If we're using prime, prime_fd is always a valid fd.
+		 */
+		if (use_flink)
+			igt_assert_eq(drm_intel_bo_flink(bo, &(t_data.flink_name)), 0);
+		else {
+			pthread_mutex_lock(&t_data.mutex);
+			igt_assert_eq(drm_intel_bo_gem_export_to_prime(bo, &(t_data.prime_fd)), 0);
+			igt_assert(t_data.prime_fd != -1);
+			pthread_mutex_unlock(&t_data.mutex);
+		}
+
+		drm_intel_bo_unreference(bo);
+
+		pthread_mutex_lock(&t_data.mutex);
+		close(t_data.prime_fd);
+		t_data.prime_fd = -1;
+		pthread_mutex_unlock(&t_data.mutex);
+	}
+
+	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);
+}
+
 pthread_t test_thread_id1;
 pthread_t test_thread_id2;
 pthread_t test_thread_id3;
@@ -153,6 +245,16 @@ igt_main {
 		drm_intel_bufmgr_gem_enable_reuse(bufmgr);
 	}
 
+	igt_subtest("import-close-race-flink") {
+		use_flink = true;
+		test_import_close_race();
+	}
+
+	igt_subtest("import-close-race-prime") {
+		use_flink = false;
+		test_import_close_race();
+	}
+
 	igt_subtest("flink") {
 		use_flink = true;
 
@@ -180,4 +282,5 @@ igt_main {
 		pthread_join(test_thread_id3, NULL);
 		pthread_join(test_thread_id4, NULL);
 	}
+
 }
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/drm_import_export: Add tests for prime/flink sharing races

[Thomas Wood]

On 24 July 2015 at 10:24, Michał Winiarski <michal.winiarski@intel.com> wrote:
> It is possible to race between unreference of the underlying BO and
> importing it from prime_fd/name. Verify that the behaviour of libdrm
> is consistent for prime/flink.

Could you add this description into the source file as a comment?
There also appears to be an extra white space change at the end of
your patch.

>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  tests/drm_import_export.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
>
> diff --git a/tests/drm_import_export.c b/tests/drm_import_export.c
> index 57b13dd..db11c18 100644
> --- a/tests/drm_import_export.c
> +++ b/tests/drm_import_export.c
> @@ -131,6 +131,98 @@ static void * test_thread(void * par)
>         return NULL;
>  }
>
> +#define IMPORT_RACE_LOOPS 100000
> +
> +struct import_race_thread_data {
> +       int prime_fd;
> +       uint32_t flink_name;
> +       unsigned int stop;
> +       pthread_mutex_t mutex;
> +};
> +
> +static void *import_close_thread(void *data)
> +{
> +       struct import_race_thread_data *t = (struct import_race_thread_data *)data;
> +       drm_intel_bo *bo;
> +       pthread_mutex_lock(&t->mutex);
> +       while (!t->stop) {
> +               pthread_mutex_unlock(&t->mutex);
> +               bo = NULL;
> +               if (use_flink)
> +                       bo = drm_intel_bo_gem_create_from_name(bufmgr, "buf-shared", t->flink_name);
> +               else {
> +                       pthread_mutex_lock(&t->mutex);
> +                       if (t->prime_fd != -1) {
> +                               bo = drm_intel_bo_gem_create_from_prime(bufmgr, t->prime_fd, 4096);
> +                               pthread_mutex_unlock(&t->mutex);
> +                       }
> +                       else
> +                               /* We take the lock right after entering the loop */
> +                               continue;
> +               }
> +               if (bo == NULL) {
> +                       /*
> +                        * If the bo is NULL it means that we've unreferenced in other
> +                        * thread - therefore we should expect ENOENT
> +                        */
> +                       igt_assert_eq(errno, ENOENT);
> +                       continue;
> +               }
> +
> +               drm_intel_bo_unreference(bo);
> +
> +               pthread_mutex_lock(&t->mutex);
> +       }
> +       pthread_mutex_unlock(&t->mutex);
> +
> +       return NULL;
> +}
> +
> +static void test_import_close_race(void)
> +{
> +       pthread_t t;
> +       unsigned int loops = IMPORT_RACE_LOOPS;
> +       drm_intel_bo *bo;
> +       struct import_race_thread_data t_data;
> +
> +       memset(&t_data, 0, sizeof(t_data));
> +       pthread_mutex_init(&t_data.mutex, NULL);
> +       t_data.prime_fd = -1;
> +
> +       igt_assert_eq(pthread_create(&t, NULL, import_close_thread , &t_data), 0);
> +
> +       while (loops--) {
> +               bo = drm_intel_bo_alloc(bufmgr, "buf-shared", 4096, 4096);
> +               igt_assert(bo != NULL);
> +               /*
> +                * We setup the test in such way, that create_from_* can race between
> +                * unreference. If we're using prime, prime_fd is always a valid fd.
> +                */
> +               if (use_flink)
> +                       igt_assert_eq(drm_intel_bo_flink(bo, &(t_data.flink_name)), 0);
> +               else {
> +                       pthread_mutex_lock(&t_data.mutex);
> +                       igt_assert_eq(drm_intel_bo_gem_export_to_prime(bo, &(t_data.prime_fd)), 0);
> +                       igt_assert(t_data.prime_fd != -1);
> +                       pthread_mutex_unlock(&t_data.mutex);
> +               }
> +
> +               drm_intel_bo_unreference(bo);
> +
> +               pthread_mutex_lock(&t_data.mutex);
> +               close(t_data.prime_fd);
> +               t_data.prime_fd = -1;
> +               pthread_mutex_unlock(&t_data.mutex);
> +       }
> +
> +       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);
> +}
> +
>  pthread_t test_thread_id1;
>  pthread_t test_thread_id2;
>  pthread_t test_thread_id3;
> @@ -153,6 +245,16 @@ igt_main {
>                 drm_intel_bufmgr_gem_enable_reuse(bufmgr);
>         }
>
> +       igt_subtest("import-close-race-flink") {
> +               use_flink = true;
> +               test_import_close_race();
> +       }
> +
> +       igt_subtest("import-close-race-prime") {
> +               use_flink = false;
> +               test_import_close_race();
> +       }
> +
>         igt_subtest("flink") {
>                 use_flink = true;
>
> @@ -180,4 +282,5 @@ igt_main {
>                 pthread_join(test_thread_id3, NULL);
>                 pthread_join(test_thread_id4, NULL);
>         }
> +
>  }
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2] tests/drm_import_export: Add tests for prime/flink sharing races

[Michał Winiarski]

It is possible to race between unreference of the underlying BO and
importing it from prime_fd/name. Verify that the behaviour of libdrm
is consistent for prime/flink.

v2: more comments in source file, dropped extra whitespace

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Thomas Wood <thomas.wood@intel.com>
---
 tests/drm_import_export.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/tests/drm_import_export.c b/tests/drm_import_export.c
index 57b13dd..e24e0df 100644
--- a/tests/drm_import_export.c
+++ b/tests/drm_import_export.c
@@ -131,6 +131,108 @@ static void * test_thread(void * par)
 	return NULL;
 }
 
+#define IMPORT_RACE_LOOPS 100000
+
+struct import_race_thread_data {
+	int prime_fd;
+	uint32_t flink_name;
+	unsigned int stop;
+	pthread_mutex_t mutex;
+};
+
+/*
+ * Attempt to import the bo. It is possible that GEM_CLOSE was already called
+ * in different thread and from i915 point of view the handle is no longer
+ * valid (thus create_from_prime/name should fail).
+ */
+static void *import_close_thread(void *data)
+{
+	struct import_race_thread_data *t = (struct import_race_thread_data *)data;
+	drm_intel_bo *bo;
+	pthread_mutex_lock(&t->mutex);
+	while (!t->stop) {
+		pthread_mutex_unlock(&t->mutex);
+		bo = NULL;
+		if (use_flink)
+			bo = drm_intel_bo_gem_create_from_name(bufmgr, "buf-shared", t->flink_name);
+		else {
+			pthread_mutex_lock(&t->mutex);
+			if (t->prime_fd != -1) {
+				bo = drm_intel_bo_gem_create_from_prime(bufmgr, t->prime_fd, 4096);
+				pthread_mutex_unlock(&t->mutex);
+			}
+			else
+				/* We take the lock right after entering the loop */
+				continue;
+		}
+		if (bo == NULL) {
+			/*
+			 * If the bo is NULL it means that we've unreferenced in other
+			 * thread - therefore we should expect ENOENT
+			 */
+			igt_assert_eq(errno, ENOENT);
+			continue;
+		}
+
+		drm_intel_bo_unreference(bo);
+
+		pthread_mutex_lock(&t->mutex);
+	}
+	pthread_mutex_unlock(&t->mutex);
+
+	return NULL;
+}
+
+/*
+ * It is possible to race between unreference of the underlying BO and importing
+ * it from prime_fd/name. Verify that the behaviour of libdrm is consistent for
+ * prime/flink.
+ */
+static void test_import_close_race(void)
+{
+	pthread_t t;
+	unsigned int loops = IMPORT_RACE_LOOPS;
+	drm_intel_bo *bo;
+	struct import_race_thread_data t_data;
+
+	memset(&t_data, 0, sizeof(t_data));
+	pthread_mutex_init(&t_data.mutex, NULL);
+	t_data.prime_fd = -1;
+
+	igt_assert_eq(pthread_create(&t, NULL, import_close_thread , &t_data), 0);
+
+	while (loops--) {
+		bo = drm_intel_bo_alloc(bufmgr, "buf-shared", 4096, 4096);
+		igt_assert(bo != NULL);
+		/*
+		 * We setup the test in such way, that create_from_* can race between
+		 * unreference. If we're using prime, prime_fd is always a valid fd.
+		 */
+		if (use_flink)
+			igt_assert_eq(drm_intel_bo_flink(bo, &(t_data.flink_name)), 0);
+		else {
+			pthread_mutex_lock(&t_data.mutex);
+			igt_assert_eq(drm_intel_bo_gem_export_to_prime(bo, &(t_data.prime_fd)), 0);
+			igt_assert(t_data.prime_fd != -1);
+			pthread_mutex_unlock(&t_data.mutex);
+		}
+
+		drm_intel_bo_unreference(bo);
+
+		pthread_mutex_lock(&t_data.mutex);
+		close(t_data.prime_fd);
+		t_data.prime_fd = -1;
+		pthread_mutex_unlock(&t_data.mutex);
+	}
+
+	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);
+}
+
 pthread_t test_thread_id1;
 pthread_t test_thread_id2;
 pthread_t test_thread_id3;
@@ -153,6 +255,16 @@ igt_main {
 		drm_intel_bufmgr_gem_enable_reuse(bufmgr);
 	}
 
+	igt_subtest("import-close-race-flink") {
+		use_flink = true;
+		test_import_close_race();
+	}
+
+	igt_subtest("import-close-race-prime") {
+		use_flink = false;
+		test_import_close_race();
+	}
+
 	igt_subtest("flink") {
 		use_flink = true;
 
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2] tests/drm_import_export: Add tests for prime/flink sharing races

[Thomas Wood]

On 24 July 2015 at 15:43, Michał Winiarski <michal.winiarski@intel.com> wrote:
> It is possible to race between unreference of the underlying BO and
> importing it from prime_fd/name. Verify that the behaviour of libdrm
> is consistent for prime/flink.
>
> v2: more comments in source file, dropped extra whitespace

Thanks, patch pushed.

>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Thomas Wood <thomas.wood@intel.com>
> ---
>  tests/drm_import_export.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>
> diff --git a/tests/drm_import_export.c b/tests/drm_import_export.c
> index 57b13dd..e24e0df 100644
> --- a/tests/drm_import_export.c
> +++ b/tests/drm_import_export.c
> @@ -131,6 +131,108 @@ static void * test_thread(void * par)
>         return NULL;
>  }
>
> +#define IMPORT_RACE_LOOPS 100000
> +
> +struct import_race_thread_data {
> +       int prime_fd;
> +       uint32_t flink_name;
> +       unsigned int stop;
> +       pthread_mutex_t mutex;
> +};
> +
> +/*
> + * Attempt to import the bo. It is possible that GEM_CLOSE was already called
> + * in different thread and from i915 point of view the handle is no longer
> + * valid (thus create_from_prime/name should fail).
> + */
> +static void *import_close_thread(void *data)
> +{
> +       struct import_race_thread_data *t = (struct import_race_thread_data *)data;
> +       drm_intel_bo *bo;
> +       pthread_mutex_lock(&t->mutex);
> +       while (!t->stop) {
> +               pthread_mutex_unlock(&t->mutex);
> +               bo = NULL;
> +               if (use_flink)
> +                       bo = drm_intel_bo_gem_create_from_name(bufmgr, "buf-shared", t->flink_name);
> +               else {
> +                       pthread_mutex_lock(&t->mutex);
> +                       if (t->prime_fd != -1) {
> +                               bo = drm_intel_bo_gem_create_from_prime(bufmgr, t->prime_fd, 4096);
> +                               pthread_mutex_unlock(&t->mutex);
> +                       }
> +                       else
> +                               /* We take the lock right after entering the loop */
> +                               continue;
> +               }
> +               if (bo == NULL) {
> +                       /*
> +                        * If the bo is NULL it means that we've unreferenced in other
> +                        * thread - therefore we should expect ENOENT
> +                        */
> +                       igt_assert_eq(errno, ENOENT);
> +                       continue;
> +               }
> +
> +               drm_intel_bo_unreference(bo);
> +
> +               pthread_mutex_lock(&t->mutex);
> +       }
> +       pthread_mutex_unlock(&t->mutex);
> +
> +       return NULL;
> +}
> +
> +/*
> + * It is possible to race between unreference of the underlying BO and importing
> + * it from prime_fd/name. Verify that the behaviour of libdrm is consistent for
> + * prime/flink.
> + */
> +static void test_import_close_race(void)
> +{
> +       pthread_t t;
> +       unsigned int loops = IMPORT_RACE_LOOPS;
> +       drm_intel_bo *bo;
> +       struct import_race_thread_data t_data;
> +
> +       memset(&t_data, 0, sizeof(t_data));
> +       pthread_mutex_init(&t_data.mutex, NULL);
> +       t_data.prime_fd = -1;
> +
> +       igt_assert_eq(pthread_create(&t, NULL, import_close_thread , &t_data), 0);
> +
> +       while (loops--) {
> +               bo = drm_intel_bo_alloc(bufmgr, "buf-shared", 4096, 4096);
> +               igt_assert(bo != NULL);
> +               /*
> +                * We setup the test in such way, that create_from_* can race between
> +                * unreference. If we're using prime, prime_fd is always a valid fd.
> +                */
> +               if (use_flink)
> +                       igt_assert_eq(drm_intel_bo_flink(bo, &(t_data.flink_name)), 0);
> +               else {
> +                       pthread_mutex_lock(&t_data.mutex);
> +                       igt_assert_eq(drm_intel_bo_gem_export_to_prime(bo, &(t_data.prime_fd)), 0);
> +                       igt_assert(t_data.prime_fd != -1);
> +                       pthread_mutex_unlock(&t_data.mutex);
> +               }
> +
> +               drm_intel_bo_unreference(bo);
> +
> +               pthread_mutex_lock(&t_data.mutex);
> +               close(t_data.prime_fd);
> +               t_data.prime_fd = -1;
> +               pthread_mutex_unlock(&t_data.mutex);
> +       }
> +
> +       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);
> +}
> +
>  pthread_t test_thread_id1;
>  pthread_t test_thread_id2;
>  pthread_t test_thread_id3;
> @@ -153,6 +255,16 @@ igt_main {
>                 drm_intel_bufmgr_gem_enable_reuse(bufmgr);
>         }
>
> +       igt_subtest("import-close-race-flink") {
> +               use_flink = true;
> +               test_import_close_race();
> +       }
> +
> +       igt_subtest("import-close-race-prime") {
> +               use_flink = false;
> +               test_import_close_race();
> +       }
> +
>         igt_subtest("flink") {
>                 use_flink = true;
>
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH libdrm] intel: Serialize drmPrimeFDToHandle with struct_mutex

[Chris Wilson]

On Fri, Jul 24, 2015 at 11:22:34AM +0200, Michał Winiarski wrote:
> From: Rafał Sapała <rafal.a.sapala@intel.com>
> 
> It is possible to hit a race condition in create_from_prime, when trying
> to import a BO that's currently being freed. In case of prime sharing
> we'll succesfully get a handle, but fail on get_tiling call, potentially
> confusing the caller (and requiring different locking scheme than with
> sharing using flink). Wrap fd_to_handle with struct_mutex to force
> a more consistent behaviour between prime/flink, convert fprintf to DBG
> when handling errors.

The race is that the kernel returns us the same file-private handle as
the first thread, but that first thread is about to call gem_close
(thereby removing the handle from the file completely) and does so
between us acquiring the handle and taking the mutex. If we take
the mutex, then we acquire the refcnt on the bo prior to the first
thread completing its unref (and so preventing the early close). Or we
acquire the handle after the earlier close, in which case we are the new
owner.

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

Re: [PATCH libdrm] intel: Serialize drmPrimeFDToHandle with struct_mutex

[Damien Lespiau]

On Fri, Jul 24, 2015 at 11:51:01AM +0100, Chris Wilson wrote:
> On Fri, Jul 24, 2015 at 11:22:34AM +0200, Michał Winiarski wrote:
> > From: Rafał Sapała <rafal.a.sapala@intel.com>
> > 
> > It is possible to hit a race condition in create_from_prime, when trying
> > to import a BO that's currently being freed. In case of prime sharing
> > we'll succesfully get a handle, but fail on get_tiling call, potentially
> > confusing the caller (and requiring different locking scheme than with
> > sharing using flink). Wrap fd_to_handle with struct_mutex to force
> > a more consistent behaviour between prime/flink, convert fprintf to DBG
> > when handling errors.
> 
> The race is that the kernel returns us the same file-private handle as
> the first thread, but that first thread is about to call gem_close
> (thereby removing the handle from the file completely) and does so
> between us acquiring the handle and taking the mutex. If we take
> the mutex, then we acquire the refcnt on the bo prior to the first
> thread completing its unref (and so preventing the early close). Or we
> acquire the handle after the earlier close, in which case we are the new
> owner.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the patch & review, pushed.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx