[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAF6AEGs+6cveKbv=onEJSZJERk8m56YJzza6A2+eLd3+6MuWMg@mail.gmail.com>
Date: Tue, 22 Aug 2023 10:15:52 -0700
From: Rob Clark <robdclark@...il.com>
To: Christian König <ckoenig.leichtzumerken@...il.com>
Cc: dri-devel@...ts.freedesktop.org,
Christian König <christian.koenig@....com>,
Rob Clark <robdclark@...omium.org>,
Bas Nieuwenhuizen <bas@...nieuwenhuizen.nl>,
Sumit Semwal <sumit.semwal@...aro.org>,
Gustavo Padovan <gustavo@...ovan.org>,
Chris Wilson <chris@...is-wilson.co.uk>,
"open list:SYNC FILE FRAMEWORK" <linux-media@...r.kernel.org>,
"moderated list:DMA BUFFER SHARING FRAMEWORK"
<linaro-mm-sig@...ts.linaro.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [Linaro-mm-sig] [PATCH v2] dma-buf/sw_sync: Avoid recursive lock
during fence signal
On Tue, Aug 22, 2023 at 6:01 AM Christian König
<ckoenig.leichtzumerken@...il.com> wrote:
>
> Am 18.08.23 um 16:59 schrieb Rob Clark:
> > From: Rob Clark <robdclark@...omium.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@...nieuwenhuizen.nl>
> > Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> > Signed-off-by: Rob Clark <robdclark@...omium.org>
>
> Reviewed-by: Christian König <christian.koenig@....com>
Thanks, any chance you could take this via drm-misc?
BR,
-R
>
> > ---
> > 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);
> > + }
> > }
> >
> > /**
>
Powered by blists - more mailing lists