| Message ID | 20230818145939.39697-1-robdclark@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] dma-buf/sw_sync: Avoid recursive lock during fence signal | expand |
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); > + } > } > > /**
On Tue, Aug 22, 2023 at 6:01 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > 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> 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); > > + } > > } > > > > /** >
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); + } } /**