lkml.org 
[lkml]   [2023]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [Linaro-mm-sig] [PATCH v2] dma-buf/sw_sync: Avoid recursive lock during fence signal
From
Am 18.08.23 um 16:59 schrieb Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> 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).
>
> To avoid that, temporarily hold an extra reference to the signalled
> fences until after we drop the lock.
>
> (This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
> which avoids some potential UAF issues with the original patch.)
>
> v2: Remove now obsolete comment, use list_move_tail() and
> list_del_init()
>
> Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
> drivers/dma-buf/sw_sync.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 63f0aeb66db6..f0a35277fd84 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -191,6 +191,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
> */
> static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> {
> + LIST_HEAD(signalled);
> struct sync_pt *pt, *next;
>
> trace_sync_timeline(obj);
> @@ -203,21 +204,20 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> if (!timeline_fence_signaled(&pt->base))
> break;
>
> - list_del_init(&pt->link);
> + dma_fence_get(&pt->base);
> +
> + list_move_tail(&pt->link, &signalled);
> 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);
> }
>
> spin_unlock_irq(&obj->lock);
> +
> + list_for_each_entry_safe(pt, next, &signalled, link) {
> + list_del_init(&pt->link);
> + dma_fence_put(&pt->base);
> + }
> }
>
> /**

\
 
 \ /
  Last update: 2023-08-22 15:02    [W:0.063 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site