Thanks for updating the patch. LGTM
On Tue, Jul 14, 2020 at 10:07 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>
> From: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
