From patchwork Wed Jul 15 10:04:31 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 11664717 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id F1AE2618 for ; Wed, 15 Jul 2020 10:05:04 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DB1F6206D5 for ; Wed, 15 Jul 2020 10:05:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DB1F6206D5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=chris-wilson.co.uk Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D9E7B6EA80; Wed, 15 Jul 2020 10:05:02 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from fireflyinternet.com (unknown [77.68.26.236]) by gabe.freedesktop.org (Postfix) with ESMTPS id 177426EA7E; Wed, 15 Jul 2020 10:05:00 +0000 (UTC) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from build.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 21824638-1500050 for multiple; Wed, 15 Jul 2020 11:04:39 +0100 From: Chris Wilson To: dri-devel@lists.freedesktop.org Subject: [PATCH 1/2] dma-buf/sw_sync: Avoid recursive lock during fence signal Date: Wed, 15 Jul 2020 11:04:31 +0100 Message-Id: <20200715100432.13928-2-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200715100432.13928-1-chris@chris-wilson.co.uk> References: <20200715100432.13928-1-chris@chris-wilson.co.uk> MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Gustavo Padovan , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Chris Wilson , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" If a signal callback releases the sw_sync fence, that will trigger a deadlock as the timeline_fence_release recurses onto the fence->lock (used both for signaling and the the timeline tree). If we always hold a reference for an unsignaled fence held by the timeline, we no longer need to detach the fence from the timeline upon release. This is only possible since commit ea4d5a270b57 ("dma-buf/sw_sync: force signal all unsignaled fences on dying timeline") where we introduced decoupling of the fences from the timeline upon release. Reported-by: Bas Nieuwenhuizen Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") Signed-off-by: Chris Wilson Cc: Sumit Semwal Cc: Chris Wilson Cc: Gustavo Padovan Cc: Christian König Cc: Reviewed-by: Bas Nieuwenhuizen --- drivers/dma-buf/sw_sync.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 348b3a9170fa..4cc2ac03a84a 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -130,16 +130,7 @@ static const char *timeline_fence_get_timeline_name(struct dma_fence *fence) 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); - } - spin_unlock_irqrestore(fence->lock, flags); sync_timeline_put(parent); dma_fence_free(fence); @@ -203,18 +194,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) if (!timeline_fence_signaled(&pt->base)) break; - list_del_init(&pt->link); + list_del(&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(). - */ dma_fence_signal_locked(&pt->base); + dma_fence_put(&pt->base); } spin_unlock_irq(&obj->lock); @@ -261,13 +245,9 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, } else if (cmp < 0) { p = &parent->rb_left; } else { - if (dma_fence_get_rcu(&other->base)) { - sync_timeline_put(obj); - kfree(pt); - pt = other; - goto unlock; - } - p = &parent->rb_left; + dma_fence_put(&pt->base); + pt = other; + goto unlock; } } rb_link_node(&pt->node, parent, p); @@ -278,6 +258,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list); } unlock: + dma_fence_get(&pt->base); /* keep a ref for the timeline */ spin_unlock_irq(&obj->lock); return pt; @@ -316,6 +297,7 @@ static int sw_sync_debugfs_release(struct inode *inode, struct file *file) list_for_each_entry_safe(pt, next, &obj->pt_list, link) { dma_fence_set_error(&pt->base, -ENOENT); dma_fence_signal_locked(&pt->base); + dma_fence_put(&pt->base); } spin_unlock_irq(&obj->lock); From patchwork Wed Jul 15 10:04:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 11664713 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 89494138C for ; Wed, 15 Jul 2020 10:04:52 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 72139206D5 for ; Wed, 15 Jul 2020 10:04:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 72139206D5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=chris-wilson.co.uk Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CE66C6EA81; Wed, 15 Jul 2020 10:04:49 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from fireflyinternet.com (unknown [77.68.26.236]) by gabe.freedesktop.org (Postfix) with ESMTPS id 681796EA7D; Wed, 15 Jul 2020 10:04:47 +0000 (UTC) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from build.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 21824639-1500050 for multiple; Wed, 15 Jul 2020 11:04:40 +0100 From: Chris Wilson To: dri-devel@lists.freedesktop.org Subject: [PATCH 2/2] dma-buf/selftests: Add locking selftests for sw_sync Date: Wed, 15 Jul 2020 11:04:32 +0100 Message-Id: <20200715100432.13928-3-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200715100432.13928-1-chris@chris-wilson.co.uk> References: <20200715100432.13928-1-chris@chris-wilson.co.uk> MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, Chris Wilson Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 Nieuwenhuizen 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 Cc: Bas Nieuwenhuizen Reviewed-by: Bas Nieuwenhuizen --- 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..145fd330f1c6 --- /dev/null +++ b/drivers/dma-buf/st-sw_sync.c @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2020 Intel Corporation + */ + +#include +#include +#include +#include +#include +#include +#include + +#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 err; +} + +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 err; +} + +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 err; +} + +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 (!cb.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 err; +} + +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 (!cb.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 err; +} + +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 (!cb.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 err; +} + +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 4cc2ac03a84a..72de93211c0c 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -392,3 +392,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 6176e52ba2d7..b3e2dbaedf7e 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -69,4 +69,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 */