diff mbox series

[1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal

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

Commit Message

Chris Wilson July 15, 2020, 10:04 a.m. UTC
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(-)

Comments

Bas Nieuwenhuizen July 15, 2020, 11:26 a.m. UTC | #1
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 mbox series

Patch

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);