linux-kernel - [PATCH] dma-fence: allow dma fence to have their own lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220530142232.2871634-1-senozhatsky@chromium.org>
Date:   Mon, 30 May 2022 23:22:32 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Sumit Semwal <sumit.semwal@...aro.org>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Christian Konig <christian.koenig@....com>
Cc:     Tomasz Figa <tfiga@...omium.org>,
        Ricardo Ribalda <ribalda@...omium.org>,
        Christoph Hellwig <hch@...radead.org>,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: [PATCH] dma-fence: allow dma fence to have their own lock

	RFC

	I don't have a good name for this yet and I did not spend
	any time on documentataion (for that reason)

We create fences (out fences) as part of operations execution, which
are short-lived objects, we want to release all memory after operation
execution is completed or when operation gets cancelled/deleted via
ioctl().

This creates a bit of a problem. DMA fences are refcounted objects and
exporter never knows when importer imports a fence or puts its refcount,
so exporter never knows when fence will be destoyed, which should not
be a problem for refcounted objects, but here comes the twist...

	operation A - creates and exports out fence X
	... user-space imports fence X
	operation A - finishes execution, signals fence X
	kfree operation A, put dma_fence

DMA fences are designed to borrow spinlock that DMA fences use to
protect struct dma_fence members:

	struct dma_fence {
	        spinlock_t *lock;

	        const struct dma_fence_ops *ops;
		.....
	};

	void dma_fence_init(struct dma_fence *fence,
			const struct dma_fence_ops *ops,
			spinlock_t *lock,
			u64 context,
			u64 seqno);

So the `lock` should have at least same lifespan as the DMA fence
that borrows it, which is impossible to guarantee in our case. When
we kfree operation A struct we also kfree ->lock that operation
lends to DMA fence, which outlives operation A (depending on what
fence importers do and when they drop imported fence refcount).

This patch adds a new memnber to struct dma_fence: __lock_inplace.
Which is a lock that DMA fence will use to protect its own data when
it cannot reliably borrow a lock from the outside object.

I also had a patch that puts inplace and borrowed locks to an unnamed
uninon and adds one more dma_fence_flag_bits to distinguish between
fences with borrowed and inplace locks

	struct dma_fence {
		uninon {
			spinlock_t *lock;
			spinlock_t __lock_inplace;
		};
		...
	};

And then instead of locking/unlocking ->lock directly we would use
dma_fence_lock_irqsave()/dma_fence_unlock_irqrestore() macros which
would check fence flags and either use borrowed lock or inplace lock.
But after seeing how owten drivers directly access fence ->lock I
decided to scratch that approach and just add extra spinlock member.

Not-Yet-Signed-off-by: Sergey Senozhatsky <senozhatsky@...omium.org>
---
 drivers/dma-buf/dma-fence.c | 10 ++++++++++
 include/linux/dma-fence.h   |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 066400ed8841..7ae40b8adb73 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -958,3 +958,13 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
+
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno)
+{
+	spin_lock_init(&fence->__lock_inplace);
+
+	dma_fence_init(fence, ops, &fence->__lock_inplace, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_inplace_lock_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 1ea691753bd3..6b15a0d2eccf 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -64,6 +64,8 @@ struct dma_fence_cb;
  */
 struct dma_fence {
 	spinlock_t *lock;
+	spinlock_t __lock_inplace;
+
 	const struct dma_fence_ops *ops;
 	/*
 	 * We clear the callback list on kref_put so that by the time we
@@ -262,6 +264,10 @@ struct dma_fence_ops {
 void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 		    spinlock_t *lock, u64 context, u64 seqno);

+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno);
+
 void dma_fence_release(struct kref *kref);
 void dma_fence_free(struct dma_fence *fence);
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq);
-- 
2.36.1.124.g0e6072fb45-goog