Since we decouple the sync_pt from the timeline tree upon release, in order to allow releasing the sync_pt from a signal callback we need to separate the sync_pt signaling lock from the timeline tree lock.

Suggested-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl --- drivers/dma-buf/sw_sync.c | 18 +++++++++++------- drivers/dma-buf/sync_debug.h | 2 ++ 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 807c82148062..116dad6c7905 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -132,14 +132,17 @@ static void timeline_fence_release(struct dma_fence *fence) { struct sync_pt *pt = dma_fence_to_sync_pt(fence); struct sync_timeline *parent = dma_fence_parent(fence); - unsigned long flags;

- spin_lock_irqsave(fence->lock, flags); if (!list_empty(&pt->link)) { - list_del(&pt->link); - rb_erase(&pt->node, &parent->pt_tree); + unsigned long flags; + + spin_lock_irqsave(&parent->lock, flags); + if (!list_empty(&pt->link)) { + list_del(&pt->link); + rb_erase(&pt->node, &parent->pt_tree); + } + spin_unlock_irqrestore(&parent->lock, flags); } - spin_unlock_irqrestore(fence->lock, flags);

sync_timeline_put(parent); dma_fence_free(fence); @@ -252,12 +255,13 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, return NULL;

sync_timeline_get(obj); - dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock, + spin_lock_init(&pt->lock); + dma_fence_init(&pt->base, &timeline_fence_ops, &pt->lock, obj->context, value); INIT_LIST_HEAD(&pt->link);

spin_lock_irq(&obj->lock); - if (!dma_fence_is_signaled_locked(&pt->base)) { + if (!dma_fence_is_signaled(&pt->base)) { struct rb_node **p = &obj->pt_tree.rb_node; struct rb_node *parent = NULL;

diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h index 6176e52ba2d7..fd073fc32329 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -55,11 +55,13 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence) * @base: base fence object * @link: link on the sync timeline's list * @node: node in the sync timeline's tree + * @lock: fence signaling lock */ struct sync_pt { struct dma_fence base; struct list_head link; struct rb_node node; + spinlock_t lock; };

extern const struct file_operations sw_sync_debugfs_fops;

While sw_sync is purely a debug facility for userspace to create fences and timelines it can control, nevertheless it has some tricky locking semantics of its own. In particular, Bas Nieuwenhuize reported that we had reintroduced a deadlock if a signal callback attempted to destroy the fence. So let's add a few trivial selftests to make sure that once fixed again, it stays fixed.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl --- drivers/dma-buf/Makefile | 3 +- drivers/dma-buf/selftests.h | 1 + drivers/dma-buf/st-sw_sync.c | 279 +++++++++++++++++++++++++++++++++++ drivers/dma-buf/sw_sync.c | 39 +++++ drivers/dma-buf/sync_debug.h | 8 + 5 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/st-sw_sync.c

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 995e05f609ff..9be4d4611609 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF) += udmabuf.o dmabuf_selftests-y := \ selftest.o \ st-dma-fence.o \ - st-dma-fence-chain.o + st-dma-fence-chain.o \ + st-sw_sync.o

obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h index bc8cea67bf1e..232499a24872 100644 --- a/drivers/dma-buf/selftests.h +++ b/drivers/dma-buf/selftests.h @@ -12,3 +12,4 @@ selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */ selftest(dma_fence, dma_fence) selftest(dma_fence_chain, dma_fence_chain) +selftest(sw_sync, sw_sync) diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c new file mode 100644 index 000000000000..3a21e7717df7 --- /dev/null +++ b/drivers/dma-buf/st-sw_sync.c @@ -0,0 +1,279 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2020 Intel Corporation + */ + +#include <linux/delay.h> +#include <linux/dma-fence.h> +#include <linux/kernel.h> +#include <linux/kthread.h> +#include <linux/sched/signal.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +#include "sync_debug.h" +#include "selftest.h" + +static int sanitycheck(void *arg) +{ + struct sync_timeline *tl; + struct dma_fence *f; + int err = -ENOMEM; + + /* Quick check we can create the timeline and syncpt */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) + goto out; + + dma_fence_signal(f); + dma_fence_put(f); + + err = 0; +out: + st_sync_timeline_put(tl); + return 0; +} + +static int signal(void *arg) +{ + struct sync_timeline *tl; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that the syncpt fence is signaled when the timeline advances */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_is_signaled(f)) { + pr_err("syncpt:%lld signaled too early\n", f->seqno); + goto out_fence; + } + + st_sync_timeline_signal(tl, 1); + + if (!dma_fence_is_signaled(f)) { + pr_err("syncpt:%lld not signaled after increment\n", f->seqno); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_signal(f); + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +struct cb_destroy { + struct dma_fence_cb cb; + struct dma_fence *f; +}; + +static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb) +{ + struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb); + + pr_info("syncpt:%llx destroying syncpt:%llx\n", + fence->seqno, cb->f->seqno); + dma_fence_put(cb->f); + cb->f = NULL; +} + +static int cb_autodestroy(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + int err = -EINVAL; + + /* Check that we can drop the final syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + cb.f = st_sync_pt_create(tl, 1); + if (!cb.f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(cb.f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 1); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", cb.f->seqno); + dma_fence_put(cb.f); + goto out; + } + + err = 0; +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_12(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop some other syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + cb.f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 1); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_21(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop an earlier syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + cb.f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 2); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_22(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop the later syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + cb.f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 2); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +int sw_sync(void) +{ + static const struct subtest tests[] = { + SUBTEST(sanitycheck), + SUBTEST(signal), + SUBTEST(cb_autodestroy), + SUBTEST(cb_destroy_12), + SUBTEST(cb_destroy_21), + SUBTEST(cb_destroy_22), + }; + + return subtests(tests, NULL); +} diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 116dad6c7905..7c229f4a6b56 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -426,3 +426,42 @@ const struct file_operations sw_sync_debugfs_fops = { .unlocked_ioctl = sw_sync_ioctl, .compat_ioctl = compat_ptr_ioctl, }; + +#if IS_ENABLED(CONFIG_DMABUF_SELFTESTS) +struct sync_timeline *st_sync_timeline_create(const char *name) +{ + return sync_timeline_create(name); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_create); + +void st_sync_timeline_get(struct sync_timeline *tl) +{ + sync_timeline_get(tl); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_get); + +void st_sync_timeline_put(struct sync_timeline *tl) +{ + sync_timeline_put(tl); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_put); + +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc) +{ + sync_timeline_signal(tl, inc); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_signal); + +struct dma_fence * +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno) +{ + struct sync_pt *pt; + + pt = sync_pt_create(tl, seqno); + if (!pt) + return NULL; + + return &pt->base; +} +EXPORT_SYMBOL_GPL(st_sync_pt_create); +#endif diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h index fd073fc32329..7c96d9a2c02d 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -71,4 +71,12 @@ void sync_timeline_debug_remove(struct sync_timeline *obj); void sync_file_debug_add(struct sync_file *fence); void sync_file_debug_remove(struct sync_file *fence);

+struct sync_timeline *st_sync_timeline_create(const char *name); +void st_sync_timeline_get(struct sync_timeline *tl); +void st_sync_timeline_put(struct sync_timeline *tl); +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc); + +struct dma_fence * +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno); + #endif /* _LINUX_SYNC_H */

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