* [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal
[not found] <20200715100432.13928-1-chris@chris-wilson.co.uk>
@ 2020-07-15 10:04 ` Chris Wilson
2020-07-15 11:26 ` Bas Nieuwenhuizen
0 siblings, 1 reply; 2+ messages in thread
From: Chris Wilson @ 2020-07-15 10:04 UTC (permalink / raw)
To: dri-devel
Cc: intel-gfx, Chris Wilson, Bas Nieuwenhuizen, Sumit Semwal,
Gustavo Padovan, Christian König, stable
If a signal callback releases the sw_sync fence, that will trigger a
deadlock as the timeline_fence_release recurses onto the fence->lock
(used both for signaling and the the timeline tree).
If we always hold a reference for an unsignaled fence held by the
timeline, we no longer need to detach the fence from the timeline upon
release. This is only possible since commit ea4d5a270b57
("dma-buf/sw_sync: force signal all unsignaled fences on dying timeline")
where we introduced decoupling of the fences from the timeline upon release.
Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: <stable@vger.kernel.org>
---
drivers/dma-buf/sw_sync.c | 32 +++++++-------------------------
1 file changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 348b3a9170fa..4cc2ac03a84a 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -130,16 +130,7 @@ static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
static void timeline_fence_release(struct dma_fence *fence)
{
- struct sync_pt *pt = dma_fence_to_sync_pt(fence);
struct sync_timeline *parent = dma_fence_parent(fence);
- unsigned long flags;
-
- spin_lock_irqsave(fence->lock, flags);
- if (!list_empty(&pt->link)) {
- list_del(&pt->link);
- rb_erase(&pt->node, &parent->pt_tree);
- }
- spin_unlock_irqrestore(fence->lock, flags);
sync_timeline_put(parent);
dma_fence_free(fence);
@@ -203,18 +194,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
if (!timeline_fence_signaled(&pt->base))
break;
- list_del_init(&pt->link);
+ list_del(&pt->link);
rb_erase(&pt->node, &obj->pt_tree);
- /*
- * A signal callback may release the last reference to this
- * fence, causing it to be freed. That operation has to be
- * last to avoid a use after free inside this loop, and must
- * be after we remove the fence from the timeline in order to
- * prevent deadlocking on timeline->lock inside
- * timeline_fence_release().
- */
dma_fence_signal_locked(&pt->base);
+ dma_fence_put(&pt->base);
}
spin_unlock_irq(&obj->lock);
@@ -261,13 +245,9 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
} else if (cmp < 0) {
p = &parent->rb_left;
} else {
- if (dma_fence_get_rcu(&other->base)) {
- sync_timeline_put(obj);
- kfree(pt);
- pt = other;
- goto unlock;
- }
- p = &parent->rb_left;
+ dma_fence_put(&pt->base);
+ pt = other;
+ goto unlock;
}
}
rb_link_node(&pt->node, parent, p);
@@ -278,6 +258,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
}
unlock:
+ dma_fence_get(&pt->base); /* keep a ref for the timeline */
spin_unlock_irq(&obj->lock);
return pt;
@@ -316,6 +297,7 @@ static int sw_sync_debugfs_release(struct inode *inode, struct file *file)
list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
dma_fence_set_error(&pt->base, -ENOENT);
dma_fence_signal_locked(&pt->base);
+ dma_fence_put(&pt->base);
}
spin_unlock_irq(&obj->lock);
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal
2020-07-15 10:04 ` [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
@ 2020-07-15 11:26 ` Bas Nieuwenhuizen
0 siblings, 0 replies; 2+ messages in thread
From: Bas Nieuwenhuizen @ 2020-07-15 11:26 UTC (permalink / raw)
To: Chris Wilson
Cc: ML dri-devel, intel-gfx, Sumit Semwal, Gustavo Padovan,
Christian König, stable
Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
On Wed, Jul 15, 2020 at 12:04 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> If a signal callback releases the sw_sync fence, that will trigger a
> deadlock as the timeline_fence_release recurses onto the fence->lock
> (used both for signaling and the the timeline tree).
>
> If we always hold a reference for an unsignaled fence held by the
> timeline, we no longer need to detach the fence from the timeline upon
> release. This is only possible since commit ea4d5a270b57
> ("dma-buf/sw_sync: force signal all unsignaled fences on dying timeline")
> where we introduced decoupling of the fences from the timeline upon release.
>
> Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: <stable@vger.kernel.org>
> ---
> drivers/dma-buf/sw_sync.c | 32 +++++++-------------------------
> 1 file changed, 7 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 348b3a9170fa..4cc2ac03a84a 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -130,16 +130,7 @@ static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
>
> static void timeline_fence_release(struct dma_fence *fence)
> {
> - struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> struct sync_timeline *parent = dma_fence_parent(fence);
> - unsigned long flags;
> -
> - spin_lock_irqsave(fence->lock, flags);
> - if (!list_empty(&pt->link)) {
> - list_del(&pt->link);
> - rb_erase(&pt->node, &parent->pt_tree);
> - }
> - spin_unlock_irqrestore(fence->lock, flags);
>
> sync_timeline_put(parent);
> dma_fence_free(fence);
> @@ -203,18 +194,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> if (!timeline_fence_signaled(&pt->base))
> break;
>
> - list_del_init(&pt->link);
> + list_del(&pt->link);
> rb_erase(&pt->node, &obj->pt_tree);
>
> - /*
> - * A signal callback may release the last reference to this
> - * fence, causing it to be freed. That operation has to be
> - * last to avoid a use after free inside this loop, and must
> - * be after we remove the fence from the timeline in order to
> - * prevent deadlocking on timeline->lock inside
> - * timeline_fence_release().
> - */
> dma_fence_signal_locked(&pt->base);
> + dma_fence_put(&pt->base);
> }
>
> spin_unlock_irq(&obj->lock);
> @@ -261,13 +245,9 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
> } else if (cmp < 0) {
> p = &parent->rb_left;
> } else {
> - if (dma_fence_get_rcu(&other->base)) {
> - sync_timeline_put(obj);
> - kfree(pt);
> - pt = other;
> - goto unlock;
> - }
> - p = &parent->rb_left;
> + dma_fence_put(&pt->base);
> + pt = other;
> + goto unlock;
> }
> }
> rb_link_node(&pt->node, parent, p);
> @@ -278,6 +258,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
> parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
> }
> unlock:
> + dma_fence_get(&pt->base); /* keep a ref for the timeline */
> spin_unlock_irq(&obj->lock);
>
> return pt;
> @@ -316,6 +297,7 @@ static int sw_sync_debugfs_release(struct inode *inode, struct file *file)
> list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
> dma_fence_set_error(&pt->base, -ENOENT);
> dma_fence_signal_locked(&pt->base);
> + dma_fence_put(&pt->base);
> }
>
> spin_unlock_irq(&obj->lock);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-15 11:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200715100432.13928-1-chris@chris-wilson.co.uk>
2020-07-15 10:04 ` [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
2020-07-15 11:26 ` Bas Nieuwenhuizen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).