| Message ID | 20200714200646.14041-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal. | expand |
Thanks for updating the patch. LGTM On Tue, Jul 14, 2020 at 10:07 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > From: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > > Calltree: > timeline_fence_release > drm_sched_entity_wakeup > dma_fence_signal_locked > sync_timeline_signal > sw_sync_ioctl > > Releasing the reference to the fence in the fence signal callback > seems reasonable to me, so this patch avoids the locking issue in > sw_sync. > > d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists") > fixed the recursive locking issue but caused an use-after-free. Later > d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") > fixed the use-after-free but reintroduced the recursive locking issue. > > In this attempt we avoid the use-after-free still because the release > function still always locks, and outside of the locking region in the > signal function we have properly refcounted references. > > We furthermore also avoid the recurive lock by making sure that either: > > 1) We have a properly refcounted reference, preventing the signal from > triggering the release function inside the locked region. > 2) The refcount was already zero, and hence nobody will be able to trigger > the release function from the signal function. > > v2: Move dma_fence_signal() into second loop in preparation to moving > the callback out of the timeline obj->lock. > > Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") > 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> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/dma-buf/sw_sync.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 348b3a9170fa..807c82148062 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -192,6 +192,7 @@ static const struct dma_fence_ops timeline_fence_ops = { > static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) > { > struct sync_pt *pt, *next; > + LIST_HEAD(signal); > > trace_sync_timeline(obj); > > @@ -203,21 +204,32 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) > if (!timeline_fence_signaled(&pt->base)) > break; > > - list_del_init(&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(). > + * We need to take a reference to avoid a release during > + * signalling (which can cause a recursive lock of obj->lock). > + * If refcount was already zero, another thread is already > + * taking care of destroying the fence. > */ > - dma_fence_signal_locked(&pt->base); > + if (!dma_fence_get_rcu(&pt->base)) > + continue; > + > + list_move_tail(&pt->link, &signal); > + rb_erase(&pt->node, &obj->pt_tree); > } > > spin_unlock_irq(&obj->lock); > + > + list_for_each_entry_safe(pt, next, &signal, link) { > + /* > + * This needs to be cleared before release, otherwise the > + * timeline_fence_release function gets confused about also > + * removing the fence from the pt_tree. > + */ > + list_del_init(&pt->link); > + > + dma_fence_signal(&pt->base); > + dma_fence_put(&pt->base); > + } > } > > /** > -- > 2.20.1 >
Am 14.07.20 um 22:06 schrieb Chris Wilson: > From: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > > Calltree: > timeline_fence_release > drm_sched_entity_wakeup > dma_fence_signal_locked > sync_timeline_signal > sw_sync_ioctl > > Releasing the reference to the fence in the fence signal callback > seems reasonable to me, so this patch avoids the locking issue in > sw_sync. > > d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists") > fixed the recursive locking issue but caused an use-after-free. Later > d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") > fixed the use-after-free but reintroduced the recursive locking issue. > > In this attempt we avoid the use-after-free still because the release > function still always locks, and outside of the locking region in the > signal function we have properly refcounted references. > > We furthermore also avoid the recurive lock by making sure that either: > > 1) We have a properly refcounted reference, preventing the signal from > triggering the release function inside the locked region. > 2) The refcount was already zero, and hence nobody will be able to trigger > the release function from the signal function. > > v2: Move dma_fence_signal() into second loop in preparation to moving > the callback out of the timeline obj->lock. > > Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") > 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> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Looks reasonable to me, but I'm not an expert on this container. So patch is Acked-by: Christian König <christian.koenig@amd.com> Regards, Christian. > --- > drivers/dma-buf/sw_sync.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 348b3a9170fa..807c82148062 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -192,6 +192,7 @@ static const struct dma_fence_ops timeline_fence_ops = { > static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) > { > struct sync_pt *pt, *next; > + LIST_HEAD(signal); > > trace_sync_timeline(obj); > > @@ -203,21 +204,32 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) > if (!timeline_fence_signaled(&pt->base)) > break; > > - list_del_init(&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(). > + * We need to take a reference to avoid a release during > + * signalling (which can cause a recursive lock of obj->lock). > + * If refcount was already zero, another thread is already > + * taking care of destroying the fence. > */ > - dma_fence_signal_locked(&pt->base); > + if (!dma_fence_get_rcu(&pt->base)) > + continue; > + > + list_move_tail(&pt->link, &signal); > + rb_erase(&pt->node, &obj->pt_tree); > } > > spin_unlock_irq(&obj->lock); > + > + list_for_each_entry_safe(pt, next, &signal, link) { > + /* > + * This needs to be cleared before release, otherwise the > + * timeline_fence_release function gets confused about also > + * removing the fence from the pt_tree. > + */ > + list_del_init(&pt->link); > + > + dma_fence_signal(&pt->base); > + dma_fence_put(&pt->base); > + } > } > > /**
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 348b3a9170fa..807c82148062 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -192,6 +192,7 @@ static const struct dma_fence_ops timeline_fence_ops = { static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) { struct sync_pt *pt, *next; + LIST_HEAD(signal); trace_sync_timeline(obj); @@ -203,21 +204,32 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) if (!timeline_fence_signaled(&pt->base)) break; - list_del_init(&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(). + * We need to take a reference to avoid a release during + * signalling (which can cause a recursive lock of obj->lock). + * If refcount was already zero, another thread is already + * taking care of destroying the fence. */ - dma_fence_signal_locked(&pt->base); + if (!dma_fence_get_rcu(&pt->base)) + continue; + + list_move_tail(&pt->link, &signal); + rb_erase(&pt->node, &obj->pt_tree); } spin_unlock_irq(&obj->lock); + + list_for_each_entry_safe(pt, next, &signal, link) { + /* + * This needs to be cleared before release, otherwise the + * timeline_fence_release function gets confused about also + * removing the fence from the pt_tree. + */ + list_del_init(&pt->link); + + dma_fence_signal(&pt->base); + dma_fence_put(&pt->base); + } } /**