repost fix for a real prime issue

repost fix for a real prime issue

[Dave Airlie]

Please don't bikeshed this with requirements to fix problems that
are there now anyways. This is the simplest patch to fix an obvious
problem, it doesn't fix all the other problems.

I should have merged this months ago, but people keep wanting a
superpatch to fix everything.

Dave.

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

[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.

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

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index af779ae..f0f7a86 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -209,7 +209,7 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 				obj->import_attach->dmabuf);
 	}
 	if (obj->export_dma_buf) {
-		drm_prime_remove_imported_buf_handle(&filp->prime,
+		drm_prime_remove_exported_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..05ee250 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -157,6 +157,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
  *
@@ -201,6 +203,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	void *buf;
 	int ret;
+	struct dma_buf *dmabuf;
+	uint32_t exp_handle;
 
 	obj = drm_gem_object_lookup(dev, file_priv, handle);
 	if (!obj)
@@ -209,41 +213,52 @@ 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);
+		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
+		 */
 		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);
+		mutex_unlock(&file_priv->prime.lock);
+		return PTR_ERR(buf);
 	}
+	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);
+	ret = drm_prime_add_exported_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;
 	}
+	*prime_fd = dma_buf_fd(buf, flags);
+	mutex_unlock(&file_priv->prime.lock);
+	return 0;
 
+out_have_obj:
+	get_dma_buf(dmabuf);
+	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
+					  dmabuf, &exp_handle);
+	if (WARN_ON(ret == -ENOENT || exp_handle != handle)) {
+		dma_buf_put(dmabuf);
+		drm_gem_object_unreference_unlocked(obj);
+		mutex_unlock(&file_priv->prime.lock);
+		return -EINVAL;
+	}
+	*prime_fd = dma_buf_fd(dmabuf, flags);
+	drm_gem_object_unreference_unlocked(obj);
 	mutex_unlock(&file_priv->prime.lock);
 	return 0;
 }
@@ -314,7 +329,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 +506,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)
+int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
 {
 	struct drm_prime_member *member;
 
@@ -504,9 +519,21 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
 	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);
+}
 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 */
+	get_dma_buf(dma_buf);
+	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle);
+}
+
+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,9 +545,9 @@ 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;
 
@@ -533,4 +560,19 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f
 	}
 	mutex_unlock(&prime_fpriv->lock);
 }
+
+void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
+{
+	return drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
+}
 EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
+
+void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
+{
+	drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
+	dma_buf_put(dma_buf);
+}
+EXPORT_SYMBOL(drm_prime_remove_exported_buf_handle);
+
+
+
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2d94d74..0466808 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1594,8 +1594,9 @@ 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);
+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_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
+void drm_prime_remove_exported_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

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

[Imre Deak]

On Wed, 2013-04-10 at 10:56 +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.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

This fixes the i-g-t/prime_self_import test case too, so fwiw:
Acked-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/drm_gem.c   |  2 +-
>  drivers/gpu/drm/drm_prime.c | 94 ++++++++++++++++++++++++++++++++-------------
>  include/drm/drmP.h          |  3 +-
>  3 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index af779ae..f0f7a86 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -209,7 +209,7 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
>  				obj->import_attach->dmabuf);
>  	}
>  	if (obj->export_dma_buf) {
> -		drm_prime_remove_imported_buf_handle(&filp->prime,
> +		drm_prime_remove_exported_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..05ee250 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -157,6 +157,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
>   *
> @@ -201,6 +203,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	void *buf;
>  	int ret;
> +	struct dma_buf *dmabuf;
> +	uint32_t exp_handle;
>  
>  	obj = drm_gem_object_lookup(dev, file_priv, handle);
>  	if (!obj)
> @@ -209,41 +213,52 @@ 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);
> +		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
> +		 */
>  		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);
> +		mutex_unlock(&file_priv->prime.lock);
> +		return PTR_ERR(buf);
>  	}
> +	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);
> +	ret = drm_prime_add_exported_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;
>  	}
> +	*prime_fd = dma_buf_fd(buf, flags);
> +	mutex_unlock(&file_priv->prime.lock);
> +	return 0;
>  
> +out_have_obj:
> +	get_dma_buf(dmabuf);
> +	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> +					  dmabuf, &exp_handle);
> +	if (WARN_ON(ret == -ENOENT || exp_handle != handle)) {
> +		dma_buf_put(dmabuf);
> +		drm_gem_object_unreference_unlocked(obj);
> +		mutex_unlock(&file_priv->prime.lock);
> +		return -EINVAL;
> +	}
> +	*prime_fd = dma_buf_fd(dmabuf, flags);
> +	drm_gem_object_unreference_unlocked(obj);
>  	mutex_unlock(&file_priv->prime.lock);
>  	return 0;
>  }
> @@ -314,7 +329,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 +506,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)
> +int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
>  {
>  	struct drm_prime_member *member;
>  
> @@ -504,9 +519,21 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
>  	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);
> +}
>  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 */
> +	get_dma_buf(dma_buf);
> +	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle);
> +}
> +
> +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,9 +545,9 @@ 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;
>  
> @@ -533,4 +560,19 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f
>  	}
>  	mutex_unlock(&prime_fpriv->lock);
>  }
> +
> +void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	return drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +}
>  EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
> +
> +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +	dma_buf_put(dma_buf);
> +}
> +EXPORT_SYMBOL(drm_prime_remove_exported_buf_handle);
> +
> +
> +
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 2d94d74..0466808 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1594,8 +1594,9 @@ 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);
> +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_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
> +void drm_prime_remove_exported_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] drm/prime: keep a reference from the handle to exported dma-buf (v2.1)

[Daniel Vetter]

On Wed, Apr 10, 2013 at 10:56:43AM +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.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Ok, so I've figured I'll do a little exercising in laywer-thinking and I
grumpily agree that you can make the case that your patch doesn't
introduce any new races.

And with some convolution I think we can even fix the remaining ones
without undoing this patch here: The reason I've been harping about where
the unref for obj->export_dma_buf should happen is that that's currently
broken and at least my idea of fixing the below mess would have solved it
(I think), too. But we can smash the refcount fix for obj->export_dma_buf
on top of this here.

On second though it's actually a bit cleaner like that ;-)

And if I follow the legal-brainbender argument, the race I've pointed out
is also a pre-existing one. But the last issue stuck, so I want to clarify
that before smashing an r-b onto this patch. See below.

> ---
>  drivers/gpu/drm/drm_gem.c   |  2 +-
>  drivers/gpu/drm/drm_prime.c | 94 ++++++++++++++++++++++++++++++++-------------
>  include/drm/drmP.h          |  3 +-
>  3 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index af779ae..f0f7a86 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -209,7 +209,7 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
>  				obj->import_attach->dmabuf);
>  	}
>  	if (obj->export_dma_buf) {
> -		drm_prime_remove_imported_buf_handle(&filp->prime,
> +		drm_prime_remove_exported_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..05ee250 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -157,6 +157,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
>   *
> @@ -201,6 +203,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	void *buf;
>  	int ret;
> +	struct dma_buf *dmabuf;
> +	uint32_t exp_handle;
>  
>  	obj = drm_gem_object_lookup(dev, file_priv, handle);
>  	if (!obj)
> @@ -209,41 +213,52 @@ 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);
> +		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
> +		 */
>  		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);
> +		mutex_unlock(&file_priv->prime.lock);
> +		return PTR_ERR(buf);
>  	}
> +	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);
> +	ret = drm_prime_add_exported_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;
>  	}
> +	*prime_fd = dma_buf_fd(buf, flags);
> +	mutex_unlock(&file_priv->prime.lock);
> +	return 0;
>  
> +out_have_obj:
> +	get_dma_buf(dmabuf);
> +	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> +					  dmabuf, &exp_handle);

So after much pondering the only thing I could come up with here is that
this tries to duct-tape over userspace racing handle_to_fd against
gem_close (which would also reap the buf_handle lookup table). Or more
evil, gem_close + gem_open (using an flink name or dma_buf handle acquired
somewhere else).

Now afaics we don't care one bit about this, and all issues/races in this
area are pre-existing:
- The slightly incosisten locking of the handle->dma_buf table
- The races around obj->export_dma_buf.

Presuming those are fixed I still don't see a reason to care - userspace
might as well do a gem_close right _after_ the handle_to_fd call (and
maybe even follow with a fd_to_handle right afterwards) and end up with
the exact same situation you're trying to prevent here. Of course, there's
not much point in userspace doing that, but meh.

So imo this check here should be just ripped out. Or do I miss something
here?

Cheers, Daniel

> +	if (WARN_ON(ret == -ENOENT || exp_handle != handle)) {
> +		dma_buf_put(dmabuf);
> +		drm_gem_object_unreference_unlocked(obj);
> +		mutex_unlock(&file_priv->prime.lock);
> +		return -EINVAL;
> +	}
> +	*prime_fd = dma_buf_fd(dmabuf, flags);
> +	drm_gem_object_unreference_unlocked(obj);
>  	mutex_unlock(&file_priv->prime.lock);
>  	return 0;
>  }
> @@ -314,7 +329,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 +506,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)
> +int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
>  {
>  	struct drm_prime_member *member;
>  
> @@ -504,9 +519,21 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
>  	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);
> +}
>  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 */
> +	get_dma_buf(dma_buf);
> +	return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle);
> +}
> +
> +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,9 +545,9 @@ 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;
>  
> @@ -533,4 +560,19 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f
>  	}
>  	mutex_unlock(&prime_fpriv->lock);
>  }
> +
> +void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	return drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +}
>  EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
> +
> +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> +	drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +	dma_buf_put(dma_buf);
> +}
> +EXPORT_SYMBOL(drm_prime_remove_exported_buf_handle);
> +
> +
> +
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 2d94d74..0466808 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1594,8 +1594,9 @@ 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);
> +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_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
> +void drm_prime_remove_exported_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
> 
> _______________________________________________
> 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: repost fix for a real prime issue

[Daniel Vetter]

On Wed, Apr 10, 2013 at 10:56:42AM +1000, Dave Airlie wrote:
> Please don't bikeshed this with requirements to fix problems that
> are there now anyways. This is the simplest patch to fix an obvious
> problem, it doesn't fix all the other problems.
> 
> I should have merged this months ago, but people keep wanting a
> superpatch to fix everything.

Imo same review on the semantics of the patch itself still applies:

http://lists.freedesktop.org/archives/dri-devel/2012-December/032374.html

Two main things:
- I think the dma_buf reference attached to gem handles should be dropped
  in drm_gem_object_handle_free instead of drm_gem_handle_delete.
- I still have no idea what the drm_prime_lookup_buf_handle check in
  handle_to_fd is for ...

Note that the locking review was in a 2nd mail:

http://lists.freedesktop.org/archives/dri-devel/2012-December/032376.html

Imo this is an issue with this very patch since this patch also adds the
dma_buf reference on exported objects while a handle is open.

So I don't think my original review asked for a superpatch to fix all the
issues with currently have, but only for a correct one implementing the
handle holds ref on exported obj logic ;-)

I'll try to pimp the self import testcase we have a bit to exercise these
corner cases.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch