* [PATCH 2/3] dma-buf/sw_sync: Separate signal/timeline locks
2020-07-14 20:06 [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
@ 2020-07-14 20:06 ` Chris Wilson
2020-07-14 20:14 ` [PATCH v2] " Chris Wilson
2020-07-14 20:06 ` [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync Chris Wilson
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2020-07-14 20:06 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Chris Wilson
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;
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2] dma-buf/sw_sync: Separate signal/timeline locks
2020-07-14 20:06 ` [PATCH 2/3] dma-buf/sw_sync: Separate signal/timeline locks Chris Wilson
@ 2020-07-14 20:14 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2020-07-14 20:14 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Chris Wilson
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.
v2: Mark up the unlocked read of the current timeline value.
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 | 33 ++++++++++++++++++++-------------
drivers/dma-buf/sync_debug.h | 2 ++
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 807c82148062..5851bf7076d0 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -132,24 +132,32 @@ 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);
}
-static bool timeline_fence_signaled(struct dma_fence *fence)
+static int timeline_value(struct dma_fence *fence)
{
- struct sync_timeline *parent = dma_fence_parent(fence);
+ return READ_ONCE(dma_fence_parent(fence)->value);
+}
- return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
+static bool timeline_fence_signaled(struct dma_fence *fence)
+{
+ return !__dma_fence_is_later(fence->seqno,
+ timeline_value(fence),
+ fence->ops);
}
static bool timeline_fence_enable_signaling(struct dma_fence *fence)
@@ -166,9 +174,7 @@ static void timeline_fence_value_str(struct dma_fence *fence,
static void timeline_fence_timeline_value_str(struct dma_fence *fence,
char *str, int size)
{
- struct sync_timeline *parent = dma_fence_parent(fence);
-
- snprintf(str, size, "%d", parent->value);
+ snprintf(str, size, "%d", timeline_value(fence));
}
static const struct dma_fence_ops timeline_fence_ops = {
@@ -252,12 +258,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;
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync
2020-07-14 20:06 [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
2020-07-14 20:06 ` [PATCH 2/3] dma-buf/sw_sync: Separate signal/timeline locks Chris Wilson
@ 2020-07-14 20:06 ` Chris Wilson
2020-07-14 20:29 ` Bas Nieuwenhuizen
2020-07-14 20:14 ` [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Bas Nieuwenhuizen
2020-07-15 8:57 ` Christian König
3 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2020-07-14 20:06 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Chris Wilson
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 */
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync
2020-07-14 20:06 ` [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync Chris Wilson
@ 2020-07-14 20:29 ` Bas Nieuwenhuizen
0 siblings, 0 replies; 7+ messages in thread
From: Bas Nieuwenhuizen @ 2020-07-14 20:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, ML dri-devel
Awsome, thanks for adding the tests!
Got to say I'm not that familiar with the self-test framework idioms,
but from my perspective patches 2 and 3 are
Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
as well.
On Tue, Jul 14, 2020 at 10:06 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> 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 */
> --
> 2.20.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.
2020-07-14 20:06 [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
2020-07-14 20:06 ` [PATCH 2/3] dma-buf/sw_sync: Separate signal/timeline locks Chris Wilson
2020-07-14 20:06 ` [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync Chris Wilson
@ 2020-07-14 20:14 ` Bas Nieuwenhuizen
2020-07-15 8:57 ` Christian König
3 siblings, 0 replies; 7+ messages in thread
From: Bas Nieuwenhuizen @ 2020-07-14 20:14 UTC (permalink / raw)
To: Chris Wilson
Cc: Gustavo Padovan, intel-gfx, stable, Christian König, ML dri-devel
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.
2020-07-14 20:06 [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Chris Wilson
` (2 preceding siblings ...)
2020-07-14 20:14 ` [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal Bas Nieuwenhuizen
@ 2020-07-15 8:57 ` Christian König
3 siblings, 0 replies; 7+ messages in thread
From: Christian König @ 2020-07-15 8:57 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: Gustavo Padovan, intel-gfx, stable
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);
> + }
> }
>
> /**
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread