[PATCH 2/2] dma-buf: add warning when dma_fence is signaled from IOCTL

[Christian König]

From: Christian König <ckoenig at able.fritz.box>

We have the re-occurring problem that people try to invent a
DMA-fences implementation which signals fences based on an userspace
IOCTL.

This is well known as source of hard to track down crashes and is
documented to be an invalid approach. The problem is that it seems
to work during initial testing and only long term tests points out
why this can never work correctly.

So give at least a warning when people try to signal a fence from
task context and not from interrupts or a work item. This check is
certainly not perfect but better than nothing.

Signed-off-by: Christian König <ckoenig at able.fritz.box>
---
 drivers/dma-buf/dma-fence.c | 59 +++++++++++++++++++++++++++----------
 include/linux/dma-fence.h   |  9 ++++--
 2 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 3f78c56b58dc..2bce620eacac 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -345,33 +345,23 @@ void __dma_fence_might_wait(void)
 }
 #endif
 
-
 /**
- * dma_fence_signal_timestamp_locked - signal completion of a fence
+ * dma_fence_signal_internal - internal signal completion of a fence
  * @fence: the fence to signal
  * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain
  *
- * Signal completion for software callbacks on a fence, this will unblock
- * dma_fence_wait() calls and run all the callbacks added with
- * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from the unsignaled to the signaled state and not back, it will
- * only be effective the first time. Set the timestamp provided as the fence
- * signal timestamp.
- *
- * Unlike dma_fence_signal_timestamp(), this function must be called with
- * &dma_fence.lock held.
+ * Internal signal the dma_fence without error checking. Should *NEVER* be used
+ * by drivers or external code directly.
  *
  * Returns 0 on success and a negative error value when @fence has been
  * signalled already.
  */
-int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
-				      ktime_t timestamp)
+int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp)
 {
 	struct dma_fence_cb *cur, *tmp;
 	struct list_head cb_list;
 
 	lockdep_assert_held(fence->lock);
-
 	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
 				      &fence->flags)))
 		return -EINVAL;
@@ -390,7 +380,46 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	return 0;
 }
-EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
+EXPORT_SYMBOL(dma_fence_signal_internal);
+
+/**
+ * dma_fence_signal_timestamp_locked - signal completion of a fence
+ * @fence: the fence to signal
+ * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain
+ *
+ * Signal completion for software callbacks on a fence, this will unblock
+ * dma_fence_wait() calls and run all the callbacks added with
+ * dma_fence_add_callback(). Can be called multiple times, but since a fence
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time. Set the timestamp provided as the fence
+ * signal timestamp.
+ *
+ * Unlike dma_fence_signal_timestamp(), this function must be called with
+ * &dma_fence.lock held.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
+ */
+int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
+				      ktime_t timestamp)
+{
+	/*
+	 * We have the re-occurring problem that people try to invent a
+	 * DMA-fences implementation which signals fences based on an userspace
+	 * IOCTL.
+	 *
+	 * This is well known as source of hard to track down crashes and is
+	 * documented to be an invalid approach. The problem is that it seems
+	 * to work during initial testing and only long term tests points out
+	 * why this can never work correctly.
+	 *
+	 * So give at least a warning when people try to signal a fence from
+	 * task context and not from interrupts or a work item. This check is
+	 * certainly not perfect but better than nothing.
+	 */
+	WARN_ON_ONCE(!in_interrupt() && !current_work());
+	return dma_fence_signal_internal(fence, timestamp);
+}
 
 /**
  * dma_fence_signal_timestamp - signal completion of a fence
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 64639e104110..8dbcd66989b8 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -369,6 +369,7 @@ int dma_fence_signal_locked(struct dma_fence *fence);
 int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
 int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 				      ktime_t timestamp);
+int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp);
 signed long dma_fence_default_wait(struct dma_fence *fence,
 				   bool intr, signed long timeout);
 int dma_fence_add_callback(struct dma_fence *fence,
@@ -422,7 +423,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
-		dma_fence_signal_locked(fence);
+		dma_fence_signal_internal(fence, ktime_get());
 		return true;
 	}
 
@@ -448,11 +449,15 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
+	unsigned long flags;
+
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
-		dma_fence_signal(fence);
+		spin_lock_irqsave(fence->lock, flags);
+		dma_fence_signal_internal(fence, ktime_get());
+		spin_unlock_irqrestore(fence->lock, flags);
 		return true;
 	}
 
-- 
2.43.0