[PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.

[PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.

[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>
---
 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

[PATCH 2/3] dma-buf/sw_sync: Separate signal/timeline locks

[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

[PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync

[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

[PATCH v2] dma-buf/sw_sync: Separate signal/timeline locks

[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

Re: [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.

[Bas Nieuwenhuizen]

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

Re: [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync

[Bas Nieuwenhuizen]

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

Re: [PATCH 1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal.

[Christian König]

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