dma_fence_is_signaled() contains a lockless check of the signaled-bit,
which is set under lock protection. This is a race condition which makes
the API fragile and has caused at least two bugs already, addressed in:
commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM")
It has been argued by some that this race cannot be closed for the
entire kernel through locking because this would lead to a cache line
invalidation for all parties reading that fence. While it remains
unclear why these parties don't simply let themselves be notified about
the fence being signaled instead of checking this repeatedly manually,
there is a way to preserve this behavior in any case:
This patch adds a new flag for dma_fence_init(),
DMA_FENCE_INIT_FLAG_RACY_SIGNALED_CHECK. If this flag is not set, the
spinlock is taken and dma_fence_is_signaled() becomes perfectly
synchronized: it can only return true once signaling of the fence is
completed.
Disable the racy check for everyone by default. Should some parties
really experience severe performance degredations because of this, they
can trivially re-enable the race condition for their driver by setting
the flag in dma_fence_init().
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/dma-buf/dma-fence.c | 2 ++
include/linux/dma-fence.h | 22 ++++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index f7c084926ae6..1ae8c7aade39 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1119,6 +1119,8 @@ dma_fence_init(struct dma_fence *fence, const struct
dma_fence_ops *ops,
/* Mapp the init_flags to the fence-internal flags. */
if (test_bit(DMA_FENCE_INIT_FLAG_SEQNO64, &init_flags))
set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence_flag_bits);
+ if (test_bit(DMA_FENCE_INIT_FLAG_RACY_SIGNALED_CHECK, &init_flags))
+ set_bit(DMA_FENCE_FLAG_RACY_SIGNALED_CHECK_BIT,
&fence_flag_bits);
__dma_fence_init(fence, ops, lock, context, seqno, fence_flag_bits);
}
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 52e7ddfd2d06..ee6e8de3ebc6 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -115,6 +115,7 @@ enum dma_fence_flag_bits {
DMA_FENCE_FLAG_SIGNALED_BIT,
DMA_FENCE_FLAG_TIMESTAMP_BIT,
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+ DMA_FENCE_FLAG_RACY_SIGNALED_CHECK_BIT,
DMA_FENCE_FLAG_USER_BITS, /* must always be last member */
};
@@ -124,9 +125,12 @@ enum dma_fence_flag_bits {
* These flags can be passed in dma_fence_init().
*
* @DMA_FENCE_INIT_FLAG_SEQNO64: Use 64-bit sequence numbers.
+ * @DMA_FENCE_INIT_FLAG_RACY_SIGNALED_CHECK:
+ * Legacy flag. Enables a race condition in dma_fence_is_signaled().
*/
enum dma_fence_init_flags {
DMA_FENCE_INIT_FLAG_SEQNO64,
+ DMA_FENCE_INIT_FLAG_RACY_SIGNALED_CHECK,
/* Must always be last member. */
DMA_FENCE_INIT_FLAG_NR_OF_FLAGs,
@@ -568,9 +572,27 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+ const bool racy_check = test_bit(DMA_FENCE_FLAG_RACY_SIGNALED_CHECK_BIT,
+ &fence->flags);
const struct dma_fence_ops *ops;
+ bool signaled = false;
+ unsigned long flags;
+ /*
+ * For legacy reasons, there is a race window here against parties
+ * signaling the fence. They might still be doing that right now.
+ *
+ * For most users, this window shall be closed, but there are parties
+ * for whom the cache-line invalidation by taking the lock might be so
+ * significant that they prefer to accept the race at their own risk.
+ */
+ if (!racy_check)
+ dma_fence_lock_irqsave(fence, flags);
if (dma_fence_test_signaled_flag(fence))
+ signaled = true;
+ if (!racy_check)
+ dma_fence_unlock_irqrestore(fence, flags);
+ if (signaled)
return true;
rcu_read_lock();
--
2.56.0
