[PATCH 1/2] drm/prime: keep a reference from the handle to exported dma-buf (v5)

[PATCH 1/2] drm/prime: keep a reference from the handle to exported dma-buf (v5)

[Dave Airlie]

Currently we have a problem with this:
1. i915: create gem object
2. i915: export gem object to prime
3. radeon: import gem object
4. close prime fd
5. radeon: unref object
6. i915: unref object

i915 has an imported object reference in its file priv, that isn't
cleaned up properly until fd close. The reference gets added at step 2,
but at step 6 we don't have enough info to clean it up.

The solution is to take a reference on the dma-buf when we export it,
and drop the reference when the gem handle goes away.

So when we export a dma_buf from a gem object, we keep track of it
with the handle, we take a reference to the dma_buf. When we close
the handle (i.e. userspace is finished with the buffer), we drop
the reference to the dma_buf, and it gets collected.

This patch isn't meant to fix any other problem or bikesheds, and it doesn't
fix any races with other scenarios.

v1.1: move export symbol line back up.

v2: okay I had to do a bit more, as the first patch showed a leak
on one of my tests, that I found using the dma-buf debugfs support,
the problem case is exporting a buffer twice with the same handle,
we'd add another export handle for it unnecessarily, however
we now fail if we try to export the same object with a different gem handle,
however I'm not sure if that is a case I want to support, and I've
gotten the code to WARN_ON if we hit something like that.

v2.1: rebase this patch, write better commit msg.
v3: cleanup error handling, track import vs export in linked list,
these two patches were separate previously, but seem to work better
like this.
v4: danvet is correct, this code is no longer useful, since the buffer
better exist, so remove it.
v5: always take a reference to the dma buf object, import or export.
(Imre Deak contributed this originally)

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_gem.c   |  4 +-
 drivers/gpu/drm/drm_prime.c | 93 ++++++++++++++++++++++++++++-----------------
 include/drm/drmP.h          |  4 +-
 3 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index af779ae..cf919e3 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -205,11 +205,11 @@ static void
 drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 {
 	if (obj->import_attach) {
-		drm_prime_remove_imported_buf_handle(&filp->prime,
+		drm_prime_remove_buf_handle(&filp->prime,
 				obj->import_attach->dmabuf);
 	}
 	if (obj->export_dma_buf) {
-		drm_prime_remove_imported_buf_handle(&filp->prime,
+		drm_prime_remove_buf_handle(&filp->prime,
 				obj->export_dma_buf);
 	}
 }
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 366910d..eb7c38d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -57,10 +57,14 @@
  * use the drm_gem_prime_{import,export} helpers.
  */
 
+#define PRIME_IMPORT 1
+#define PRIME_EXPORT 2
+
 struct drm_prime_member {
 	struct list_head entry;
 	struct dma_buf *dma_buf;
 	uint32_t handle;
+	int type;
 };
 
 static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
@@ -157,6 +161,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
 	.vunmap = drm_gem_dmabuf_vunmap,
 };
 
+static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
+
 /**
  * DOC: PRIME Helpers
  *
@@ -200,7 +206,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 {
 	struct drm_gem_object *obj;
 	void *buf;
-	int ret;
+	int ret = 0;
+	struct dma_buf *dmabuf;
 
 	obj = drm_gem_object_lookup(dev, file_priv, handle);
 	if (!obj)
@@ -209,43 +216,44 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	mutex_lock(&file_priv->prime.lock);
 	/* re-export the original imported object */
 	if (obj->import_attach) {
-		get_dma_buf(obj->import_attach->dmabuf);
-		*prime_fd = dma_buf_fd(obj->import_attach->dmabuf, flags);
-		drm_gem_object_unreference_unlocked(obj);
-		mutex_unlock(&file_priv->prime.lock);
-		return 0;
+		dmabuf = obj->import_attach->dmabuf;
+		goto out_have_obj;
 	}
 
 	if (obj->export_dma_buf) {
-		get_dma_buf(obj->export_dma_buf);
-		*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
-		drm_gem_object_unreference_unlocked(obj);
-	} else {
-		buf = dev->driver->gem_prime_export(dev, obj, flags);
-		if (IS_ERR(buf)) {
-			/* normally the created dma-buf takes ownership of the ref,
-			 * but if that fails then drop the ref
-			 */
-			drm_gem_object_unreference_unlocked(obj);
-			mutex_unlock(&file_priv->prime.lock);
-			return PTR_ERR(buf);
-		}
-		obj->export_dma_buf = buf;
-		*prime_fd = dma_buf_fd(buf, flags);
+		dmabuf = obj->export_dma_buf;
+		goto out_have_obj;
 	}
+
+	buf = dev->driver->gem_prime_export(dev, obj, flags);
+	if (IS_ERR(buf)) {
+		/* normally the created dma-buf takes ownership of the ref,
+		 * but if that fails then drop the ref
+		 */
+		ret = PTR_ERR(buf);
+		goto out;
+	}
+	obj->export_dma_buf = buf;
+
 	/* 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_imported_buf_handle(&file_priv->prime,
-			obj->export_dma_buf, handle);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(obj);
-		mutex_unlock(&file_priv->prime.lock);
-		return ret;
-	}
+	ret = drm_prime_add_exported_buf_handle(&file_priv->prime,
+						obj->export_dma_buf, handle);
+	if (ret)
+		goto out;
 
+	*prime_fd = dma_buf_fd(buf, flags);
 	mutex_unlock(&file_priv->prime.lock);
 	return 0;
+
+out_have_obj:
+	get_dma_buf(dmabuf);
+	*prime_fd = dma_buf_fd(dmabuf, flags);
+out:
+	drm_gem_object_unreference_unlocked(obj);
+	mutex_unlock(&file_priv->prime.lock);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
@@ -314,7 +322,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
 	mutex_lock(&file_priv->prime.lock);
 
-	ret = drm_prime_lookup_imported_buf_handle(&file_priv->prime,
+	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
 			dma_buf, handle);
 	if (!ret) {
 		ret = 0;
@@ -491,7 +499,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 }
 EXPORT_SYMBOL(drm_prime_destroy_file_private);
 
-int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
+static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle, int type)
 {
 	struct drm_prime_member *member;
 
@@ -499,14 +507,27 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
 	if (!member)
 		return -ENOMEM;
 
+	get_dma_buf(dma_buf);
 	member->dma_buf = dma_buf;
 	member->handle = handle;
+	member->type = type;
 	list_add(&member->entry, &prime_fpriv->head);
 	return 0;
 }
+
+int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
+{
+	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle, PRIME_IMPORT);
+}
 EXPORT_SYMBOL(drm_prime_add_imported_buf_handle);
 
-int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
+static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
+{
+	/* take a reference to the buf handle for this case */
+	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle, PRIME_EXPORT);
+}
+
+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;
 
@@ -518,19 +539,23 @@ int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fp
 	}
 	return -ENOENT;
 }
-EXPORT_SYMBOL(drm_prime_lookup_imported_buf_handle);
+EXPORT_SYMBOL(drm_prime_lookup_buf_handle);
 
-void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
+void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
 {
 	struct drm_prime_member *member, *safe;
 
 	mutex_lock(&prime_fpriv->lock);
 	list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) {
 		if (member->dma_buf == dma_buf) {
+			dma_buf_put(dma_buf);
 			list_del(&member->entry);
 			kfree(member);
 		}
 	}
 	mutex_unlock(&prime_fpriv->lock);
 }
-EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
+EXPORT_SYMBOL(drm_prime_remove_buf_handle);
+
+
+
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2d94d74..69712a6 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1594,8 +1594,8 @@ extern void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *s
 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_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
-int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle);
-void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
+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);
 int drm_prime_lookup_obj(struct drm_device *dev, struct dma_buf *buf,
-- 
1.8.2

[PATCH 2/2] drm: prime: fix refcounting on the dmabuf import error path

[Dave Airlie]

From: Imre Deak <imre.deak@intel.com>

In commit be8a42ae60 we inroduced a refcount problem, where on the
drm_gem_prime_fd_to_handle() error path we'll call dma_buf_put() for
self imported dma buffers.

Fix this by taking a reference on the dma buffer in the .gem_import
hook instead of assuming the caller had taken one. Besides fixing the
bug this is also more logical.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
---
 drivers/gpu/drm/drm_prime.c                | 8 +++++++-
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 4 +++-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     | 5 ++++-
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c  | 1 -
 drivers/gpu/drm/udl/udl_gem.c              | 4 ++++
 5 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index eb7c38d..71f02ff 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -276,7 +276,6 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 			 * refcount on gem itself instead of f_count of dmabuf.
 			 */
 			drm_gem_object_reference(obj);
-			dma_buf_put(dma_buf);
 			return obj;
 		}
 	}
@@ -285,6 +284,8 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 	if (IS_ERR(attach))
 		return ERR_PTR(PTR_ERR(attach));
 
+	get_dma_buf(dma_buf);
+
 	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR_OR_NULL(sgt)) {
 		ret = PTR_ERR(sgt);
@@ -305,6 +306,8 @@ fail_unmap:
 	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
 fail_detach:
 	dma_buf_detach(dma_buf, attach);
+	dma_buf_put(dma_buf);
+
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL(drm_gem_prime_import);
@@ -347,6 +350,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		goto fail;
 
 	mutex_unlock(&file_priv->prime.lock);
+
+	dma_buf_put(dma_buf);
+
 	return 0;
 
 fail:
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index ba0a3aa..ff7f2a8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -235,7 +235,6 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 			 * refcount on gem itself instead of f_count of dmabuf.
 			 */
 			drm_gem_object_reference(obj);
-			dma_buf_put(dma_buf);
 			return obj;
 		}
 	}
@@ -244,6 +243,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 	if (IS_ERR(attach))
 		return ERR_PTR(-EINVAL);
 
+	get_dma_buf(dma_buf);
 
 	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR_OR_NULL(sgt)) {
@@ -298,6 +298,8 @@ err_unmap_attach:
 	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
 err_buf_detach:
 	dma_buf_detach(dma_buf, attach);
+	dma_buf_put(dma_buf);
+
 	return ERR_PTR(ret);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 6a5af68..c303de1 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -271,7 +271,6 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 			 * refcount on gem itself instead of f_count of dmabuf.
 			 */
 			drm_gem_object_reference(&obj->base);
-			dma_buf_put(dma_buf);
 			return &obj->base;
 		}
 	}
@@ -281,6 +280,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
+	get_dma_buf(dma_buf);
+
 	obj = i915_gem_object_alloc(dev);
 	if (obj == NULL) {
 		ret = -ENOMEM;
@@ -300,5 +301,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 
 fail_detach:
 	dma_buf_detach(dma_buf, attach);
+	dma_buf_put(dma_buf);
+
 	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index ac74d1b..1bdf7e1 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -212,7 +212,6 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 			 * refcount on gem itself instead of f_count of dmabuf.
 			 */
 			drm_gem_object_reference(obj);
-			dma_buf_put(buffer);
 			return obj;
 		}
 	}
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 3816270..ef034fa 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -303,6 +303,8 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
+	get_dma_buf(dma_buf);
+
 	sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(sg)) {
 		ret = PTR_ERR(sg);
@@ -322,5 +324,7 @@ fail_unmap:
 	dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
 fail_detach:
 	dma_buf_detach(dma_buf, attach);
+	dma_buf_put(dma_buf);
+
 	return ERR_PTR(ret);
 }
-- 
1.8.2

Re: [PATCH 1/2] drm/prime: keep a reference from the handle to exported dma-buf (v5)

[Imre Deak]

On Fri, 2013-04-19 at 11:11 +1000, Dave Airlie wrote:
> Currently we have a problem with this:
> 1. i915: create gem object
> 2. i915: export gem object to prime
> 3. radeon: import gem object
> 4. close prime fd
> 5. radeon: unref object
> 6. i915: unref object
> 
> i915 has an imported object reference in its file priv, that isn't
> cleaned up properly until fd close. The reference gets added at step 2,
> but at step 6 we don't have enough info to clean it up.
> 
> The solution is to take a reference on the dma-buf when we export it,
> and drop the reference when the gem handle goes away.
> 
> So when we export a dma_buf from a gem object, we keep track of it
> with the handle, we take a reference to the dma_buf. When we close
> the handle (i.e. userspace is finished with the buffer), we drop
> the reference to the dma_buf, and it gets collected.
> 
> This patch isn't meant to fix any other problem or bikesheds, and it doesn't
> fix any races with other scenarios.
> 
> v1.1: move export symbol line back up.
> 
> v2: okay I had to do a bit more, as the first patch showed a leak
> on one of my tests, that I found using the dma-buf debugfs support,
> the problem case is exporting a buffer twice with the same handle,
> we'd add another export handle for it unnecessarily, however
> we now fail if we try to export the same object with a different gem handle,
> however I'm not sure if that is a case I want to support, and I've
> gotten the code to WARN_ON if we hit something like that.
> 
> v2.1: rebase this patch, write better commit msg.
> v3: cleanup error handling, track import vs export in linked list,
> these two patches were separate previously, but seem to work better
> like this.
> v4: danvet is correct, this code is no longer useful, since the buffer
> better exist, so remove it.
> v5: always take a reference to the dma buf object, import or export.
> (Imre Deak contributed this originally)
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

drm_prime_destroy_file_private() lacks a drm_buf_put(). A separate
issue, but the list should actually be empty at that point so perhaps we
should warn if it's not.

bikeshed: we don't really need the PRIME_{EXPORT,IMPORT} tracking.

Otherwise looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/drm_gem.c   |  4 +-
>  drivers/gpu/drm/drm_prime.c | 93 ++++++++++++++++++++++++++++-----------------
>  include/drm/drmP.h          |  4 +-
>  3 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index af779ae..cf919e3 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -205,11 +205,11 @@ static void
>  drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
>  {
>  	if (obj->import_attach) {
> -		drm_prime_remove_imported_buf_handle(&filp->prime,
> +		drm_prime_remove_buf_handle(&filp->prime,
>  				obj->import_attach->dmabuf);
>  	}
>  	if (obj->export_dma_buf) {
> -		drm_prime_remove_imported_buf_handle(&filp->prime,
> +		drm_prime_remove_buf_handle(&filp->prime,
>  				obj->export_dma_buf);
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 366910d..eb7c38d 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -57,10 +57,14 @@
>   * use the drm_gem_prime_{import,export} helpers.
>   */
>  
> +#define PRIME_IMPORT 1
> +#define PRIME_EXPORT 2
> +
>  struct drm_prime_member {
>  	struct list_head entry;
>  	struct dma_buf *dma_buf;
>  	uint32_t handle;
> +	int type;
>  };
>  
>  static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> @@ -157,6 +161,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>  	.vunmap = drm_gem_dmabuf_vunmap,
>  };
>  
> +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
> +
>  /**
>   * DOC: PRIME Helpers
>   *
> @@ -200,7 +206,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  {
>  	struct drm_gem_object *obj;
>  	void *buf;
> -	int ret;
> +	int ret = 0;
> +	struct dma_buf *dmabuf;
>  
>  	obj = drm_gem_object_lookup(dev, file_priv, handle);
>  	if (!obj)
> @@ -209,43 +216,44 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	mutex_lock(&file_priv->prime.lock);
>  	/* re-export the original imported object */
>  	if (obj->import_attach) {
> -		get_dma_buf(obj->import_attach->dmabuf);
> -		*prime_fd = dma_buf_fd(obj->import_attach->dmabuf, flags);
> -		drm_gem_object_unreference_unlocked(obj);
> -		mutex_unlock(&file_priv->prime.lock);
> -		return 0;
> +		dmabuf = obj->import_attach->dmabuf;
> +		goto out_have_obj;
>  	}
>  
>  	if (obj->export_dma_buf) {
> -		get_dma_buf(obj->export_dma_buf);
> -		*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
> -		drm_gem_object_unreference_unlocked(obj);
> -	} else {
> -		buf = dev->driver->gem_prime_export(dev, obj, flags);
> -		if (IS_ERR(buf)) {
> -			/* normally the created dma-buf takes ownership of the ref,
> -			 * but if that fails then drop the ref
> -			 */
> -			drm_gem_object_unreference_unlocked(obj);
> -			mutex_unlock(&file_priv->prime.lock);
> -			return PTR_ERR(buf);
> -		}
> -		obj->export_dma_buf = buf;
> -		*prime_fd = dma_buf_fd(buf, flags);
> +		dmabuf = obj->export_dma_buf;
> +		goto out_have_obj;
>  	}
> +
> +	buf = dev->driver->gem_prime_export(dev, obj, flags);
> +	if (IS_ERR(buf)) {
> +		/* normally the created dma-buf takes ownership of the ref,
> +		 * but if that fails then drop the ref
> +		 */
> +		ret = PTR_ERR(buf);
> +		goto out;
> +	}
> +	obj->export_dma_buf = buf;
> +
>  	/* 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_imported_buf_handle(&file_priv->prime,
> -			obj->export_dma_buf, handle);
> -	if (ret) {
> -		drm_gem_object_unreference_unlocked(obj);
> -		mutex_unlock(&file_priv->prime.lock);
> -		return ret;
> -	}
> +	ret = drm_prime_add_exported_buf_handle(&file_priv->prime,
> +						obj->export_dma_buf, handle);
> +	if (ret)
> +		goto out;
>  
> +	*prime_fd = dma_buf_fd(buf, flags);
>  	mutex_unlock(&file_priv->prime.lock);
>  	return 0;
> +
> +out_have_obj:
> +	get_dma_buf(dmabuf);
> +	*prime_fd = dma_buf_fd(dmabuf, flags);
> +out:
> +	drm_gem_object_unreference_unlocked(obj);
> +	mutex_unlock(&file_priv->prime.lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>  
> @@ -314,7 +322,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>  	mutex_lock(&file_priv->prime.lock);
>  
> -	ret = drm_prime_lookup_imported_buf_handle(&file_priv->prime,
> +	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
>  			dma_buf, handle);
>  	if (!ret) {
>  		ret = 0;
> @@ -491,7 +499,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
>  }
>  EXPORT_SYMBOL(drm_prime_destroy_file_private);
>  
> -int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle, int type)
>  {
>  	struct drm_prime_member *member;
>  
> @@ -499,14 +507,27 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
>  	if (!member)
>  		return -ENOMEM;
>  
> +	get_dma_buf(dma_buf);
>  	member->dma_buf = dma_buf;
>  	member->handle = handle;
> +	member->type = type;
>  	list_add(&member->entry, &prime_fpriv->head);
>  	return 0;
>  }
> +
> +int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +{
> +	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle, PRIME_IMPORT);
> +}
>  EXPORT_SYMBOL(drm_prime_add_imported_buf_handle);
>  
> -int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
> +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +{
> +	/* take a reference to the buf handle for this case */
> +	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle, PRIME_EXPORT);
> +}
> +
> +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;
>  
> @@ -518,19 +539,23 @@ int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fp
>  	}
>  	return -ENOENT;
>  }
> -EXPORT_SYMBOL(drm_prime_lookup_imported_buf_handle);
> +EXPORT_SYMBOL(drm_prime_lookup_buf_handle);
>  
> -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
>  {
>  	struct drm_prime_member *member, *safe;
>  
>  	mutex_lock(&prime_fpriv->lock);
>  	list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) {
>  		if (member->dma_buf == dma_buf) {
> +			dma_buf_put(dma_buf);
>  			list_del(&member->entry);
>  			kfree(member);
>  		}
>  	}
>  	mutex_unlock(&prime_fpriv->lock);
>  }
> -EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
> +EXPORT_SYMBOL(drm_prime_remove_buf_handle);
> +
> +
> +
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 2d94d74..69712a6 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1594,8 +1594,8 @@ extern void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *s
>  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_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
> -int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle);
> -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
> +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);
>  int drm_prime_lookup_obj(struct drm_device *dev, struct dma_buf *buf,

Re: [PATCH 2/2] drm: prime: fix refcounting on the dmabuf import error path

[Daniel Vetter]

On Fri, Apr 19, 2013 at 11:11:56AM +1000, Dave Airlie wrote:
> From: Imre Deak <imre.deak@intel.com>
> 
> In commit be8a42ae60 we inroduced a refcount problem, where on the
> drm_gem_prime_fd_to_handle() error path we'll call dma_buf_put() for
> self imported dma buffers.
> 
> Fix this by taking a reference on the dma buffer in the .gem_import
> hook instead of assuming the caller had taken one. Besides fixing the
> bug this is also more logical.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Dave Airlie <airlied@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_prime.c                | 8 +++++++-
>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 4 +++-
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c     | 5 ++++-
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c  | 1 -
>  drivers/gpu/drm/udl/udl_gem.c              | 4 ++++
>  5 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index eb7c38d..71f02ff 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -276,7 +276,6 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>  			 * refcount on gem itself instead of f_count of dmabuf.
>  			 */
>  			drm_gem_object_reference(obj);
> -			dma_buf_put(dma_buf);
>  			return obj;
>  		}
>  	}
> @@ -285,6 +284,8 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>  	if (IS_ERR(attach))
>  		return ERR_PTR(PTR_ERR(attach));
>  
> +	get_dma_buf(dma_buf);
> +
>  	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
>  	if (IS_ERR_OR_NULL(sgt)) {
>  		ret = PTR_ERR(sgt);
> @@ -305,6 +306,8 @@ fail_unmap:
>  	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
>  fail_detach:
>  	dma_buf_detach(dma_buf, attach);
> +	dma_buf_put(dma_buf);
> +
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL(drm_gem_prime_import);
> @@ -347,6 +350,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  		goto fail;
>  
>  	mutex_unlock(&file_priv->prime.lock);
> +
> +	dma_buf_put(dma_buf);
> +
>  	return 0;
>  
>  fail:
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> index ba0a3aa..ff7f2a8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> @@ -235,7 +235,6 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
>  			 * refcount on gem itself instead of f_count of dmabuf.
>  			 */
>  			drm_gem_object_reference(obj);
> -			dma_buf_put(dma_buf);
>  			return obj;
>  		}
>  	}
> @@ -244,6 +243,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
>  	if (IS_ERR(attach))
>  		return ERR_PTR(-EINVAL);
>  
> +	get_dma_buf(dma_buf);
>  
>  	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
>  	if (IS_ERR_OR_NULL(sgt)) {
> @@ -298,6 +298,8 @@ err_unmap_attach:
>  	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
>  err_buf_detach:
>  	dma_buf_detach(dma_buf, attach);
> +	dma_buf_put(dma_buf);
> +
>  	return ERR_PTR(ret);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 6a5af68..c303de1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -271,7 +271,6 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  			 * refcount on gem itself instead of f_count of dmabuf.
>  			 */
>  			drm_gem_object_reference(&obj->base);
> -			dma_buf_put(dma_buf);
>  			return &obj->base;
>  		}
>  	}
> @@ -281,6 +280,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> +	get_dma_buf(dma_buf);
> +
>  	obj = i915_gem_object_alloc(dev);
>  	if (obj == NULL) {
>  		ret = -ENOMEM;
> @@ -300,5 +301,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  
>  fail_detach:
>  	dma_buf_detach(dma_buf, attach);
> +	dma_buf_put(dma_buf);
> +
>  	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> index ac74d1b..1bdf7e1 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> @@ -212,7 +212,6 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  			 * refcount on gem itself instead of f_count of dmabuf.
>  			 */
>  			drm_gem_object_reference(obj);
> -			dma_buf_put(buffer);
>  			return obj;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> index 3816270..ef034fa 100644
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ b/drivers/gpu/drm/udl/udl_gem.c
> @@ -303,6 +303,8 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> +	get_dma_buf(dma_buf);
> +
>  	sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
>  	if (IS_ERR(sg)) {
>  		ret = PTR_ERR(sg);
> @@ -322,5 +324,7 @@ fail_unmap:
>  	dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>  fail_detach:
>  	dma_buf_detach(dma_buf, attach);
> +	dma_buf_put(dma_buf);
> +
>  	return ERR_PTR(ret);
>  }
> -- 
> 1.8.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/prime: keep a reference from the handle to exported dma-buf (v5)

[Daniel Vetter]

On Fri, Apr 19, 2013 at 08:54:58AM +0300, Imre Deak wrote:
> On Fri, 2013-04-19 at 11:11 +1000, Dave Airlie wrote:
> > Currently we have a problem with this:
> > 1. i915: create gem object
> > 2. i915: export gem object to prime
> > 3. radeon: import gem object
> > 4. close prime fd
> > 5. radeon: unref object
> > 6. i915: unref object
> > 
> > i915 has an imported object reference in its file priv, that isn't
> > cleaned up properly until fd close. The reference gets added at step 2,
> > but at step 6 we don't have enough info to clean it up.
> > 
> > The solution is to take a reference on the dma-buf when we export it,
> > and drop the reference when the gem handle goes away.
> > 
> > So when we export a dma_buf from a gem object, we keep track of it
> > with the handle, we take a reference to the dma_buf. When we close
> > the handle (i.e. userspace is finished with the buffer), we drop
> > the reference to the dma_buf, and it gets collected.
> > 
> > This patch isn't meant to fix any other problem or bikesheds, and it doesn't
> > fix any races with other scenarios.
> > 
> > v1.1: move export symbol line back up.
> > 
> > v2: okay I had to do a bit more, as the first patch showed a leak
> > on one of my tests, that I found using the dma-buf debugfs support,
> > the problem case is exporting a buffer twice with the same handle,
> > we'd add another export handle for it unnecessarily, however
> > we now fail if we try to export the same object with a different gem handle,
> > however I'm not sure if that is a case I want to support, and I've
> > gotten the code to WARN_ON if we hit something like that.
> > 
> > v2.1: rebase this patch, write better commit msg.
> > v3: cleanup error handling, track import vs export in linked list,
> > these two patches were separate previously, but seem to work better
> > like this.
> > v4: danvet is correct, this code is no longer useful, since the buffer
> > better exist, so remove it.
> > v5: always take a reference to the dma buf object, import or export.
> > (Imre Deak contributed this originally)
> > 
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> 
> drm_prime_destroy_file_private() lacks a drm_buf_put(). A separate
> issue, but the list should actually be empty at that point so perhaps we
> should warn if it's not.
> 
> bikeshed: we don't really need the PRIME_{EXPORT,IMPORT} tracking.
> 
> Otherwise looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Yeah, I couldn't find anything else to bitch about than what Imre spotted,
and we can easily fix that up in follow-up patches for 3.11. So

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch