diff mbox

drm/prime: Move all unreferences on fd_to_handle error paths to after unlock

Message ID 1444816320-3038-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 14, 2015, 9:52 a.m. UTC 
If we need to take the error path and drop the references to the objects
and handle we created earlier, there is a possibility that we then free
the object and then attempt to free any associated PRIME resources under
the prime mutex. If we are holding the prime mutex when we attempt to
free the handle/object, we risk a recursive deadlock.

[  677.932957] =============================================
[  677.932957] [ INFO: possible recursive locking detected ]
[  677.932957] 4.3.0-rc5-virtio-gpu+ #11 Not tainted
[  677.932957] ---------------------------------------------
[  677.932957] Xorg/2661 is trying to acquire lock:
[  677.932957]  (&prime_fpriv->lock){+.+.+.}, at: [<ffffffffa00151b0>] drm_gem_remove_prime_handles.isra.7+0x20/0x50 [drm]
[  677.932957] but task is already holding lock:
[  677.932957]  (&prime_fpriv->lock){+.+.+.}, at: [<ffffffffa002d68b>] drm_gem_prime_fd_to_handle+0x4b/0x240 [drm]

Reported-by: Dave Airlie <airlied@gmail.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@gmail.com>
---
 drivers/gpu/drm/drm_prime.c | 47 ++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

Comments

Chris Wilson Oct. 14, 2015, 10:01 a.m. UTC | #1 
On Wed, Oct 14, 2015 at 10:52:00AM +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 27aa7183b20b..1bdd69e1ef43 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -564,26 +564,26 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  			       uint32_t *handle)

>  	/* never seen this one, need to import */
> -	mutex_lock(&dev->object_name_lock);

I found the use of object_name_lock here confusing. I couldn't see what
it was meant to protect (it is supposed to just lock access to the
global flink name assigned to the object, and I'm sure what value it is
adding to drm_gem_handle_create_tail() either). Anyway the handle here is
protected against concurrent creation/destruction by the prime.lock, so
it looked safe enough to move the object_name_lock back to to
drm_gem_handle_create_tail().
-Chris
Dave Airlie Oct. 14, 2015, 10:57 p.m. UTC | #2 
On 14 October 2015 at 19:52, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If we need to take the error path and drop the references to the objects
> and handle we created earlier, there is a possibility that we then free
> the object and then attempt to free any associated PRIME resources under
> the prime mutex. If we are holding the prime mutex when we attempt to
> free the handle/object, we risk a recursive deadlock.

This doesn't fix the case where drm_gem_handle_create_tail internally
fails and calls drm_gem_handle_delete, which then takes the lock again.

Dave.
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 27aa7183b20b..1bdd69e1ef43 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -564,26 +564,26 @@  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 			       uint32_t *handle)
 {
 	struct dma_buf *dma_buf;
-	struct drm_gem_object *obj;
+	struct drm_gem_object *obj = NULL;
 	int ret;
 
+	*handle = 0;
+
 	dma_buf = dma_buf_get(prime_fd);
 	if (IS_ERR(dma_buf))
 		return PTR_ERR(dma_buf);
 
 	mutex_lock(&file_priv->prime.lock);
 
-	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
-			dma_buf, handle);
+	ret = drm_prime_lookup_buf_handle(&file_priv->prime, dma_buf, handle);
 	if (ret == 0)
-		goto out_put;
+		goto out;
 
 	/* 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_unlock;
+		goto out;
 	}
 
 	if (obj->dma_buf) {
@@ -593,33 +593,24 @@  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		get_dma_buf(dma_buf);
 	}
 
-	/* 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);
+	ret = drm_gem_handle_create(file_priv, obj, handle);
 	if (ret)
-		goto out_put;
+		goto out;
 
-	ret = drm_prime_add_buf_handle(&file_priv->prime,
-			dma_buf, *handle);
-	if (ret)
-		goto fail;
+	ret = drm_prime_add_buf_handle(&file_priv->prime, dma_buf, *handle);
 
+out:
 	mutex_unlock(&file_priv->prime.lock);
-
-	dma_buf_put(dma_buf);
-
-	return 0;
-
-fail:
-	/* hmm, if driver attached, we are relying on the free-object path
-	 * to detach.. which seems ok..
-	 */
-	drm_gem_handle_delete(file_priv, *handle);
-out_unlock:
-	mutex_unlock(&dev->object_name_lock);
-out_put:
+	if (ret) {
+		/* hmm, if driver attached, we are relying on the
+		 * free-object path to detach.. which seems ok..
+		 */
+		if (*handle)
+			drm_gem_handle_delete(file_priv, *handle);
+	}
+	if (!IS_ERR_OR_NULL(obj))
+		drm_gem_object_unreference_unlocked(obj);
 	dma_buf_put(dma_buf);
-	mutex_unlock(&file_priv->prime.lock);
 	return ret;
 }
 EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);