| Message ID | 20200715100432.13928-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal | expand |
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 >
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);
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(-)