* drm prime locking recursion
@ 2015-10-14 3:08 Dave Airlie
2015-10-14 9:52 ` [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Dave Airlie @ 2015-10-14 3:08 UTC (permalink / raw)
To: dri-devel
Got this playing with virgl, it happens when gem_object_open driver
callback fails.
Now this probably shouldn't be failing that often, but when it does
deadlock seems wrong.
Dave.
happens if the driver fails
[ 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]
[ 677.932957]
other info that might help us debug this:
[ 677.932957] Possible unsafe locking scenario:
[ 677.932957] CPU0
[ 677.932957] ----
[ 677.932957] lock(&prime_fpriv->lock);
[ 677.932957] lock(&prime_fpriv->lock);
[ 677.932957]
*** DEADLOCK ***
[ 677.932957] May be due to missing lock nesting notation
[ 677.932957] 1 lock held by Xorg/2661:
[ 677.932957] #0: (&prime_fpriv->lock){+.+.+.}, at:
[<ffffffffa002d68b>] drm_gem_prime_fd_to_handle+0x4b/0x240 [drm]
[ 677.932957]
stack backtrace:
[ 677.932957] CPU: 1 PID: 2661 Comm: Xorg Not tainted 4.3.0-rc5-virtio-gpu+ #11
[ 677.932957] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 677.932957] ffffffff82619740 ffff88007b22fb20 ffffffff813159f9
ffffffff82619740
[ 677.932957] ffff88007b22fbd8 ffffffff810d373d 0000000000000000
ffff88007b22fb58
[ 677.932957] ffffffff00000000 ffff88004cfd8700 00000000008e8474
0000000000048af0
[ 677.932957] Call Trace:
[ 677.932957] [<ffffffff813159f9>] dump_stack+0x4b/0x72
[ 677.932957] [<ffffffff810d373d>] __lock_acquire+0x193d/0x1a60
[ 677.932957] [<ffffffff810d411d>] lock_acquire+0x6d/0x90
[ 677.932957] [<ffffffffa00151b0>] ?
drm_gem_remove_prime_handles.isra.7+0x20/0x50 [drm]
[ 677.932957] [<ffffffff816d30e4>] mutex_lock_nested+0x64/0x3a0
[ 677.932957] [<ffffffffa00151b0>] ?
drm_gem_remove_prime_handles.isra.7+0x20/0x50 [drm]
[ 677.932957] [<ffffffffa00151b0>]
drm_gem_remove_prime_handles.isra.7+0x20/0x50 [drm]
[ 677.932957] [<ffffffffa0015947>] drm_gem_handle_delete+0xd7/0x110 [drm]
[ 677.932957] [<ffffffffa0015baf>] drm_gem_handle_create_tail+0xff/0x160 [drm]
[ 677.932957] [<ffffffffa002d731>] drm_gem_prime_fd_to_handle+0xf1/0x240 [drm]
[ 677.932957] [<ffffffffa002dd28>]
drm_prime_fd_to_handle_ioctl+0x28/0x40 [drm]
[ 677.932957] [<ffffffffa0016594>] drm_ioctl+0x124/0x4f0 [drm]
[ 677.932957] [<ffffffffa002dd00>] ?
drm_prime_handle_to_fd_ioctl+0x60/0x60 [drm]
[ 677.932957] [<ffffffff812b40e7>] ? ioctl_has_perm+0xa7/0xc0
[ 677.932957] [<ffffffff811c30aa>] do_vfs_ioctl+0x2da/0x530
[ 677.932957] [<ffffffff812b4159>] ? selinux_file_ioctl+0x59/0xf0
[ 677.932957] [<ffffffff812a68ae>] ? security_file_ioctl+0x3e/0x60
[ 677.932957] [<ffffffff811c3374>] SyS_ioctl+0x74/0x80
[ 677.932957] [<ffffffff816d6632>] entry_SYSCALL_64_fastpath+0x12/0x76
[ 840.028068] INFO: task Xorg:2661 blocked for more than 120 seconds.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock 2015-10-14 3:08 drm prime locking recursion Dave Airlie @ 2015-10-14 9:52 ` Chris Wilson 2015-10-14 10:01 ` Chris Wilson 2015-10-14 22:57 ` Dave Airlie 0 siblings, 2 replies; 4+ messages in thread From: Chris Wilson @ 2015-10-14 9:52 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter 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(-) 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); -- 2.6.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock 2015-10-14 9:52 ` [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock Chris Wilson @ 2015-10-14 10:01 ` Chris Wilson 2015-10-14 22:57 ` Dave Airlie 1 sibling, 0 replies; 4+ messages in thread From: Chris Wilson @ 2015-10-14 10:01 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter 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 -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock 2015-10-14 9:52 ` [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock Chris Wilson 2015-10-14 10:01 ` Chris Wilson @ 2015-10-14 22:57 ` Dave Airlie 1 sibling, 0 replies; 4+ messages in thread From: Dave Airlie @ 2015-10-14 22:57 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, dri-devel 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-14 22:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-14 3:08 drm prime locking recursion Dave Airlie 2015-10-14 9:52 ` [PATCH] drm/prime: Move all unreferences on fd_to_handle error paths to after unlock Chris Wilson 2015-10-14 10:01 ` Chris Wilson 2015-10-14 22:57 ` Dave Airlie
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.