[PATCH 00/20] prime patches

[PATCH 00/20] prime patches

[Daniel Vetter]

Hi all,

So I've finally tracked down the last thing which I didn't really understand in
my earlier patches and made sure it won't ever break again by writing testcases.

The one thing still left to do (but I have tests for it already) is to make sure
we don't duplicate buffers when importing foreign buffers on two open fds. This
is the use-case for which the exynos guys recently posted a few hacky patches.

I've already merged the i915 patches from this series. Since there's no real
functional depency all the patches here can go through drm-next without issues.

Comments&flames highly welcome.

Cheers, Daniel

Daniel Vetter (19):
  drm: use common drm_gem_dmabuf_release in i915/exynos drivers
  drm/prime: remove cargo-cult locking from map_sg helper
  drm/prime: add a bit of documentation about gem_obj->import_attach
  drm/gem: move drm_gem_object_handle_unreference_unlocked into
    drm_gem.c
  drm/gem: remove bogus NULL check from
    drm_gem_object_handle_unreference_unlocked
  drm/gem: WARN about unbalanced handle refcounts
  drm/gem: fix up flink name create race
  drm/prime: fix error path in drm_gem_prime_fd_to_handle
  drm/gem: make drm_gem_object_handle_unreference_unlocked static
  drm/gem: create drm_gem_dumb_destroy
  drm/prime: use proper pointer in drm_gem_prime_handle_to_fd
  drm/prime: shrink critical section protected by prime lock
  drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle
  drm/gem: switch dev->object_name_lock to a mutex
  drm/gem: completely close gem_open vs. gem_close races
  drm/prime: proper locking+refcounting for obj->dma_buf link
  drm/prime: Simplify drm_gem_remove_prime_handles
  drm/prime: make drm_prime_lookup_buf_handle static
  drm/prime: Always add exported buffers to the handle cache

Inki Dae (1):
  drm/exynos: explicit store base gem object in dma_buf->priv

 drivers/gpu/drm/drm_fops.c                 |   1 +
 drivers/gpu/drm/drm_gem.c                  | 178 ++++++++++++++++++---------
 drivers/gpu/drm/drm_info.c                 |   2 +-
 drivers/gpu/drm/drm_prime.c                | 190 ++++++++++++++++++-----------
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |  35 ++----
 drivers/gpu/drm/exynos/exynos_drm_gem.c    |   2 +-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     |  13 +-
 include/drm/drmP.h                         |  79 ++++++------
 8 files changed, 297 insertions(+), 203 deletions(-)

-- 
1.8.3.2

[PATCH 01/20] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

[Daniel Vetter]

Note that this is slightly tricky since both drivers store their
native objects in dma_buf->priv. But both also embed the base
drm_gem_object at the first position, so the implicit cast is ok.

To use the release helper we need to export it, too.

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c                |  3 ++-
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +----------------------
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     | 13 +------------
 include/drm/drmP.h                         |  1 +
 4 files changed, 5 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 85e450e..a35f206 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -192,7 +192,7 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 	/* nothing to be done here */
 }
 
-static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
@@ -202,6 +202,7 @@ static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 		drm_gem_object_unreference_unlocked(obj);
 	}
 }
+EXPORT_SYMBOL(drm_gem_dmabuf_release);
 
 static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index a0f997e..3cd56e1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -127,27 +127,6 @@ static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 	/* Nothing to do. */
 }
 
-static void exynos_dmabuf_release(struct dma_buf *dmabuf)
-{
-	struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv;
-
-	/*
-	 * exynos_dmabuf_release() call means that file object's
-	 * f_count is 0 and it calls drm_gem_object_handle_unreference()
-	 * to drop the references that these values had been increased
-	 * at drm_prime_handle_to_fd()
-	 */
-	if (exynos_gem_obj->base.export_dma_buf == dmabuf) {
-		exynos_gem_obj->base.export_dma_buf = NULL;
-
-		/*
-		 * drop this gem object refcount to release allocated buffer
-		 * and resources.
-		 */
-		drm_gem_object_unreference_unlocked(&exynos_gem_obj->base);
-	}
-}
-
 static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
 						unsigned long page_num)
 {
@@ -193,7 +172,7 @@ static struct dma_buf_ops exynos_dmabuf_ops = {
 	.kunmap			= exynos_gem_dmabuf_kunmap,
 	.kunmap_atomic		= exynos_gem_dmabuf_kunmap_atomic,
 	.mmap			= exynos_gem_dmabuf_mmap,
-	.release		= exynos_dmabuf_release,
+	.release		= drm_gem_dmabuf_release,
 };
 
 struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index f7af7e4..e918b05 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -103,17 +103,6 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
 	mutex_unlock(&obj->base.dev->struct_mutex);
 }
 
-static void i915_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
-	struct drm_i915_gem_object *obj = dma_buf->priv;
-
-	if (obj->base.export_dma_buf == dma_buf) {
-		/* drop the reference on the export fd holds */
-		obj->base.export_dma_buf = NULL;
-		drm_gem_object_unreference_unlocked(&obj->base);
-	}
-}
-
 static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
@@ -224,7 +213,7 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size
 static const struct dma_buf_ops i915_dmabuf_ops =  {
 	.map_dma_buf = i915_gem_map_dma_buf,
 	.unmap_dma_buf = i915_gem_unmap_dma_buf,
-	.release = i915_gem_dmabuf_release,
+	.release = drm_gem_dmabuf_release,
 	.kmap = i915_gem_dmabuf_kmap,
 	.kmap_atomic = i915_gem_dmabuf_kmap_atomic,
 	.kunmap = i915_gem_dmabuf_kunmap,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3ecdde6..016e75e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1495,6 +1495,7 @@ extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 		struct dma_buf *dma_buf);
 extern int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
 
 extern int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 					struct drm_file *file_priv);
-- 
1.8.3.2

[PATCH 02/20] drm/exynos: explicit store base gem object in dma_buf->priv

[Daniel Vetter]

From: Inki Dae <inki.dae@samsung.com>

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 3cd56e1..fd76449 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -22,6 +22,11 @@ struct exynos_drm_dmabuf_attachment {
 	bool is_mapped;
 };
 
+static struct exynos_drm_gem_obj *dma_buf_to_obj(struct dma_buf *buf)
+{
+	return to_exynos_gem_obj(buf->priv);
+}
+
 static int exynos_gem_attach_dma_buf(struct dma_buf *dmabuf,
 					struct device *dev,
 					struct dma_buf_attachment *attach)
@@ -63,7 +68,7 @@ static struct sg_table *
 					enum dma_data_direction dir)
 {
 	struct exynos_drm_dmabuf_attachment *exynos_attach = attach->priv;
-	struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv;
+	struct exynos_drm_gem_obj *gem_obj = dma_buf_to_obj(attach->dmabuf);
 	struct drm_device *dev = gem_obj->base.dev;
 	struct exynos_drm_gem_buf *buf;
 	struct scatterlist *rd, *wr;
@@ -180,7 +185,7 @@ struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev,
 {
 	struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj);
 
-	return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops,
+	return dma_buf_export(obj, &exynos_dmabuf_ops,
 				exynos_gem_obj->base.size, flags);
 }
 
@@ -198,8 +203,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 	if (dma_buf->ops == &exynos_dmabuf_ops) {
 		struct drm_gem_object *obj;
 
-		exynos_gem_obj = dma_buf->priv;
-		obj = &exynos_gem_obj->base;
+		obj = dma_buf->priv;
 
 		/* is it from our device? */
 		if (obj->dev == drm_dev) {
-- 
1.8.3.2

[PATCH 03/20] drm/prime: remove cargo-cult locking from map_sg helper

[Daniel Vetter]

I've checked both implementations (radeon/nouveau) and they both grab
the page array from ttm simply by dereferencing it and then wrapping
it up with drm_prime_pages_to_sg in the callback and map it with
dma_map_sg (in the helper).

Only the grabbing of the underlying page array is anything we need to
be concerned about, and either those pages are pinned independently,
or we're screwed no matter what.

And indeed, nouveau/radeon pin the backing storage in their
attach/detach functions.

Since I've created this patch cma prime support for dma_buf was added.
drm_gem_cma_prime_get_sg_table only calls kzalloc and the creates&maps
the sg table with dma_get_sgtable. It doesn't touch any gem object
state otherwise. So the cma helpers also look safe.

The only thing we might claim it does is prevent concurrent mapping of
dma_buf attachments. But a) that's not allowed and b) the current code
is racy already since it checks whether the sg mapping exists _before_
grabbing the lock.

So the dev->struct_mutex locking here does absolutely nothing useful,
but only distracts. Remove it.

This should also help Maarten's work to eventually pin the backing
storage more dynamically by preventing locking inversions around
dev->struct_mutex.

v2: Add analysis for recently added cma helper prime code.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index a35f206..f115962 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -167,8 +167,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
 	if (WARN_ON(prime_attach->dir != DMA_NONE))
 		return ERR_PTR(-EBUSY);
 
-	mutex_lock(&obj->dev->struct_mutex);
-
 	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
 
 	if (!IS_ERR(sgt)) {
@@ -182,7 +180,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
 		}
 	}
 
-	mutex_unlock(&obj->dev->struct_mutex);
 	return sgt;
 }
 
-- 
1.8.3.2

[PATCH 04/20] drm/prime: add a bit of documentation about gem_obj->import_attach

[Daniel Vetter]

Lifetime rules seem to be solid around ->import_attach. So this patch
just properly documents them.

Note that pointing directly at the attachment might have issues for
devices that have multiple struct device *dev parts constituting the
logical gpu and so might need multiple attachment points. Similarly
for drm devices which don't need a dma attachment at all (like udl).

But fixing that up is material for different patches.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drmP.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 016e75e..3711e97 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -678,7 +678,16 @@ struct drm_gem_object {
 	/* dma buf exported from this GEM object */
 	struct dma_buf *export_dma_buf;
 
-	/* dma buf attachment backing this object */
+	/**
+	 * import_attach - dma buf attachment backing this object
+	 *
+	 * Any foreign dma_buf imported as a gem object has this set to the
+	 * attachment point for the device. This is invariant over the lifetime
+	 * of a gem object.
+	 *
+	 * The driver's ->gem_free_object callback is responsible for cleaning
+	 * up the dma_buf attachment and references acquired at import time.
+	 */
 	struct dma_buf_attachment *import_attach;
 };
 
-- 
1.8.3.2

[PATCH 05/20] drm/gem: move drm_gem_object_handle_unreference_unlocked into drm_gem.c

[Daniel Vetter]

We have three callers of this function now and it's neither
performance critical nor really small. So an inline function feels
like overkill and unecessarily separates the different parts of the
code.

Since all callers of drm_gem_object_handle_free are now in drm_gem.c
we can make that static (and remove the unused EXPORT_SYMBOL). To
avoid a forward declaration move it (and drm_gem_object_free_bug) up a
bit.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 89 ++++++++++++++++++++++++++++-------------------
 include/drm/drmP.h        | 21 +----------
 2 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9ab038c..77c7d19 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -201,6 +201,60 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 	}
 }
 
+static void drm_gem_object_ref_bug(struct kref *list_kref)
+{
+	BUG();
+}
+
+/**
+ * Called after the last handle to the object has been closed
+ *
+ * Removes any name for the object. Note that this must be
+ * called before drm_gem_object_free or we'll be touching
+ * freed memory
+ */
+static void drm_gem_object_handle_free(struct drm_gem_object *obj)
+{
+	struct drm_device *dev = obj->dev;
+
+	/* Remove any name for this object */
+	spin_lock(&dev->object_name_lock);
+	if (obj->name) {
+		idr_remove(&dev->object_name_idr, obj->name);
+		obj->name = 0;
+		spin_unlock(&dev->object_name_lock);
+		/*
+		 * The object name held a reference to this object, drop
+		 * that now.
+		*
+		* This cannot be the last reference, since the handle holds one too.
+		 */
+		kref_put(&obj->refcount, drm_gem_object_ref_bug);
+	} else
+		spin_unlock(&dev->object_name_lock);
+
+}
+
+void
+drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
+{
+	if (obj == NULL)
+		return;
+
+	if (atomic_read(&obj->handle_count) == 0)
+		return;
+
+	/*
+	* Must bump handle count first as this may be the last
+	* ref, in which case the object would disappear before we
+	* checked for a name
+	*/
+
+	if (atomic_dec_and_test(&obj->handle_count))
+		drm_gem_object_handle_free(obj);
+	drm_gem_object_unreference_unlocked(obj);
+}
+
 /**
  * Removes the mapping from handle to filp for this object.
  */
@@ -533,41 +587,6 @@ drm_gem_object_free(struct kref *kref)
 }
 EXPORT_SYMBOL(drm_gem_object_free);
 
-static void drm_gem_object_ref_bug(struct kref *list_kref)
-{
-	BUG();
-}
-
-/**
- * Called after the last handle to the object has been closed
- *
- * Removes any name for the object. Note that this must be
- * called before drm_gem_object_free or we'll be touching
- * freed memory
- */
-void drm_gem_object_handle_free(struct drm_gem_object *obj)
-{
-	struct drm_device *dev = obj->dev;
-
-	/* Remove any name for this object */
-	spin_lock(&dev->object_name_lock);
-	if (obj->name) {
-		idr_remove(&dev->object_name_idr, obj->name);
-		obj->name = 0;
-		spin_unlock(&dev->object_name_lock);
-		/*
-		 * The object name held a reference to this object, drop
-		 * that now.
-		*
-		* This cannot be the last reference, since the handle holds one too.
-		 */
-		kref_put(&obj->refcount, drm_gem_object_ref_bug);
-	} else
-		spin_unlock(&dev->object_name_lock);
-
-}
-EXPORT_SYMBOL(drm_gem_object_handle_free);
-
 void drm_gem_vm_open(struct vm_area_struct *vma)
 {
 	struct drm_gem_object *obj = vma->vm_private_data;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3711e97..91c8d05 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1572,7 +1572,6 @@ int drm_gem_object_init(struct drm_device *dev,
 			struct drm_gem_object *obj, size_t size);
 void drm_gem_private_object_init(struct drm_device *dev,
 				 struct drm_gem_object *obj, size_t size);
-void drm_gem_object_handle_free(struct drm_gem_object *obj);
 void drm_gem_vm_open(struct vm_area_struct *vma);
 void drm_gem_vm_close(struct vm_area_struct *vma);
 int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
@@ -1619,25 +1618,7 @@ drm_gem_object_handle_reference(struct drm_gem_object *obj)
 	atomic_inc(&obj->handle_count);
 }
 
-static inline void
-drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
-{
-	if (obj == NULL)
-		return;
-
-	if (atomic_read(&obj->handle_count) == 0)
-		return;
-
-	/*
-	* Must bump handle count first as this may be the last
-	* ref, in which case the object would disappear before we
-	* checked for a name
-	*/
-
-	if (atomic_dec_and_test(&obj->handle_count))
-		drm_gem_object_handle_free(obj);
-	drm_gem_object_unreference_unlocked(obj);
-}
+void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
 
 void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
 int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
-- 
1.8.3.2

[PATCH 06/20] drm/gem: remove bogus NULL check from drm_gem_object_handle_unreference_unlocked

[Daniel Vetter]

Calling this function with a NULL object is simply a bug, so papering
over a NULL object not a good idea.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 77c7d19..b1d9e25 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -238,9 +238,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
-	if (obj == NULL)
-		return;
-
 	if (atomic_read(&obj->handle_count) == 0)
 		return;
 
-- 
1.8.3.2

[PATCH 07/20] drm/gem: WARN about unbalanced handle refcounts

[Daniel Vetter]

Trying to drop a reference we don't have is a pretty serious bug.
Trying to paper over it is an even worse offense.

So scream into dmesg with a big WARN in case that ever happens.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index b1d9e25..9118f5f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -238,7 +238,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
-	if (atomic_read(&obj->handle_count) == 0)
+	if (WARN_ON(atomic_read(&obj->handle_count) == 0))
 		return;
 
 	/*
-- 
1.8.3.2

[PATCH 08/20] drm/gem: fix up flink name create race

[Daniel Vetter]

This is the 2nd attempt, I've always been a bit dissatisified with the
tricky nature of the first one:

http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html

The issue is that the flink ioctl can race with calling gem_close on
the last gem handle. In that case we'll end up with a zero handle
count, but an flink name (and it's corresponding reference). Which
results in a neat space leak.

In my first attempt I've solved this by rechecking the handle count.
But fundamentally the issue is that ->handle_count isn't your usual
refcount - it can be resurrected from 0 among other things.

For those special beasts atomic_t often suggest way more ordering that
it actually guarantees. To prevent being tricked by those hairy
semantics take the easy way out and simply protect the handle with the
existing dev->object_name_lock.

With that change implemented it's dead easy to fix the flink vs. gem
close reace: When we try to create the name we simply have to check
whether there's still officially a gem handle around and if not refuse
to create the flink name. Since the handle count decrement and flink
name destruction is now also protected by that lock the reace is gone
and we can't ever leak the flink reference again.

Outside of the drm core only the exynos driver looks at the handle
count, and tbh I have no idea why (it's just for debug dmesg output
luckily).

I've considered inlining the drm_gem_object_handle_free, but I plan to
add more name-like things (like the exported dma_buf) to this scheme,
so it's clearer to leave the handle freeing in its own function.

This is exercised by the new gem_flink_race i-g-t testcase, which on
my snb leaks gem objects at a rate of roughly 1k objects/s.

v2: Fix up the error path handling in handle_create and make it more
robust by simply calling object_handle_unreference.

v3: Fix up the handle_unreference logic bug - atomic_dec_and_test
retursn 1 for 0. Oops.

v4: Squash in inlining of drm_gem_object_handle_reference as suggested
by Dave Airlie and add a note that we now have a testcase.

Cc: Dave Airlie <airlied@gmail.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c               | 31 ++++++++++++++++++++-----------
 drivers/gpu/drm/drm_info.c              |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_gem.c |  2 +-
 include/drm/drmP.h                      | 19 ++++++++++---------
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9118f5f..c972a8f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -154,7 +154,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
 	obj->filp = NULL;
 
 	kref_init(&obj->refcount);
-	atomic_set(&obj->handle_count, 0);
+	obj->handle_count = 0;
 	obj->size = size;
 }
 EXPORT_SYMBOL(drm_gem_private_object_init);
@@ -218,11 +218,9 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 	struct drm_device *dev = obj->dev;
 
 	/* Remove any name for this object */
-	spin_lock(&dev->object_name_lock);
 	if (obj->name) {
 		idr_remove(&dev->object_name_idr, obj->name);
 		obj->name = 0;
-		spin_unlock(&dev->object_name_lock);
 		/*
 		 * The object name held a reference to this object, drop
 		 * that now.
@@ -230,15 +228,13 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 		* This cannot be the last reference, since the handle holds one too.
 		 */
 		kref_put(&obj->refcount, drm_gem_object_ref_bug);
-	} else
-		spin_unlock(&dev->object_name_lock);
-
+	}
 }
 
 void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
-	if (WARN_ON(atomic_read(&obj->handle_count) == 0))
+	if (WARN_ON(obj->handle_count == 0))
 		return;
 
 	/*
@@ -247,8 +243,11 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 	* checked for a name
 	*/
 
-	if (atomic_dec_and_test(&obj->handle_count))
+	spin_lock(&obj->dev->object_name_lock);
+	if (--obj->handle_count == 0)
 		drm_gem_object_handle_free(obj);
+	spin_unlock(&obj->dev->object_name_lock);
+
 	drm_gem_object_unreference_unlocked(obj);
 }
 
@@ -326,17 +325,21 @@ drm_gem_handle_create(struct drm_file *file_priv,
 	 * allocation under our spinlock.
 	 */
 	idr_preload(GFP_KERNEL);
+	spin_lock(&dev->object_name_lock);
 	spin_lock(&file_priv->table_lock);
 
 	ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
-
+	drm_gem_object_reference(obj);
+	obj->handle_count++;
 	spin_unlock(&file_priv->table_lock);
+	spin_unlock(&dev->object_name_lock);
 	idr_preload_end();
-	if (ret < 0)
+	if (ret < 0) {
+		drm_gem_object_handle_unreference_unlocked(obj);
 		return ret;
+	}
 	*handlep = ret;
 
-	drm_gem_object_handle_reference(obj);
 
 	if (dev->driver->gem_open_object) {
 		ret = dev->driver->gem_open_object(obj, file_priv);
@@ -454,6 +457,12 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
 
 	idr_preload(GFP_KERNEL);
 	spin_lock(&dev->object_name_lock);
+	/* prevent races with concurrent gem_close. */
+	if (obj->handle_count == 0) {
+		ret = -ENOENT;
+		goto err;
+	}
+
 	if (!obj->name) {
 		ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT);
 		if (ret < 0)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index d4b20ce..f4b348c 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -207,7 +207,7 @@ static int drm_gem_one_name_info(int id, void *ptr, void *data)
 
 	seq_printf(m, "%6d %8zd %7d %8d\n",
 		   obj->name, obj->size,
-		   atomic_read(&obj->handle_count),
+		   obj->handle_count,
 		   atomic_read(&obj->refcount.refcount));
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index b904633..f3c6f40 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -136,7 +136,7 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj)
 	obj = &exynos_gem_obj->base;
 	buf = exynos_gem_obj->buffer;
 
-	DRM_DEBUG_KMS("handle count = %d\n", atomic_read(&obj->handle_count));
+	DRM_DEBUG_KMS("handle count = %d\n", obj->handle_count);
 
 	/*
 	 * do not release memory region from exporter.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 91c8d05..254f395 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -631,8 +631,16 @@ struct drm_gem_object {
 	/** Reference count of this object */
 	struct kref refcount;
 
-	/** Handle count of this object. Each handle also holds a reference */
-	atomic_t handle_count; /* number of handles on this object */
+	/**
+	 * handle_count - gem file_priv handle count of this object
+	 *
+	 * Each handle also holds a reference. Note that when the handle_count
+	 * drops to 0 any global names (e.g. the id in the flink namespace) will
+	 * be cleared.
+	 *
+	 * Protected by dev->object_name_lock.
+	 * */
+	unsigned handle_count;
 
 	/** Related drm device */
 	struct drm_device *dev;
@@ -1611,13 +1619,6 @@ int drm_gem_handle_create(struct drm_file *file_priv,
 			  u32 *handlep);
 int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
 
-static inline void
-drm_gem_object_handle_reference(struct drm_gem_object *obj)
-{
-	drm_gem_object_reference(obj);
-	atomic_inc(&obj->handle_count);
-}
-
 void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
 
 void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
-- 
1.8.3.2

[PATCH 09/20] drm/prime: fix error path in drm_gem_prime_fd_to_handle

[Daniel Vetter]

handle_unreference only clears up the obj->name and the reference,
but would leave a dangling handle in the idr. The right thing
to do is to call handle_delete.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index f115962..82cd83e 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -476,7 +476,7 @@ fail:
 	/* hmm, if driver attached, we are relying on the free-object path
 	 * to detach.. which seems ok..
 	 */
-	drm_gem_object_handle_unreference_unlocked(obj);
+	drm_gem_handle_delete(file_priv, *handle);
 out_put:
 	dma_buf_put(dma_buf);
 	mutex_unlock(&file_priv->prime.lock);
-- 
1.8.3.2

[PATCH 10/20] drm/gem: make drm_gem_object_handle_unreference_unlocked static

[Daniel Vetter]

No one outside of drm should use this, the official interfaces are
drm_gem_handle_create and drm_gem_handle_delete. The handle refcounting
is purely an implementation detail of gem.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 2 +-
 include/drm/drmP.h        | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index c972a8f..b554154 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -231,7 +231,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 	}
 }
 
-void
+static void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
 	if (WARN_ON(obj->handle_count == 0))
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 254f395..0eb4a37 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1619,7 +1619,6 @@ int drm_gem_handle_create(struct drm_file *file_priv,
 			  u32 *handlep);
 int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
 
-void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
 
 void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
 int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
-- 
1.8.3.2

[PATCH 11/20] drm/gem: create drm_gem_dumb_destroy

[Daniel Vetter]

All the gem based kms drivers really want the same function to
destroy a dumb framebuffer backing storage object.

So give it to them and roll it out in all drivers.

This still leaves the option open for kms drivers which don't use GEM
for backing storage, but it does decently simplify matters for gem
drivers.

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Ben Skeggs <skeggsb@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Clark <robdclark@gmail.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drmP.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0eb4a37..f08bbcf 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1619,6 +1619,9 @@ int drm_gem_handle_create(struct drm_file *file_priv,
 			  u32 *handlep);
 int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
 
+int drm_gem_dumb_destroy(struct drm_file *file,
+			 struct drm_device *dev,
+			 uint32_t handle);
 
 void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
 int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
-- 
1.8.3.2

[PATCH 12/20] drm/prime: use proper pointer in drm_gem_prime_handle_to_fd

[Daniel Vetter]

Part of the function uses the properly-typed dmabuf variable, the
other an untyped void *buf. Kill the later.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 82cd83e..c2d6d54 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -303,7 +303,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		int *prime_fd)
 {
 	struct drm_gem_object *obj;
-	void *buf;
 	int ret = 0;
 	struct dma_buf *dmabuf;
 
@@ -323,15 +322,15 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		goto out_have_obj;
 	}
 
-	buf = dev->driver->gem_prime_export(dev, obj, flags);
-	if (IS_ERR(buf)) {
+	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	if (IS_ERR(dmabuf)) {
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
 		 */
-		ret = PTR_ERR(buf);
+		ret = PTR_ERR(dmabuf);
 		goto out;
 	}
-	obj->export_dma_buf = buf;
+	obj->export_dma_buf = dmabuf;
 
 	/* if we've exported this buffer the cheat and add it to the import list
 	 * so we get the correct handle back
@@ -341,7 +340,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	if (ret)
 		goto fail_put_dmabuf;
 
-	ret = dma_buf_fd(buf, flags);
+	ret = dma_buf_fd(dmabuf, flags);
 	if (ret < 0)
 		goto fail_rm_handle;
 
@@ -362,11 +361,12 @@ out_have_obj:
 	goto out;
 
 fail_rm_handle:
-	drm_prime_remove_buf_handle_locked(&file_priv->prime, buf);
+	drm_prime_remove_buf_handle_locked(&file_priv->prime,
+					   dmabuf);
 fail_put_dmabuf:
 	/* clear NOT to be checked when releasing dma_buf */
 	obj->export_dma_buf = NULL;
-	dma_buf_put(buf);
+	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_unreference_unlocked(obj);
 	mutex_unlock(&file_priv->prime.lock);
-- 
1.8.3.2

[PATCH 13/20] drm/prime: shrink critical section protected by prime lock

[Daniel Vetter]

When exporting a gem object as a dma-buf the critical section for the
per-fd prime lock is just the adding (and in case of errors, removing)
of the handle to the per-fd lookup cache.

So restrict the critical section to just that part of the function.

This simplifies later reordering.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index c2d6d54..cb04516 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -310,7 +310,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	if (!obj)
 		return -ENOENT;
 
-	mutex_lock(&file_priv->prime.lock);
 	/* re-export the original imported object */
 	if (obj->import_attach) {
 		dmabuf = obj->import_attach->dmabuf;
@@ -332,6 +331,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	}
 	obj->export_dma_buf = dmabuf;
 
+	mutex_lock(&file_priv->prime.lock);
 	/* if we've exported this buffer the cheat and add it to the import list
 	 * so we get the correct handle back
 	 */
@@ -363,13 +363,13 @@ out_have_obj:
 fail_rm_handle:
 	drm_prime_remove_buf_handle_locked(&file_priv->prime,
 					   dmabuf);
+	mutex_unlock(&file_priv->prime.lock);
 fail_put_dmabuf:
 	/* clear NOT to be checked when releasing dma_buf */
 	obj->export_dma_buf = NULL;
 	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_unreference_unlocked(obj);
-	mutex_unlock(&file_priv->prime.lock);
 	return ret;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
-- 
1.8.3.2

[PATCH 14/20] drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle

[Daniel Vetter]

if (!ret) implies that ret == 0, so no need to clear it again. And
explicitly check for ret == 0 to indicate that we're checking an errno
integer.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index cb04516..3d57601 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -444,10 +444,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
 	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
 			dma_buf, handle);
-	if (!ret) {
-		ret = 0;
+	if (ret == 0)
 		goto out_put;
-	}
 
 	/* never seen this one, need to import */
 	obj = dev->driver->gem_prime_import(dev, dma_buf);
-- 
1.8.3.2

[PATCH 15/20] drm/gem: switch dev->object_name_lock to a mutex

[Daniel Vetter]

I want to wrap the creation of a dma-buf from a gem object in it,
so that the obj->export_dma_buf cache can be atomically filled in.

Instead of creating a new mutex just for that variable I've figured
I can reuse the existing dev->object_name_lock, especially since
the new semantics will exactly mirror the flink obj->name already
protected by that lock.

v2: idr_preload/idr_preload_end is now an atomic section, so need to
move the mutex locking outside.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 18 +++++++++---------
 include/drm/drmP.h        |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index b554154..c00e273 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -93,7 +93,7 @@ drm_gem_init(struct drm_device *dev)
 {
 	struct drm_gem_mm *mm;
 
-	spin_lock_init(&dev->object_name_lock);
+	mutex_init(&dev->object_name_lock);
 	idr_init(&dev->object_name_idr);
 
 	mm = kzalloc(sizeof(struct drm_gem_mm), GFP_KERNEL);
@@ -243,10 +243,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 	* checked for a name
 	*/
 
-	spin_lock(&obj->dev->object_name_lock);
+	mutex_lock(&obj->dev->object_name_lock);
 	if (--obj->handle_count == 0)
 		drm_gem_object_handle_free(obj);
-	spin_unlock(&obj->dev->object_name_lock);
+	mutex_unlock(&obj->dev->object_name_lock);
 
 	drm_gem_object_unreference_unlocked(obj);
 }
@@ -324,16 +324,16 @@ drm_gem_handle_create(struct drm_file *file_priv,
 	 * Get the user-visible handle using idr.  Preload and perform
 	 * allocation under our spinlock.
 	 */
+	mutex_lock(&dev->object_name_lock);
 	idr_preload(GFP_KERNEL);
-	spin_lock(&dev->object_name_lock);
 	spin_lock(&file_priv->table_lock);
 
 	ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
 	drm_gem_object_reference(obj);
 	obj->handle_count++;
 	spin_unlock(&file_priv->table_lock);
-	spin_unlock(&dev->object_name_lock);
 	idr_preload_end();
+	mutex_unlock(&dev->object_name_lock);
 	if (ret < 0) {
 		drm_gem_object_handle_unreference_unlocked(obj);
 		return ret;
@@ -455,8 +455,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
 	if (obj == NULL)
 		return -ENOENT;
 
+	mutex_lock(&dev->object_name_lock);
 	idr_preload(GFP_KERNEL);
-	spin_lock(&dev->object_name_lock);
 	/* prevent races with concurrent gem_close. */
 	if (obj->handle_count == 0) {
 		ret = -ENOENT;
@@ -478,8 +478,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
 	ret = 0;
 
 err:
-	spin_unlock(&dev->object_name_lock);
 	idr_preload_end();
+	mutex_unlock(&dev->object_name_lock);
 	drm_gem_object_unreference_unlocked(obj);
 	return ret;
 }
@@ -502,11 +502,11 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
 	if (!(dev->driver->driver_features & DRIVER_GEM))
 		return -ENODEV;
 
-	spin_lock(&dev->object_name_lock);
+	mutex_lock(&dev->object_name_lock);
 	obj = idr_find(&dev->object_name_idr, (int) args->name);
 	if (obj)
 		drm_gem_object_reference(obj);
-	spin_unlock(&dev->object_name_lock);
+	mutex_unlock(&dev->object_name_lock);
 	if (!obj)
 		return -ENOENT;
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f08bbcf..49e514b 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1218,7 +1218,7 @@ struct drm_device {
 
 	/** \name GEM information */
 	/*@{ */
-	spinlock_t object_name_lock;
+	struct mutex object_name_lock;
 	struct idr object_name_idr;
 	/*@} */
 	int switch_power_state;
-- 
1.8.3.2

[PATCH 16/20] drm/gem: completely close gem_open vs. gem_close races

[Daniel Vetter]

The gem flink name holds a reference onto the object itself, and this
self-reference would prevent an flink'ed object from every being
freed. To break that loop we remove the flink name when the last
userspace handle disappears, i.e. when obj->handle_count reaches 0.

Now in gem_open we drop the dev->object_name_lock between the flink
name lookup and actually adding the handle. This means a concurrent
gem_close of the last handle could result in the flink name getting
reaped right inbetween, i.e.

Thread 1		Thread 2
gem_open		gem_close

flink -> obj lookup
			handle_count drops to 0
			remove flink name
create_handle
handle_count++

If someone now flinks this object again, we'll get a new flink name.

We can close this race by removing the lock dropping and making the
entire lookup+handle_create sequence atomic. Unfortunately to still be
able to share the handle_create logic this requires a
handle_create_tail function which drops the lock - we can't hold the
object_name_lock while calling into a driver's ->gem_open callback.

Note that for flink fixing this race isn't really important, since
racing gem_open against gem_close is clearly a userspace bug. And no
matter how the race ends, we won't leak any references.

But with dma-buf where the userspace dma-buf fd itself is refcounted
this is a valid sequence and hence we should fix it. Therefore this
patch here is just a warm-up exercise (and for consistency between
flink buffer sharing and dma-buf buffer sharing with self-imports).

Also note that this extension of the critical section in gem_open
protected by dev->object_name_lock only works because it's now a
mutex: A spinlock would conflict with the potential memory allocation
in idr_preload().

This is exercises by igt/gem_flink_race/flink_name.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 42 +++++++++++++++++++++++++++++++-----------
 include/drm/drmP.h        |  3 +++
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index c00e273..0e9a9c7 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -308,23 +308,26 @@ int drm_gem_dumb_destroy(struct drm_file *file,
 EXPORT_SYMBOL(drm_gem_dumb_destroy);
 
 /**
- * Create a handle for this object. This adds a handle reference
- * to the object, which includes a regular reference count. Callers
- * will likely want to dereference the object afterwards.
+ * drm_gem_handle_create_tail - internal functions to create a handle
+ * 
+ * This expects the dev->object_name_lock to be held already and will drop it
+ * before returning. Used to avoid races in establishing new handles when
+ * importing an object from either an flink name or a dma-buf.
  */
 int
-drm_gem_handle_create(struct drm_file *file_priv,
-		       struct drm_gem_object *obj,
-		       u32 *handlep)
+drm_gem_handle_create_tail(struct drm_file *file_priv,
+			   struct drm_gem_object *obj,
+			   u32 *handlep)
 {
 	struct drm_device *dev = obj->dev;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->object_name_lock));
+
 	/*
 	 * Get the user-visible handle using idr.  Preload and perform
 	 * allocation under our spinlock.
 	 */
-	mutex_lock(&dev->object_name_lock);
 	idr_preload(GFP_KERNEL);
 	spin_lock(&file_priv->table_lock);
 
@@ -351,6 +354,21 @@ drm_gem_handle_create(struct drm_file *file_priv,
 
 	return 0;
 }
+
+/**
+ * Create a handle for this object. This adds a handle reference
+ * to the object, which includes a regular reference count. Callers
+ * will likely want to dereference the object afterwards.
+ */
+int
+drm_gem_handle_create(struct drm_file *file_priv,
+		       struct drm_gem_object *obj,
+		       u32 *handlep)
+{
+	mutex_lock(&obj->dev->object_name_lock);
+
+	return drm_gem_handle_create_tail(file_priv, obj, handlep);
+}
 EXPORT_SYMBOL(drm_gem_handle_create);
 
 
@@ -504,13 +522,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
 
 	mutex_lock(&dev->object_name_lock);
 	obj = idr_find(&dev->object_name_idr, (int) args->name);
-	if (obj)
+	if (obj) {
 		drm_gem_object_reference(obj);
-	mutex_unlock(&dev->object_name_lock);
-	if (!obj)
+	} else {
+		mutex_unlock(&dev->object_name_lock);
 		return -ENOENT;
+	}
 
-	ret = drm_gem_handle_create(file_priv, obj, &handle);
+	/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */
+	ret = drm_gem_handle_create_tail(file_priv, obj, &handle);
 	drm_gem_object_unreference_unlocked(obj);
 	if (ret)
 		return ret;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 49e514b..3b346bc 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1614,6 +1614,9 @@ drm_gem_object_unreference_unlocked(struct drm_gem_object *obj)
 	}
 }
 
+int drm_gem_handle_create_tail(struct drm_file *file_priv,
+			       struct drm_gem_object *obj,
+			       u32 *handlep);
 int drm_gem_handle_create(struct drm_file *file_priv,
 			  struct drm_gem_object *obj,
 			  u32 *handlep);
-- 
1.8.3.2

[PATCH 17/20] drm/prime: proper locking+refcounting for obj->dma_buf link

[Daniel Vetter]

The export dma-buf cache is semantically similar to an flink name. So
semantically it makes sense to treat it the same and remove the name
(i.e. the dma_buf pointer) and its references when the last gem handle
disappears.

Again we need to be careful, but double so: Not just could someone
race and export with a gem close ioctl (so we need to recheck
obj->handle_count again when assigning the new name), but multiple
exports can also race against each another. This is prevented by
holding the dev->object_name_lock across the entire section which
touches obj->dma_buf.

With the new scheme we also need to reinstate the obj->dma_buf link at
import time (in case the only reference userspace has held in-between
was through the dma-buf fd and not through any native gem handle). For
simplicity we don't check whether it's a native object but
unconditionally set up that link - with the new scheme of removing the
obj->dma_buf reference when the last handle disappears we can do that.

To make it clear that this is not just for exported buffers anymore
als rename it from export_dma_buf to dma_buf.

To make sure that now one can race a fd_to_handle or handle_to_fd with
gem_close we use the same tricks as in flink of extending the
dev->object_name_locking critical section. With this change we finally
have a guaranteed 1:1 relationship (at least for native objects)
between gem objects and dma-bufs, even accounting for races (which can
happen since the dma-buf itself holds a reference while in-flight).

This prevent igt/prime_self_import/export-vs-gem_close-race from
Oopsing the kernel. There is still a leak though since the per-file
priv dma-buf/handle cache handling is racy. That will be fixed in a
later patch.

v2: Remove the bogus dma_buf_put from the export_and_register_object
failure path if we've raced with the handle count dropping to 0.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fops.c  |  1 +
 drivers/gpu/drm/drm_gem.c   | 24 ++++++++++++++--
 drivers/gpu/drm/drm_prime.c | 70 +++++++++++++++++++++++++++++++++++----------
 include/drm/drmP.h          | 12 ++++++--
 4 files changed, 87 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 72acae9..faf93ff3 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -545,6 +545,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	if (dev->driver->postclose)
 		dev->driver->postclose(dev, file_priv);
 
+
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_destroy_file_private(&file_priv->prime);
 
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 0e9a9c7..6b2b54e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -195,9 +195,14 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 		drm_prime_remove_buf_handle(&filp->prime,
 				obj->import_attach->dmabuf);
 	}
-	if (obj->export_dma_buf) {
+
+	/*
+	 * Note: obj->dma_buf can't disappear as long as we still hold a
+	 * handle reference in obj->handle_count.
+	 */
+	if (obj->dma_buf) {
 		drm_prime_remove_buf_handle(&filp->prime,
-				obj->export_dma_buf);
+				obj->dma_buf);
 	}
 }
 
@@ -231,6 +236,15 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
 	}
 }
 
+static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
+{
+	/* Unbreak the reference cycle if we have an exported dma_buf. */
+	if (obj->dma_buf) {
+		dma_buf_put(obj->dma_buf);
+		obj->dma_buf = NULL;
+	}
+}
+
 static void
 drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 {
@@ -244,8 +258,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 	*/
 
 	mutex_lock(&obj->dev->object_name_lock);
-	if (--obj->handle_count == 0)
+	if (--obj->handle_count == 0) {
 		drm_gem_object_handle_free(obj);
+		drm_gem_object_exported_dma_buf_free(obj);
+	}
 	mutex_unlock(&obj->dev->object_name_lock);
 
 	drm_gem_object_unreference_unlocked(obj);
@@ -589,6 +605,8 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 void
 drm_gem_object_release(struct drm_gem_object *obj)
 {
+	WARN_ON(obj->dma_buf);
+
 	if (obj->filp)
 	    fput(obj->filp);
 }
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 3d57601..5e543e9 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -193,11 +193,8 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	if (obj->export_dma_buf == dma_buf) {
-		/* drop the reference on the export fd holds */
-		obj->export_dma_buf = NULL;
-		drm_gem_object_unreference_unlocked(obj);
-	}
+	/* drop the reference on the export fd holds */
+	drm_gem_object_unreference_unlocked(obj);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_release);
 
@@ -298,6 +295,37 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_gem_prime_export);
 
+static struct dma_buf *export_and_register_object(struct drm_device *dev,
+						  struct drm_gem_object *obj,
+						  uint32_t flags)
+{
+	struct dma_buf *dmabuf;
+
+	/* prevent races with concurrent gem_close. */
+	if (obj->handle_count == 0) {
+		dmabuf = ERR_PTR(-ENOENT);
+		return dmabuf;
+	}
+
+	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	if (IS_ERR(dmabuf)) {
+		/* normally the created dma-buf takes ownership of the ref,
+		 * but if that fails then drop the ref
+		 */
+		return dmabuf;
+	}
+
+	/*
+	 * Note that callers do not need to clean up the export cache
+	 * since the check for obj->handle_count guarantees that someone
+	 * will clean it up.
+	 */
+	obj->dma_buf = dmabuf;
+	get_dma_buf(obj->dma_buf);
+
+	return dmabuf;
+}
+
 int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		struct drm_file *file_priv, uint32_t handle, uint32_t flags,
 		int *prime_fd)
@@ -313,15 +341,20 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	/* re-export the original imported object */
 	if (obj->import_attach) {
 		dmabuf = obj->import_attach->dmabuf;
+		get_dma_buf(dmabuf);
 		goto out_have_obj;
 	}
 
-	if (obj->export_dma_buf) {
-		dmabuf = obj->export_dma_buf;
+	mutex_lock(&dev->object_name_lock);
+	if (obj->dma_buf) {
+		get_dma_buf(obj->dma_buf);
+		dmabuf = obj->dma_buf;
+		mutex_unlock(&dev->object_name_lock);
 		goto out_have_obj;
 	}
 
-	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	dmabuf = export_and_register_object(dev, obj, flags);
+	mutex_unlock(&dev->object_name_lock);
 	if (IS_ERR(dmabuf)) {
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
@@ -329,14 +362,13 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		ret = PTR_ERR(dmabuf);
 		goto out;
 	}
-	obj->export_dma_buf = dmabuf;
 
 	mutex_lock(&file_priv->prime.lock);
 	/* if we've exported this buffer the cheat and add it to the import list
 	 * so we get the correct handle back
 	 */
 	ret = drm_prime_add_buf_handle(&file_priv->prime,
-				       obj->export_dma_buf, handle);
+				       dmabuf, handle);
 	if (ret)
 		goto fail_put_dmabuf;
 
@@ -349,7 +381,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	return 0;
 
 out_have_obj:
-	get_dma_buf(dmabuf);
 	ret = dma_buf_fd(dmabuf, flags);
 	if (ret < 0) {
 		dma_buf_put(dmabuf);
@@ -365,8 +396,6 @@ fail_rm_handle:
 					   dmabuf);
 	mutex_unlock(&file_priv->prime.lock);
 fail_put_dmabuf:
-	/* clear NOT to be checked when releasing dma_buf */
-	obj->export_dma_buf = NULL;
 	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_unreference_unlocked(obj);
@@ -448,13 +477,22 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		goto out_put;
 
 	/* never seen this one, need to import */
+	mutex_lock(&dev->object_name_lock);
 	obj = dev->driver->gem_prime_import(dev, dma_buf);
 	if (IS_ERR(obj)) {
 		ret = PTR_ERR(obj);
-		goto out_put;
+		goto out_unlock;
+	}
+
+	if (obj->dma_buf) {
+		WARN_ON(obj->dma_buf != dma_buf);
+	} else {
+		obj->dma_buf = dma_buf;
+		get_dma_buf(dma_buf);
 	}
 
-	ret = drm_gem_handle_create(file_priv, obj, handle);
+	/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */
+	ret = drm_gem_handle_create_tail(file_priv, obj, handle);
 	drm_gem_object_unreference_unlocked(obj);
 	if (ret)
 		goto out_put;
@@ -475,6 +513,8 @@ fail:
 	 * to detach.. which seems ok..
 	 */
 	drm_gem_handle_delete(file_priv, *handle);
+out_unlock:
+	mutex_lock(&dev->object_name_lock);
 out_put:
 	dma_buf_put(dma_buf);
 	mutex_unlock(&file_priv->prime.lock);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3b346bc..1db42c0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -683,8 +683,16 @@ struct drm_gem_object {
 
 	void *driver_private;
 
-	/* dma buf exported from this GEM object */
-	struct dma_buf *export_dma_buf;
+	/**
+	 * dma_buf - dma buf associated with this GEM object
+	 *
+	 * Pointer to the dma-buf associated with this gem object (either
+	 * through importing or exporting). We break the resulting reference
+	 * loop when the last gem handle for this object is released.
+	 *
+	 * Protected by obj->object_name_lock
+	 */
+	struct dma_buf *dma_buf;
 
 	/**
 	 * import_attach - dma buf attachment backing this object
-- 
1.8.3.2

[PATCH 18/20] drm/prime: Simplify drm_gem_remove_prime_handles

[Daniel Vetter]

with the reworking semantics and locking of the obj->dma_buf pointer
this pointer is always set as long as there's still a gem handle
around and a dma_buf associated with this gem object.

Also, the per file-priv lookup-cache for dma-buf importing is also
unified between foreign and native objects.

Hence we don't need to special case the clean any more and can simply
drop the clause which only runs for foreing objects, i.e. with
obj->import_attach set.

Note that with this change (actually with the previous one to always
set up obj->dma_buf even for foreign objects) it is no longer required
to set obj->import_attach when importing a foreing object. So update
comments accordingly, too.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c | 5 -----
 include/drm/drmP.h        | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6b2b54e..9d72028 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -191,11 +191,6 @@ EXPORT_SYMBOL(drm_gem_object_alloc);
 static void
 drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 {
-	if (obj->import_attach) {
-		drm_prime_remove_buf_handle(&filp->prime,
-				obj->import_attach->dmabuf);
-	}
-
 	/*
 	 * Note: obj->dma_buf can't disappear as long as we still hold a
 	 * handle reference in obj->handle_count.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 1db42c0..0c8a627 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -703,6 +703,11 @@ struct drm_gem_object {
 	 *
 	 * The driver's ->gem_free_object callback is responsible for cleaning
 	 * up the dma_buf attachment and references acquired at import time.
+	 *
+	 * Note that the drm gem/prime core does not depend upon drivers setting
+	 * this field any more. So for drivers where this doesn't make sense
+	 * (e.g. virtual devices or a displaylink behind an usb bus) they can
+	 * simply leave it as NULL.
 	 */
 	struct dma_buf_attachment *import_attach;
 };
-- 
1.8.3.2

[PATCH 19/20] drm/prime: make drm_prime_lookup_buf_handle static

[Daniel Vetter]

... and move it to the top of the function to avoid a forward
declaration.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_prime.c | 29 +++++++++++++++--------------
 include/drm/drmP.h          |  1 -
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 5e543e9..ed1ea5c 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -83,6 +83,21 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
 	return 0;
 }
 
+static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv,
+				       struct dma_buf *dma_buf,
+				       uint32_t *handle)
+{
+	struct drm_prime_member *member;
+
+	list_for_each_entry(member, &prime_fpriv->head, entry) {
+		if (member->dma_buf == dma_buf) {
+			*handle = member->handle;
+			return 0;
+		}
+	}
+	return -ENOENT;
+}
+
 static int drm_gem_map_attach(struct dma_buf *dma_buf,
 			      struct device *target_dev,
 			      struct dma_buf_attachment *attach)
@@ -655,20 +670,6 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 }
 EXPORT_SYMBOL(drm_prime_destroy_file_private);
 
-int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
-{
-	struct drm_prime_member *member;
-
-	list_for_each_entry(member, &prime_fpriv->head, entry) {
-		if (member->dma_buf == dma_buf) {
-			*handle = member->handle;
-			return 0;
-		}
-	}
-	return -ENOENT;
-}
-EXPORT_SYMBOL(drm_prime_lookup_buf_handle);
-
 void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
 {
 	mutex_lock(&prime_fpriv->lock);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0c8a627..8047f76 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1543,7 +1543,6 @@ int drm_gem_dumb_destroy(struct drm_file *file,
 
 void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);
 void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);
-int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle);
 void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
 
 int drm_prime_add_dma_buf(struct drm_device *dev, struct drm_gem_object *obj);
-- 
1.8.3.2

[PATCH 20/20] drm/prime: Always add exported buffers to the handle cache

[Daniel Vetter]

... not only when the dma-buf is freshly created. In contrived
examples someone else could have exported/imported the dma-buf already
and handed us the gem object with a flink name. If such on object gets
reexported as a dma_buf we won't have it in the handle cache already,
which breaks the guarantee that for dma-buf imports we always hand
back an existing handle if there is one.

This is exercised by igt/prime_self_import/with_one_bo_two_files

Now if we extend the locked sections just a notch more we can also
plug th racy buf/handle cache setup in handle_to_fd:

If evil userspace races a concurrent gem close against a prime export
operation we can end up tearing down the gem handle before the dma buf
handle cache is set up. When handle_to_fd gets around to adding the
handle to the cache there will be no one left to clean it up,
effectily leaking the bo (and the dma-buf, since the handle cache
holds a ref on the dma-buf):

Thread A			Thread B

handle_to_fd:

lookup gem object from handle
creates new dma_buf

				gem_close on the same handle
				obj->dma_buf is set, but file priv buf
				handle cache has no entry

				obj->handle_count drops to 0

drm_prime_add_buf_handle sets up the handle cache

-> We have a dma-buf reference in the handle cache, but since the
handle_count of the gem object already dropped to 0 no on will clean
it up. When closing the drm device fd we'll hit the WARN_ON in
drm_prime_destroy_file_private.

The important change is to extend the critical section of the
filp->prime.lock to cover the gem handle lookup. This serializes with
a concurrent gem handle close.

This leak is exercised by igt/prime_self_import/export-vs-gem_close-race

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem.c   |  6 ++--
 drivers/gpu/drm/drm_prime.c | 81 +++++++++++++++++++++++++++------------------
 include/drm/drmP.h          |  2 +-
 3 files changed, 53 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9d72028..a3654fe 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -195,10 +195,12 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 	 * Note: obj->dma_buf can't disappear as long as we still hold a
 	 * handle reference in obj->handle_count.
 	 */
+	mutex_lock(&filp->prime.lock);
 	if (obj->dma_buf) {
-		drm_prime_remove_buf_handle(&filp->prime,
-				obj->dma_buf);
+		drm_prime_remove_buf_handle_locked(&filp->prime,
+						   obj->dma_buf);
 	}
+	mutex_unlock(&filp->prime.lock);
 }
 
 static void drm_gem_object_ref_bug(struct kref *list_kref)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index ed1ea5c..7ae2bfc 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -83,6 +83,19 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
 	return 0;
 }
 
+static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv,
+						      uint32_t handle)
+{
+	struct drm_prime_member *member;
+
+	list_for_each_entry(member, &prime_fpriv->head, entry) {
+		if (member->handle == handle)
+			return member->dma_buf;
+	}
+
+	return NULL;
+}
+
 static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv,
 				       struct dma_buf *dma_buf,
 				       uint32_t *handle)
@@ -146,9 +159,8 @@ static void drm_gem_map_detach(struct dma_buf *dma_buf,
 	attach->priv = NULL;
 }
 
-static void drm_prime_remove_buf_handle_locked(
-		struct drm_prime_file_private *prime_fpriv,
-		struct dma_buf *dma_buf)
+void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv,
+					struct dma_buf *dma_buf)
 {
 	struct drm_prime_member *member, *safe;
 
@@ -337,6 +349,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 	 */
 	obj->dma_buf = dmabuf;
 	get_dma_buf(obj->dma_buf);
+	/* Grab a new ref since the callers is now used by the dma-buf */
+	drm_gem_object_reference(obj);
 
 	return dmabuf;
 }
@@ -349,10 +363,20 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	int ret = 0;
 	struct dma_buf *dmabuf;
 
+	mutex_lock(&file_priv->prime.lock);
 	obj = drm_gem_object_lookup(dev, file_priv, handle);
-	if (!obj)
-		return -ENOENT;
+	if (!obj)  {
+		ret = -ENOENT;
+		goto out_unlock;
+	}
+
+	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
+	if (dmabuf) {
+		get_dma_buf(dmabuf);
+		goto out_have_handle;
+	}
 
+	mutex_lock(&dev->object_name_lock);
 	/* re-export the original imported object */
 	if (obj->import_attach) {
 		dmabuf = obj->import_attach->dmabuf;
@@ -360,45 +384,45 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		goto out_have_obj;
 	}
 
-	mutex_lock(&dev->object_name_lock);
 	if (obj->dma_buf) {
 		get_dma_buf(obj->dma_buf);
 		dmabuf = obj->dma_buf;
-		mutex_unlock(&dev->object_name_lock);
 		goto out_have_obj;
 	}
 
 	dmabuf = export_and_register_object(dev, obj, flags);
-	mutex_unlock(&dev->object_name_lock);
 	if (IS_ERR(dmabuf)) {
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
 		 */
 		ret = PTR_ERR(dmabuf);
+		mutex_unlock(&dev->object_name_lock);
 		goto out;
 	}
 
-	mutex_lock(&file_priv->prime.lock);
-	/* if we've exported this buffer the cheat and add it to the import list
-	 * so we get the correct handle back
+out_have_obj:
+	/*
+	 * If we've exported this buffer then cheat and add it to the import list
+	 * so we get the correct handle back. We must do this under the
+	 * protection of dev->object_name_lock to ensure that a racing gem close
+	 * ioctl doesn't miss to remove this buffer handle from the cache.
 	 */
 	ret = drm_prime_add_buf_handle(&file_priv->prime,
 				       dmabuf, handle);
+	mutex_unlock(&dev->object_name_lock);
 	if (ret)
 		goto fail_put_dmabuf;
 
+out_have_handle:
 	ret = dma_buf_fd(dmabuf, flags);
-	if (ret < 0)
-		goto fail_rm_handle;
-
-	*prime_fd = ret;
-	mutex_unlock(&file_priv->prime.lock);
-	return 0;
-
-out_have_obj:
-	ret = dma_buf_fd(dmabuf, flags);
+	/*
+	 * We must _not_ remove the buffer from the handle cache since the newly
+	 * created dma buf is already linked in the global obj->dma_buf pointer,
+	 * and that is invariant as long as a userspace gem handle exists.
+	 * Closing the handle will clean out the cache anyway, so we don't leak.
+	 */
 	if (ret < 0) {
-		dma_buf_put(dmabuf);
+		goto fail_put_dmabuf;
 	} else {
 		*prime_fd = ret;
 		ret = 0;
@@ -406,14 +430,13 @@ out_have_obj:
 
 	goto out;
 
-fail_rm_handle:
-	drm_prime_remove_buf_handle_locked(&file_priv->prime,
-					   dmabuf);
-	mutex_unlock(&file_priv->prime.lock);
 fail_put_dmabuf:
 	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_unreference_unlocked(obj);
+out_unlock:
+	mutex_unlock(&file_priv->prime.lock);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
@@ -669,11 +692,3 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 	WARN_ON(!list_empty(&prime_fpriv->head));
 }
 EXPORT_SYMBOL(drm_prime_destroy_file_private);
-
-void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
-{
-	mutex_lock(&prime_fpriv->lock);
-	drm_prime_remove_buf_handle_locked(prime_fpriv, dma_buf);
-	mutex_unlock(&prime_fpriv->lock);
-}
-EXPORT_SYMBOL(drm_prime_remove_buf_handle);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 8047f76..417aa89 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1543,7 +1543,7 @@ int drm_gem_dumb_destroy(struct drm_file *file,
 
 void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);
 void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);
-void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
+void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
 
 int drm_prime_add_dma_buf(struct drm_device *dev, struct drm_gem_object *obj);
 int drm_prime_lookup_obj(struct drm_device *dev, struct dma_buf *buf,
-- 
1.8.3.2

Re: [PATCH 11/20] drm/gem: create drm_gem_dumb_destroy

[Daniel Vetter]

On Thu, Aug 15, 2013 at 9:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Aug 15, 2013 at 12:02:40AM +0200, Daniel Vetter wrote:
>> All the gem based kms drivers really want the same function to
>> destroy a dumb framebuffer backing storage object.
>>
>> So give it to them and roll it out in all drivers.
>>
>> This still leaves the option open for kms drivers which don't use GEM
>> for backing storage, but it does decently simplify matters for gem
>> drivers.
>
> You only add a header here, is there a chunk missing from the code or
> was it exported already and just not declared?

The patch is merged already, but somehow rebase thought that chunk is
missing. This patch can be savely ignored ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch