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