[RFC v2 00/13] Some (drm_sched_|dma_)fence lifetime issues

[RFC v2 00/13] Some (drm_sched_|dma_)fence lifetime issues

[Tvrtko Ursulin]

Hi all,

tl;dr;
Xe and probably some other drivers can tear down the internal state referenced
by an exported sync_file fence which then causes a null pointer derefences on
accessing said fence.

IGT that exploits the problem:
https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2

It seems there is a consensus this is a known problem with the dma-fence design,
where internal state shouldn't really be accessed after the fence has been
signaled. However currently the code is mostly unaware of that hence the use-
after-free potential.

To fix it, between the option of adding more reference counting and trying to
"revoke" the fence, suggestion is to focus on the later.

Reference to the recent discussion:
https://lore.kernel.org/dri-devel/20250418164246.72426-1-tvrtko.ursulin@igalia.com/

This series therefore attempts to implement a solution along those lines.

Most of the description what and how can be found in:
 "dma-fence: Add safe access helpers and document the rules"

Other than that, the series starts with some cleanups, with the general goal of
hiding more of the dma-fence implementation details behind explicit API. This
means adding helpers for access to driver and timeline name, and also moving as
much as it is easily possible of driver allocated state into the fence object
itself. Because dma-fence is already reference counted, any state we can embed
automatically becomes safe.

The moved state refers to the patches that move the 64bit seqno, dma-fence-array
and dma-fence-chain disambiguation into the fence. Again, it is better for as
much of the state to be directly embedded. And since we have plenty of free
flags, the parts I moved are all for free. No increase in struct dma_fence size.

Having said that, the series only addreses the runtime use-after-free scenarios,
such as the above explained situation with the xe driver. For now the module
unload problem is deliberately left for later. (Although again, some of the
early patches do make it safer, and will make future improvements easier due
fewer accesses to fence->ops.)

Final patch in the series is the one which makes xe compliant with the rules
and API proposed earlier in the series. It does so by ensuring there is at least
one RCU grace period between fences being signaled and driver allocated memory
accessible from xe fences getting freed. Which couples with the earlier (from
the series) refactors which added dma_fence_access_begin/end() protection to
the relevant call sites.

If this approach is acceptable the next steps will be to see if any other
drivers will need similar changes for which there are asserts in the new
dma-fence API which will help catch them.

And also to discuss whether we want to go a step futher and later move to SRCU,
so code would be protected against module unload as well.

v2:
 * Dropped module unload handling.
 * Proposing real API instead of hacks.

Tvrtko Ursulin (13):
  drm/i915: Use provided dma_fence_is_chain
  dma-fence: Change signature of __dma_fence_is_later
  dma-fence: Use a flag for 64-bit seqnos
  dma-fence: Move array and chain checks to flags
  dma-fence: Add helpers for accessing driver and timeline name
  dma-fence: Use driver and timeline name helpers internally
  sync_file: Use dma-fence driver and timeline name helpers
  drm/amdgpu: Use dma-fence driver and timeline name helpers
  drm/i915: Use dma-fence driver and timeline name helpers
  dma-fence: Add safe access helpers and document the rules
  sync_file: Protect access to driver and timeline name
  drm/i915: Protect access to driver and timeline name
  drm/xe: Make dma-fences compliant with the safe access rules

 drivers/dma-buf/dma-fence-array.c             |  2 +-
 drivers/dma-buf/dma-fence-chain.c             |  9 +-
 drivers/dma-buf/dma-fence.c                   | 87 ++++++++++++++++++-
 drivers/dma-buf/sw_sync.c                     |  2 +-
 drivers/dma-buf/sync_file.c                   | 14 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h     |  2 +-
 .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  |  5 +-
 drivers/gpu/drm/i915/gem/i915_gem_wait.c      |  7 +-
 drivers/gpu/drm/i915/gt/intel_gt_requests.c   |  6 +-
 drivers/gpu/drm/i915/i915_request.c           |  5 +-
 drivers/gpu/drm/i915/i915_sw_fence.c          |  6 +-
 drivers/gpu/drm/xe/xe_guc_exec_queue_types.h  |  2 +
 drivers/gpu/drm/xe/xe_guc_submit.c            |  7 +-
 drivers/gpu/drm/xe/xe_hw_fence.c              |  5 +-
 drivers/gpu/drm/xe/xe_sched_job.c             | 14 +--
 include/linux/dma-fence.h                     | 48 +++++++---
 include/trace/events/dma_fence.h              |  4 +-
 17 files changed, 174 insertions(+), 51 deletions(-)

-- 
2.48.0

[RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

[Tvrtko Ursulin]

Replace open-coded helper with the subsystem one.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 7127e90c1a8f..991666fd9f85 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence,
 	rcu_read_unlock();
 }
 
-static inline bool __dma_fence_is_chain(const struct dma_fence *fence)
-{
-	return fence->ops == &dma_fence_chain_ops;
-}
-
 void i915_gem_fence_wait_priority(struct dma_fence *fence,
 				  const struct i915_sched_attr *attr)
 {
@@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence,
 
 		for (i = 0; i < array->num_fences; i++)
 			fence_set_priority(array->fences[i], attr);
-	} else if (__dma_fence_is_chain(fence)) {
+	} else if (dma_fence_is_chain(fence)) {
 		struct dma_fence *iter;
 
 		/* The chain is ordered; if we boost the last, we boost all */
-- 
2.48.0

[RFC v2 02/13] dma-fence: Change signature of __dma_fence_is_later

[Tvrtko Ursulin]

With the goal of reducing the need for drivers to touch (and dereference)
fence->ops, we change the prototype of __dma_fence_is_later() to take
fence instead of fence->ops.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/dma-fence-chain.c |  2 +-
 drivers/dma-buf/sw_sync.c         |  2 +-
 drivers/gpu/drm/xe/xe_hw_fence.c  |  2 +-
 drivers/gpu/drm/xe/xe_sched_job.c | 14 ++++++++------
 include/linux/dma-fence.h         |  9 ++++-----
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index 9663ba1bb6ac..90424f23fd73 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -252,7 +252,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 	chain->prev_seqno = 0;
 
 	/* Try to reuse the context of the previous chain node. */
-	if (prev_chain && __dma_fence_is_later(seqno, prev->seqno, prev->ops)) {
+	if (prev_chain && __dma_fence_is_later(prev, seqno, prev->seqno)) {
 		context = prev->context;
 		chain->prev_seqno = prev->seqno;
 	} else {
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 4f27ee93a00c..3c20f1d31cf5 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -170,7 +170,7 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
 {
 	struct sync_timeline *parent = dma_fence_parent(fence);
 
-	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
+	return !__dma_fence_is_later(fence, fence->seqno, parent->value);
 }
 
 static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
index 0b4f12be3692..03eb8c6d1616 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.c
+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
@@ -165,7 +165,7 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence)
 	u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32);
 
 	return dma_fence->error ||
-		!__dma_fence_is_later(dma_fence->seqno, seqno, dma_fence->ops);
+		!__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno);
 }
 
 static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence)
diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
index 1905ca590965..f0a6ce610948 100644
--- a/drivers/gpu/drm/xe/xe_sched_job.c
+++ b/drivers/gpu/drm/xe/xe_sched_job.c
@@ -216,15 +216,17 @@ void xe_sched_job_set_error(struct xe_sched_job *job, int error)
 
 bool xe_sched_job_started(struct xe_sched_job *job)
 {
+	struct dma_fence *fence = dma_fence_chain_contained(job->fence);
 	struct xe_lrc *lrc = job->q->lrc[0];
 
-	return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job),
-				     xe_lrc_start_seqno(lrc),
-				     dma_fence_chain_contained(job->fence)->ops);
+	return !__dma_fence_is_later(fence,
+				     xe_sched_job_lrc_seqno(job),
+				     xe_lrc_start_seqno(lrc));
 }
 
 bool xe_sched_job_completed(struct xe_sched_job *job)
 {
+	struct dma_fence *fence = dma_fence_chain_contained(job->fence);
 	struct xe_lrc *lrc = job->q->lrc[0];
 
 	/*
@@ -232,9 +234,9 @@ bool xe_sched_job_completed(struct xe_sched_job *job)
 	 * parallel handshake is done.
 	 */
 
-	return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job),
-				     xe_lrc_seqno(lrc),
-				     dma_fence_chain_contained(job->fence)->ops);
+	return !__dma_fence_is_later(fence,
+				     xe_sched_job_lrc_seqno(job),
+				     xe_lrc_seqno(lrc));
 }
 
 void xe_sched_job_arm(struct xe_sched_job *job)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index b12776883d14..48b5202c531d 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -441,21 +441,20 @@ dma_fence_is_signaled(struct dma_fence *fence)
 
 /**
  * __dma_fence_is_later - return if f1 is chronologically later than f2
+ * @fence: fence in whose context to do the comparison
  * @f1: the first fence's seqno
  * @f2: the second fence's seqno from the same context
- * @ops: dma_fence_ops associated with the seqno
  *
  * Returns true if f1 is chronologically later than f2. Both fences must be
  * from the same context, since a seqno is not common across contexts.
  */
-static inline bool __dma_fence_is_later(u64 f1, u64 f2,
-					const struct dma_fence_ops *ops)
+static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
 {
 	/* This is for backward compatibility with drivers which can only handle
 	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
 	 * do so.
 	 */
-	if (ops->use_64bit_seqno)
+	if (fence->ops->use_64bit_seqno)
 		return f1 > f2;
 
 	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;
@@ -475,7 +474,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1,
 	if (WARN_ON(f1->context != f2->context))
 		return false;
 
-	return __dma_fence_is_later(f1->seqno, f2->seqno, f1->ops);
+	return __dma_fence_is_later(f1, f1->seqno, f2->seqno);
 }
 
 /**
-- 
2.48.0

[RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

[Tvrtko Ursulin]

With the goal of reducing the need for drivers to touch (and dereference)
fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to
the fence->flags.

Drivers which were setting this flag are changed to use new
dma_fence_init64() instead of dma_fence_init().

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/dma-fence-chain.c                | 5 ++---
 drivers/dma-buf/dma-fence.c                      | 9 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++---
 include/linux/dma-fence.h                        | 6 +++++-
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index 90424f23fd73..a8a90acf4f34 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence,
 }
 
 const struct dma_fence_ops dma_fence_chain_ops = {
-	.use_64bit_seqno = true,
 	.get_driver_name = dma_fence_chain_get_driver_name,
 	.get_timeline_name = dma_fence_chain_get_timeline_name,
 	.enable_signaling = dma_fence_chain_enable_signaling,
@@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 			seqno = max(prev->seqno, seqno);
 	}
 
-	dma_fence_init(&chain->base, &dma_fence_chain_ops,
-		       &chain->lock, context, seqno);
+	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
+			 context, seqno);
 
 	/*
 	 * Chaining dma_fence_chain container together is only allowed through
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index f0cdd3e99d36..33661658f684 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1023,3 +1023,12 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
+		 spinlock_t *lock, u64 context, u64 seqno)
+{
+	dma_fence_init(fence, ops, lock, context, seqno);
+	__set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
+}
+EXPORT_SYMBOL(dma_fence_init64);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
index 51cddfa3f1e8..5d26797356a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
@@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work)
 }
 
 static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
-	.use_64bit_seqno = true,
 	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
 	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
 };
@@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm
 	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
 	spin_lock_init(&f->lock);
 
-	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
-		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
+	dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
+			 vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
 
 	/* TODO: We probably need a separate wq here */
 	dma_fence_get(&f->base);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 48b5202c531d..ac6535716dbe 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -97,6 +97,7 @@ struct dma_fence {
 };
 
 enum dma_fence_flag_bits {
+	DMA_FENCE_FLAG_SEQNO64_BIT,
 	DMA_FENCE_FLAG_SIGNALED_BIT,
 	DMA_FENCE_FLAG_TIMESTAMP_BIT,
 	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
@@ -262,6 +263,9 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
+		      spinlock_t *lock, 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);
@@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
 	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
 	 * do so.
 	 */
-	if (fence->ops->use_64bit_seqno)
+	if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags))
 		return f1 > f2;
 
 	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;
-- 
2.48.0

[RFC v2 04/13] dma-fence: Move array and chain checks to flags

[Tvrtko Ursulin]

With the goal of reducing the need for drivers to touch fence->ops, we
add explicit flags for struct dma_fence_array and struct dma_fence_chain
and make the respective helpers (dma_fence_is_array() and
dma_fence_is_chain()) use them.

This also allows us to remove the exported symbols for the respective
operation tables.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/dma-fence-array.c | 2 +-
 drivers/dma-buf/dma-fence-chain.c | 2 +-
 include/linux/dma-fence.h         | 9 ++++-----
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 6657d4b30af9..daf444f5d228 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
 	.release = dma_fence_array_release,
 	.set_deadline = dma_fence_array_set_deadline,
 };
-EXPORT_SYMBOL(dma_fence_array_ops);
 
 /**
  * dma_fence_array_alloc - Allocate a custom fence array
@@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array,
 	spin_lock_init(&array->lock);
 	dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
 		       context, seqno);
+	__set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags);
 	init_irq_work(&array->work, irq_dma_fence_array_work);
 
 	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index a8a90acf4f34..f4abe41fb092 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = {
 	.release = dma_fence_chain_release,
 	.set_deadline = dma_fence_chain_set_deadline,
 };
-EXPORT_SYMBOL(dma_fence_chain_ops);
 
 /**
  * dma_fence_chain_init - initialize a fence chain
@@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 
 	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
 			 context, seqno);
+	__set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags);
 
 	/*
 	 * Chaining dma_fence_chain container together is only allowed through
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index ac6535716dbe..5bafd0a5f1f1 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -98,6 +98,8 @@ struct dma_fence {
 
 enum dma_fence_flag_bits {
 	DMA_FENCE_FLAG_SEQNO64_BIT,
+	DMA_FENCE_FLAG_ARRAY_BIT,
+	DMA_FENCE_FLAG_CHAIN_BIT,
 	DMA_FENCE_FLAG_SIGNALED_BIT,
 	DMA_FENCE_FLAG_TIMESTAMP_BIT,
 	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
@@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void);
 struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp);
 u64 dma_fence_context_alloc(unsigned num);
 
-extern const struct dma_fence_ops dma_fence_array_ops;
-extern const struct dma_fence_ops dma_fence_chain_ops;
-
 /**
  * dma_fence_is_array - check if a fence is from the array subclass
  * @fence: the fence to test
@@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
  */
 static inline bool dma_fence_is_array(struct dma_fence *fence)
 {
-	return fence->ops == &dma_fence_array_ops;
+	return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags);
 }
 
 /**
@@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
  */
 static inline bool dma_fence_is_chain(struct dma_fence *fence)
 {
-	return fence->ops == &dma_fence_chain_ops;
+	return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags);
 }
 
 /**
-- 
2.48.0

[RFC v2 05/13] dma-fence: Add helpers for accessing driver and timeline name

[Tvrtko Ursulin]

Add some helpers in order to enable preventing dma-fence users accessing
the implementation details directly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 include/linux/dma-fence.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 5bafd0a5f1f1..c814a86087f8 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -387,6 +387,16 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
 			       struct dma_fence_cb *cb);
 void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 
+static inline const char *dma_fence_driver_name(struct dma_fence *fence)
+{
+	return fence->ops->get_driver_name(fence);
+}
+
+static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
+{
+	return fence->ops->get_timeline_name(fence);
+}
+
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
  *                                is signaled yet.
-- 
2.48.0

[RFC v2 06/13] dma-fence: Use driver and timeline name helpers internally

[Tvrtko Ursulin]

Make the implementation use the previously added helper for accessing the
driver and timeline name. This will enable more coverage later when
asserts will be added into them.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/dma-fence.c      | 9 +++++----
 include/trace/events/dma_fence.h | 4 ++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 33661658f684..dc2456f68685 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref)
 	if (WARN(!list_empty(&fence->cb_list) &&
 		 !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags),
 		 "Fence %s:%s:%llx:%llx released with pending signals!\n",
-		 fence->ops->get_driver_name(fence),
-		 fence->ops->get_timeline_name(fence),
+		 dma_fence_driver_name(fence),
+		 dma_fence_timeline_name(fence),
 		 fence->context, fence->seqno)) {
 		unsigned long flags;
 
@@ -983,8 +983,9 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
 {
 	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
-		   fence->ops->get_driver_name(fence),
-		   fence->ops->get_timeline_name(fence), fence->seqno,
+		   dma_fence_driver_name(fence),
+		   dma_fence_timeline_name(fence),
+		   fence->seqno,
 		   dma_fence_is_signaled(fence) ? "" : "un");
 }
 EXPORT_SYMBOL(dma_fence_describe);
diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
index a4de3df8500b..84c83074ee81 100644
--- a/include/trace/events/dma_fence.h
+++ b/include/trace/events/dma_fence.h
@@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence,
 	TP_ARGS(fence),
 
 	TP_STRUCT__entry(
-		__string(driver, fence->ops->get_driver_name(fence))
-		__string(timeline, fence->ops->get_timeline_name(fence))
+		__string(driver, dma_fence_driver_name(fence))
+		__string(timeline, dma_fence_timeline_name(fence))
 		__field(unsigned int, context)
 		__field(unsigned int, seqno)
 	),
-- 
2.48.0

[RFC v2 07/13] sync_file: Use dma-fence driver and timeline name helpers

[Tvrtko Ursulin]

Access the dma-fence internals via the previously added helpers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/sync_file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index d9b1c1b2a72b..212df4b849fe 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -137,8 +137,8 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
 		struct dma_fence *fence = sync_file->fence;
 
 		snprintf(buf, len, "%s-%s%llu-%lld",
-			 fence->ops->get_driver_name(fence),
-			 fence->ops->get_timeline_name(fence),
+			 dma_fence_driver_name(fence),
+			 dma_fence_timeline_name(fence),
 			 fence->context,
 			 fence->seqno);
 	}
@@ -262,9 +262,9 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
 static int sync_fill_fence_info(struct dma_fence *fence,
 				 struct sync_fence_info *info)
 {
-	strscpy(info->obj_name, fence->ops->get_timeline_name(fence),
+	strscpy(info->obj_name, dma_fence_timeline_name(fence),
 		sizeof(info->obj_name));
-	strscpy(info->driver_name, fence->ops->get_driver_name(fence),
+	strscpy(info->driver_name, dma_fence_driver_name(fence),
 		sizeof(info->driver_name));
 
 	info->status = dma_fence_get_status(fence);
-- 
2.48.0

[RFC v2 08/13] drm/amdgpu: Use dma-fence driver and timeline name helpers

[Tvrtko Ursulin]

Access the dma-fence internals via the previously added helpers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 11dd2e0f7979..8e5bf179a6c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -33,7 +33,7 @@
 #define TRACE_INCLUDE_FILE amdgpu_trace
 
 #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
-	 job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished)
+	 dma_fence_timeline_name(&job->base.s_fence->finished)
 
 TRACE_EVENT(amdgpu_device_rreg,
 	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
-- 
2.48.0

[RFC v2 09/13] drm/i915: Use dma-fence driver and timeline name helpers

[Tvrtko Ursulin]

Access the dma-fence internals via the previously added helpers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 4 ++--
 drivers/gpu/drm/i915/i915_request.c         | 2 +-
 drivers/gpu/drm/i915/i915_sw_fence.c        | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index d1a382dfaa1d..ae3557ed6c1e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -252,8 +252,8 @@ void intel_gt_watchdog_work(struct work_struct *work)
 			struct dma_fence *f = &rq->fence;
 
 			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
-				  f->ops->get_driver_name(f),
-				  f->ops->get_timeline_name(f),
+				  dma_fence_driver_name(f),
+				  dma_fence_timeline_name(f),
 				  f->seqno);
 			i915_request_cancel(rq, -EINTR);
 		}
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index c3d27eadc0a7..4874c4f1e4ab 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2184,7 +2184,7 @@ void i915_request_show(struct drm_printer *m,
 		       const char *prefix,
 		       int indent)
 {
-	const char *name = rq->fence.ops->get_timeline_name((struct dma_fence *)&rq->fence);
+	const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
 	char buf[80] = "";
 	int x = 0;
 
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 1d4cc91c0e40..e51ca7e50a4e 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -435,8 +435,8 @@ static void timer_i915_sw_fence_wake(struct timer_list *t)
 		return;
 
 	pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n",
-		  cb->dma->ops->get_driver_name(cb->dma),
-		  cb->dma->ops->get_timeline_name(cb->dma),
+		  dma_fence_driver_name(cb->dma),
+		  dma_fence_timeline_name(cb->dma),
 		  cb->dma->seqno,
 		  i915_sw_fence_debug_hint(fence));
 
-- 
2.48.0

[RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Tvrtko Ursulin]

Dma-fence objects currently suffer from a potential use after free problem
where fences exported to userspace and other drivers can outlive the
exporting driver, or the associated data structures.

The discussion on how to address this concluded that adding reference
counting to all the involved objects is not desirable, since it would need
to be very wide reaching and could cause unloadable drivers if another
entity would be holding onto a signaled fence reference potentially
indefinitely.

This patch enables the safe access by introducing and documenting a
contract between fence exporters and users. It documents a set of
contraints and adds helpers which a) drivers with potential to suffer from
the use after free must use and b) users of the dma-fence API must use as
well.

Premise of the design has multiple sides:

1. Drivers (fence exporters) MUST ensure a RCU grace period between
signalling a fence and freeing the driver private data associated with it.

The grace period does not have to follow the signalling immediately but
HAS to happen before data is freed.

2. Users of the dma-fence API marked with such requirement MUST contain
the complete access to the data within a single code block guarded by the
new dma_fence_access_begin() and dma_fence_access_end() helpers.

The combination of the two ensures that whoever sees the
DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
valid fence->lock and valid data potentially accessed by the fence->ops
virtual functions, until the call to dma_fence_access_end().

3. Module unload (fence->ops) disappearing is for now explicitly not
handled. That would required a more complex protection, possibly needing
SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
where race between dma_fence_enable_sw_signaling, signalling, and
dereference of fence->ops->wait() would need a sleeping SRCU context.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
 include/linux/dma-fence.h   | 32 ++++++++++++-----
 2 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index dc2456f68685..cfe1d7b79c22 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
 	struct dma_fence *fence =
 		container_of(kref, struct dma_fence, refcount);
 
+	dma_fence_access_begin();
 	trace_dma_fence_destroy(fence);
 
 	if (WARN(!list_empty(&fence->cb_list) &&
@@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
 		fence->ops->release(fence);
 	else
 		dma_fence_free(fence);
+
+	dma_fence_access_end();
 }
 EXPORT_SYMBOL(dma_fence_release);
 
@@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
  */
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
 {
+	dma_fence_access_begin();
 	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
 		   dma_fence_driver_name(fence),
 		   dma_fence_timeline_name(fence),
 		   fence->seqno,
 		   dma_fence_is_signaled(fence) ? "" : "un");
+	dma_fence_access_end();
 }
 EXPORT_SYMBOL(dma_fence_describe);
 
@@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	__set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
 }
 EXPORT_SYMBOL(dma_fence_init64);
+
+/**
+ * dma_fence_driver_name - Access the driver name
+ * @fence: the fence to query
+ *
+ * Returns a driver name backing the dma-fence implementation.
+ *
+ * IMPORTANT CONSIDERATION:
+ * Dma-fence contract stipulates that access to driver provided data (data not
+ * directly embedded into the object itself), such as the &dma_fence.lock and
+ * memory potentially accessed by the &dma_fence.ops functions, is forbidden
+ * after the fence has been signalled. Drivers are allowed to free that data,
+ * and some do.
+ *
+ * To allow safe access drivers are mandated to guarantee a RCU grace period
+ * between signalling the fence and freeing said data.
+ *
+ * As such access to the driver name is only valid inside a RCU locked section.
+ * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
+ * by the &dma_fence_access_being and &dma_fence_access_end pair.
+ */
+const char *dma_fence_driver_name(struct dma_fence *fence)
+{
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "rcu_read_lock() required for safe access to returned string");
+
+	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+		return fence->ops->get_driver_name(fence);
+	else
+		return "detached-driver";
+}
+EXPORT_SYMBOL(dma_fence_driver_name);
+
+/**
+ * dma_fence_timeline_name - Access the timeline name
+ * @fence: the fence to query
+ *
+ * Returns a timeline name provided by the dma-fence implementation.
+ *
+ * IMPORTANT CONSIDERATION:
+ * Dma-fence contract stipulates that access to driver provided data (data not
+ * directly embedded into the object itself), such as the &dma_fence.lock and
+ * memory potentially accessed by the &dma_fence.ops functions, is forbidden
+ * after the fence has been signalled. Drivers are allowed to free that data,
+ * and some do.
+ *
+ * To allow safe access drivers are mandated to guarantee a RCU grace period
+ * between signalling the fence and freeing said data.
+ *
+ * As such access to the driver name is only valid inside a RCU locked section.
+ * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
+ * by the &dma_fence_access_being and &dma_fence_access_end pair.
+ */
+const char *dma_fence_timeline_name(struct dma_fence *fence)
+{
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "rcu_read_lock() required for safe access to returned string");
+
+	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+		return fence->ops->get_driver_name(fence);
+	else
+		return "signaled-timeline";
+}
+EXPORT_SYMBOL(dma_fence_timeline_name);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index c814a86087f8..c8a9915eb360 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -387,15 +387,31 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
 			       struct dma_fence_cb *cb);
 void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 
-static inline const char *dma_fence_driver_name(struct dma_fence *fence)
-{
-	return fence->ops->get_driver_name(fence);
-}
+/**
+ * DOC: Safe external access to driver provided object members
+ *
+ * All data not stored directly in the dma-fence object, such as the
+ * &dma_fence.lock and memory potentially accessed by functions in the
+ * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled
+ * because after that point drivers are allowed to free it.
+ *
+ * All code accessing that data via the dma-fence API (or directly, which is
+ * discouraged), MUST make sure to contain the complete access within a
+ * &dma_fence_access_begin and &dma_fence_access_end pair.
+ *
+ * Some dma-fence API handles this automatically, while other, as for example
+ * &dma_fence_driver_name and &dma_fence_timeline_name, leave that
+ * responsibility to the caller.
+ *
+ * To enable this scheme to work drivers MUST ensure a RCU grace period elapses
+ * between signalling the fence and freeing the said data.
+ *
+ */
+#define dma_fence_access_begin	rcu_read_lock
+#define dma_fence_access_end	rcu_read_unlock
 
-static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
-{
-	return fence->ops->get_timeline_name(fence);
-}
+const char *dma_fence_driver_name(struct dma_fence *fence);
+const char *dma_fence_timeline_name(struct dma_fence *fence);
 
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
-- 
2.48.0

[RFC v2 11/13] sync_file: Protect access to driver and timeline name

[Tvrtko Ursulin]

Protect the access to driver and timeline name which otherwise could be
freed as dma-fence exported is signalling fences.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/dma-buf/sync_file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 212df4b849fe..ad87116baa24 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -136,11 +136,13 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
 	} else {
 		struct dma_fence *fence = sync_file->fence;
 
+		dma_fence_access_begin();
 		snprintf(buf, len, "%s-%s%llu-%lld",
 			 dma_fence_driver_name(fence),
 			 dma_fence_timeline_name(fence),
 			 fence->context,
 			 fence->seqno);
+		dma_fence_access_end();
 	}
 
 	return buf;
@@ -262,6 +264,8 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
 static int sync_fill_fence_info(struct dma_fence *fence,
 				 struct sync_fence_info *info)
 {
+	dma_fence_access_begin();
+
 	strscpy(info->obj_name, dma_fence_timeline_name(fence),
 		sizeof(info->obj_name));
 	strscpy(info->driver_name, dma_fence_driver_name(fence),
@@ -273,6 +277,8 @@ static int sync_fill_fence_info(struct dma_fence *fence,
 			ktime_to_ns(dma_fence_timestamp(fence)) :
 			ktime_set(0, 0);
 
+	dma_fence_access_end();
+
 	return info->status;
 }
 
-- 
2.48.0

[RFC v2 12/13] drm/i915: Protect access to driver and timeline name

[Tvrtko Ursulin]

Protect the access to driver and timeline name which otherwise could be
freed as dma-fence exported is signalling fences.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 ++
 drivers/gpu/drm/i915/i915_request.c         | 5 +++--
 drivers/gpu/drm/i915/i915_sw_fence.c        | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index ae3557ed6c1e..11fca24c8b5b 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -251,10 +251,12 @@ void intel_gt_watchdog_work(struct work_struct *work)
 		if (!i915_request_completed(rq)) {
 			struct dma_fence *f = &rq->fence;
 
+			dma_fence_access_begin();
 			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
 				  dma_fence_driver_name(f),
 				  dma_fence_timeline_name(f),
 				  f->seqno);
+			dma_fence_access_end();
 			i915_request_cancel(rq, -EINTR);
 		}
 		i915_request_put(rq);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4874c4f1e4ab..8008b7767641 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2184,7 +2184,6 @@ void i915_request_show(struct drm_printer *m,
 		       const char *prefix,
 		       int indent)
 {
-	const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
 	char buf[80] = "";
 	int x = 0;
 
@@ -2220,6 +2219,7 @@ void i915_request_show(struct drm_printer *m,
 
 	x = print_sched_attr(&rq->sched.attr, buf, x, sizeof(buf));
 
+	dma_fence_access_begin();
 	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
 		   prefix, indent, "                ",
 		   queue_status(rq),
@@ -2228,7 +2228,8 @@ void i915_request_show(struct drm_printer *m,
 		   fence_status(rq),
 		   buf,
 		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
-		   name);
+		   dma_fence_timeline_name((struct dma_fence *)&rq->fence));
+	dma_fence_access_end();
 }
 
 static bool engine_match_ring(struct intel_engine_cs *engine, struct i915_request *rq)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index e51ca7e50a4e..e7bdc1165b90 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -434,11 +434,13 @@ static void timer_i915_sw_fence_wake(struct timer_list *t)
 	if (!fence)
 		return;
 
+	dma_fence_access_begin();
 	pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n",
 		  dma_fence_driver_name(cb->dma),
 		  dma_fence_timeline_name(cb->dma),
 		  cb->dma->seqno,
 		  i915_sw_fence_debug_hint(fence));
+	dma_fence_access_end();
 
 	i915_sw_fence_set_error_once(fence, -ETIMEDOUT);
 	i915_sw_fence_complete(fence);
-- 
2.48.0

[RFC v2 13/13] drm/xe: Make dma-fences compliant with the safe access rules

[Tvrtko Ursulin]

Xe can free some of the data pointed to by the dma-fences it exports. Most
notably the timeline name can get freed if userspace closes the associated
submit queue. At the same time the fence could have been exported to a
third party (for example a sync_fence fd) which will then cause an use-
after-free on subsequent access.

To make this safe we need to make the driver compliant with the newly
documented dma-fence rules. Driver has to ensure a RCU grace period
between signalling a fence and freeing any data pointed to by said fence.

For the timeline name we simply make the queue be freed via kfree_rcu and
for the shared lock associated with multiple queues we add a RCU grace
period before freeing the per GT structure holding the lock.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
 drivers/gpu/drm/xe/xe_guc_exec_queue_types.h | 2 ++
 drivers/gpu/drm/xe/xe_guc_submit.c           | 7 ++++++-
 drivers/gpu/drm/xe/xe_hw_fence.c             | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
index 4c39f01e4f52..a3f421e2adc0 100644
--- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
@@ -20,6 +20,8 @@ struct xe_exec_queue;
 struct xe_guc_exec_queue {
 	/** @q: Backpointer to parent xe_exec_queue */
 	struct xe_exec_queue *q;
+	/** @rcu: For safe freeing of exported dma fences */
+	struct rcu_head rcu;
 	/** @sched: GPU scheduler for this xe_exec_queue */
 	struct xe_gpu_scheduler sched;
 	/** @entity: Scheduler entity for this xe_exec_queue */
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 369be36f7dc5..cda837ff0118 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1274,7 +1274,11 @@ static void __guc_exec_queue_fini_async(struct work_struct *w)
 	xe_sched_entity_fini(&ge->entity);
 	xe_sched_fini(&ge->sched);
 
-	kfree(ge);
+	/*
+	 * RCU free due sched being exported via DRM scheduler fences
+	 * (timeline name).
+	 */
+	kfree_rcu(ge, rcu);
 	xe_exec_queue_fini(q);
 	xe_pm_runtime_put(guc_to_xe(guc));
 }
@@ -1457,6 +1461,7 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
 
 	q->guc = ge;
 	ge->q = q;
+	init_rcu_head(&ge->rcu);
 	init_waitqueue_head(&ge->suspend_wait);
 
 	for (i = 0; i < MAX_STATIC_MSG_TYPE; ++i)
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
index 03eb8c6d1616..b2a0c46dfcd4 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.c
+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
@@ -100,6 +100,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
 		spin_unlock_irqrestore(&irq->lock, flags);
 		dma_fence_end_signalling(tmp);
 	}
+
+	/* Safe release of the irq->lock used in dma_fence_init. */
+	synchronize_rcu();
 }
 
 void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
-- 
2.48.0

Re: [RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

[Matthew Brost]

On Fri, May 09, 2025 at 04:33:40PM +0100, Tvrtko Ursulin wrote:
> Replace open-coded helper with the subsystem one.
> 

You probably can just send this one by itself as it good cleanup and
independent.

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> index 7127e90c1a8f..991666fd9f85 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> @@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence,
>  	rcu_read_unlock();
>  }
>  
> -static inline bool __dma_fence_is_chain(const struct dma_fence *fence)
> -{
> -	return fence->ops == &dma_fence_chain_ops;
> -}
> -
>  void i915_gem_fence_wait_priority(struct dma_fence *fence,
>  				  const struct i915_sched_attr *attr)
>  {
> @@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence,
>  
>  		for (i = 0; i < array->num_fences; i++)
>  			fence_set_priority(array->fences[i], attr);
> -	} else if (__dma_fence_is_chain(fence)) {
> +	} else if (dma_fence_is_chain(fence)) {
>  		struct dma_fence *iter;
>  
>  		/* The chain is ordered; if we boost the last, we boost all */
> -- 
> 2.48.0
> 

Re: [RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

[Christian König]

On 5/9/25 17:47, Matthew Brost wrote:
> On Fri, May 09, 2025 at 04:33:40PM +0100, Tvrtko Ursulin wrote:
>> Replace open-coded helper with the subsystem one.
>>
> 
> You probably can just send this one by itself as it good cleanup and
> independent.
> 
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

Any objections that I start to push those patches to drm-misc-next or do you want to take this one through the i915 branch?

Regards,
Christian.

> 
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> ---
>>  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> index 7127e90c1a8f..991666fd9f85 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> @@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence,
>>  	rcu_read_unlock();
>>  }
>>  
>> -static inline bool __dma_fence_is_chain(const struct dma_fence *fence)
>> -{
>> -	return fence->ops == &dma_fence_chain_ops;
>> -}
>> -
>>  void i915_gem_fence_wait_priority(struct dma_fence *fence,
>>  				  const struct i915_sched_attr *attr)
>>  {
>> @@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence,
>>  
>>  		for (i = 0; i < array->num_fences; i++)
>>  			fence_set_priority(array->fences[i], attr);
>> -	} else if (__dma_fence_is_chain(fence)) {
>> +	} else if (dma_fence_is_chain(fence)) {
>>  		struct dma_fence *iter;
>>  
>>  		/* The chain is ordered; if we boost the last, we boost all */
>> -- 
>> 2.48.0
>>

Re: [RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

[Tvrtko Ursulin]

On 12/05/2025 09:05, Christian König wrote:
> On 5/9/25 17:47, Matthew Brost wrote:
>> On Fri, May 09, 2025 at 04:33:40PM +0100, Tvrtko Ursulin wrote:
>>> Replace open-coded helper with the subsystem one.
>>>
>>
>> You probably can just send this one by itself as it good cleanup and
>> independent.
>>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Any objections that I start to push those patches to drm-misc-next or do you want to take this one through the i915 branch?

I think it will depend on timing. If this series gets stalled, or gets 
rejected, I will push this cleanup patch to i915. But if things will be 
looking positive to merge more of this series, then it is much simpler 
to take everything via drm-misc-next and avoid branch dependencies.

Regards,

Tvrtko

>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------
>>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>>> index 7127e90c1a8f..991666fd9f85 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>>> @@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence,
>>>   	rcu_read_unlock();
>>>   }
>>>   
>>> -static inline bool __dma_fence_is_chain(const struct dma_fence *fence)
>>> -{
>>> -	return fence->ops == &dma_fence_chain_ops;
>>> -}
>>> -
>>>   void i915_gem_fence_wait_priority(struct dma_fence *fence,
>>>   				  const struct i915_sched_attr *attr)
>>>   {
>>> @@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence,
>>>   
>>>   		for (i = 0; i < array->num_fences; i++)
>>>   			fence_set_priority(array->fences[i], attr);
>>> -	} else if (__dma_fence_is_chain(fence)) {
>>> +	} else if (dma_fence_is_chain(fence)) {
>>>   		struct dma_fence *iter;
>>>   
>>>   		/* The chain is ordered; if we boost the last, we boost all */
>>> -- 
>>> 2.48.0
>>>
> 

Re: [RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

[Tvrtko Ursulin]

On 09/05/2025 16:33, Tvrtko Ursulin wrote:
> With the goal of reducing the need for drivers to touch (and dereference)
> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to
> the fence->flags.
> 
> Drivers which were setting this flag are changed to use new
> dma_fence_init64() instead of dma_fence_init().


Note that due rebase slip up I lost the hunk which actually removes the 
bool from the ops struct. Fixed locally.

Regards,

Tvrtko

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>   drivers/dma-buf/dma-fence-chain.c                | 5 ++---
>   drivers/dma-buf/dma-fence.c                      | 9 +++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++---
>   include/linux/dma-fence.h                        | 6 +++++-
>   4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index 90424f23fd73..a8a90acf4f34 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence,
>   }
>   
>   const struct dma_fence_ops dma_fence_chain_ops = {
> -	.use_64bit_seqno = true,
>   	.get_driver_name = dma_fence_chain_get_driver_name,
>   	.get_timeline_name = dma_fence_chain_get_timeline_name,
>   	.enable_signaling = dma_fence_chain_enable_signaling,
> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>   			seqno = max(prev->seqno, seqno);
>   	}
>   
> -	dma_fence_init(&chain->base, &dma_fence_chain_ops,
> -		       &chain->lock, context, seqno);
> +	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
> +			 context, seqno);
>   
>   	/*
>   	 * Chaining dma_fence_chain container together is only allowed through
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index f0cdd3e99d36..33661658f684 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -1023,3 +1023,12 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> +		 spinlock_t *lock, u64 context, u64 seqno)
> +{
> +	dma_fence_init(fence, ops, lock, context, seqno);
> +	__set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
> +}
> +EXPORT_SYMBOL(dma_fence_init64);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> index 51cddfa3f1e8..5d26797356a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work)
>   }
>   
>   static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
> -	.use_64bit_seqno = true,
>   	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
>   	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>   };
> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm
>   	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>   	spin_lock_init(&f->lock);
>   
> -	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
> -		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
> +	dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
> +			 vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>   
>   	/* TODO: We probably need a separate wq here */
>   	dma_fence_get(&f->base);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 48b5202c531d..ac6535716dbe 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -97,6 +97,7 @@ struct dma_fence {
>   };
>   
>   enum dma_fence_flag_bits {
> +	DMA_FENCE_FLAG_SEQNO64_BIT,
>   	DMA_FENCE_FLAG_SIGNALED_BIT,
>   	DMA_FENCE_FLAG_TIMESTAMP_BIT,
>   	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> @@ -262,6 +263,9 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> +		      spinlock_t *lock, 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);
> @@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
>   	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
>   	 * do so.
>   	 */
> -	if (fence->ops->use_64bit_seqno)
> +	if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags))
>   		return f1 > f2;
>   
>   	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;

Re: [RFC v2 02/13] dma-fence: Change signature of __dma_fence_is_later

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> With the goal of reducing the need for drivers to touch (and dereference)
> fence->ops, we change the prototype of __dma_fence_is_later() to take
> fence instead of fence->ops.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/dma-buf/dma-fence-chain.c |  2 +-
>  drivers/dma-buf/sw_sync.c         |  2 +-
>  drivers/gpu/drm/xe/xe_hw_fence.c  |  2 +-
>  drivers/gpu/drm/xe/xe_sched_job.c | 14 ++++++++------
>  include/linux/dma-fence.h         |  9 ++++-----
>  5 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index 9663ba1bb6ac..90424f23fd73 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -252,7 +252,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>  	chain->prev_seqno = 0;
>  
>  	/* Try to reuse the context of the previous chain node. */
> -	if (prev_chain && __dma_fence_is_later(seqno, prev->seqno, prev->ops)) {
> +	if (prev_chain && __dma_fence_is_later(prev, seqno, prev->seqno)) {
>  		context = prev->context;
>  		chain->prev_seqno = prev->seqno;
>  	} else {
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 4f27ee93a00c..3c20f1d31cf5 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -170,7 +170,7 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
>  {
>  	struct sync_timeline *parent = dma_fence_parent(fence);
>  
> -	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
> +	return !__dma_fence_is_later(fence, fence->seqno, parent->value);
>  }
>  
>  static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> index 0b4f12be3692..03eb8c6d1616 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> @@ -165,7 +165,7 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence)
>  	u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32);
>  
>  	return dma_fence->error ||
> -		!__dma_fence_is_later(dma_fence->seqno, seqno, dma_fence->ops);
> +		!__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno);
>  }
>  
>  static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence)
> diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
> index 1905ca590965..f0a6ce610948 100644
> --- a/drivers/gpu/drm/xe/xe_sched_job.c
> +++ b/drivers/gpu/drm/xe/xe_sched_job.c
> @@ -216,15 +216,17 @@ void xe_sched_job_set_error(struct xe_sched_job *job, int error)
>  
>  bool xe_sched_job_started(struct xe_sched_job *job)
>  {
> +	struct dma_fence *fence = dma_fence_chain_contained(job->fence);
>  	struct xe_lrc *lrc = job->q->lrc[0];
>  
> -	return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job),
> -				     xe_lrc_start_seqno(lrc),
> -				     dma_fence_chain_contained(job->fence)->ops);
> +	return !__dma_fence_is_later(fence,
> +				     xe_sched_job_lrc_seqno(job),
> +				     xe_lrc_start_seqno(lrc));
>  }
>  
>  bool xe_sched_job_completed(struct xe_sched_job *job)
>  {
> +	struct dma_fence *fence = dma_fence_chain_contained(job->fence);
>  	struct xe_lrc *lrc = job->q->lrc[0];
>  
>  	/*
> @@ -232,9 +234,9 @@ bool xe_sched_job_completed(struct xe_sched_job *job)
>  	 * parallel handshake is done.
>  	 */
>  
> -	return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job),
> -				     xe_lrc_seqno(lrc),
> -				     dma_fence_chain_contained(job->fence)->ops);
> +	return !__dma_fence_is_later(fence,
> +				     xe_sched_job_lrc_seqno(job),
> +				     xe_lrc_seqno(lrc));
>  }
>  
>  void xe_sched_job_arm(struct xe_sched_job *job)
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index b12776883d14..48b5202c531d 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -441,21 +441,20 @@ dma_fence_is_signaled(struct dma_fence *fence)
>  
>  /**
>   * __dma_fence_is_later - return if f1 is chronologically later than f2
> + * @fence: fence in whose context to do the comparison
>   * @f1: the first fence's seqno
>   * @f2: the second fence's seqno from the same context
> - * @ops: dma_fence_ops associated with the seqno
>   *
>   * Returns true if f1 is chronologically later than f2. Both fences must be
>   * from the same context, since a seqno is not common across contexts.
>   */
> -static inline bool __dma_fence_is_later(u64 f1, u64 f2,
> -					const struct dma_fence_ops *ops)
> +static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
>  {
>  	/* This is for backward compatibility with drivers which can only handle
>  	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
>  	 * do so.
>  	 */
> -	if (ops->use_64bit_seqno)
> +	if (fence->ops->use_64bit_seqno)
>  		return f1 > f2;
>  
>  	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;
> @@ -475,7 +474,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1,
>  	if (WARN_ON(f1->context != f2->context))
>  		return false;
>  
> -	return __dma_fence_is_later(f1->seqno, f2->seqno, f1->ops);
> +	return __dma_fence_is_later(f1, f1->seqno, f2->seqno);
>  }
>  
>  /**

Re: [RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> With the goal of reducing the need for drivers to touch (and dereference)
> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to
> the fence->flags.
> 
> Drivers which were setting this flag are changed to use new
> dma_fence_init64() instead of dma_fence_init().
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

My idea was to copy the flag from the ops during init, but that should work as well.

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/dma-buf/dma-fence-chain.c                | 5 ++---
>  drivers/dma-buf/dma-fence.c                      | 9 +++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++---
>  include/linux/dma-fence.h                        | 6 +++++-
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index 90424f23fd73..a8a90acf4f34 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence,
>  }
>  
>  const struct dma_fence_ops dma_fence_chain_ops = {
> -	.use_64bit_seqno = true,
>  	.get_driver_name = dma_fence_chain_get_driver_name,
>  	.get_timeline_name = dma_fence_chain_get_timeline_name,
>  	.enable_signaling = dma_fence_chain_enable_signaling,
> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>  			seqno = max(prev->seqno, seqno);
>  	}
>  
> -	dma_fence_init(&chain->base, &dma_fence_chain_ops,
> -		       &chain->lock, context, seqno);
> +	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
> +			 context, seqno);
>  
>  	/*
>  	 * Chaining dma_fence_chain container together is only allowed through
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index f0cdd3e99d36..33661658f684 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -1023,3 +1023,12 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> +		 spinlock_t *lock, u64 context, u64 seqno)
> +{
> +	dma_fence_init(fence, ops, lock, context, seqno);
> +	__set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
> +}
> +EXPORT_SYMBOL(dma_fence_init64);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> index 51cddfa3f1e8..5d26797356a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work)
>  }
>  
>  static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
> -	.use_64bit_seqno = true,
>  	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
>  	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>  };
> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm
>  	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>  	spin_lock_init(&f->lock);
>  
> -	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
> -		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
> +	dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
> +			 vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>  
>  	/* TODO: We probably need a separate wq here */
>  	dma_fence_get(&f->base);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 48b5202c531d..ac6535716dbe 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -97,6 +97,7 @@ struct dma_fence {
>  };
>  
>  enum dma_fence_flag_bits {
> +	DMA_FENCE_FLAG_SEQNO64_BIT,
>  	DMA_FENCE_FLAG_SIGNALED_BIT,
>  	DMA_FENCE_FLAG_TIMESTAMP_BIT,
>  	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> @@ -262,6 +263,9 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> +		      spinlock_t *lock, 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);
> @@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
>  	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
>  	 * do so.
>  	 */
> -	if (fence->ops->use_64bit_seqno)
> +	if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags))
>  		return f1 > f2;
>  
>  	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> With the goal of reducing the need for drivers to touch fence->ops, we
> add explicit flags for struct dma_fence_array and struct dma_fence_chain
> and make the respective helpers (dma_fence_is_array() and
> dma_fence_is_chain()) use them.
> 
> This also allows us to remove the exported symbols for the respective
> operation tables.

That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared.

Since the array and chain are always build in that should be completely unproblematic for driver unload.

Regards,
Christian.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/dma-buf/dma-fence-array.c | 2 +-
>  drivers/dma-buf/dma-fence-chain.c | 2 +-
>  include/linux/dma-fence.h         | 9 ++++-----
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
> index 6657d4b30af9..daf444f5d228 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
>  	.release = dma_fence_array_release,
>  	.set_deadline = dma_fence_array_set_deadline,
>  };
> -EXPORT_SYMBOL(dma_fence_array_ops);
>  
>  /**
>   * dma_fence_array_alloc - Allocate a custom fence array
> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array,
>  	spin_lock_init(&array->lock);
>  	dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
>  		       context, seqno);
> +	__set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags);
>  	init_irq_work(&array->work, irq_dma_fence_array_work);
>  
>  	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a8a90acf4f34..f4abe41fb092 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = {
>  	.release = dma_fence_chain_release,
>  	.set_deadline = dma_fence_chain_set_deadline,
>  };
> -EXPORT_SYMBOL(dma_fence_chain_ops);
>  
>  /**
>   * dma_fence_chain_init - initialize a fence chain
> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>  
>  	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>  			 context, seqno);
> +	__set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags);
>  
>  	/*
>  	 * Chaining dma_fence_chain container together is only allowed through
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index ac6535716dbe..5bafd0a5f1f1 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -98,6 +98,8 @@ struct dma_fence {
>  
>  enum dma_fence_flag_bits {
>  	DMA_FENCE_FLAG_SEQNO64_BIT,
> +	DMA_FENCE_FLAG_ARRAY_BIT,
> +	DMA_FENCE_FLAG_CHAIN_BIT,
>  	DMA_FENCE_FLAG_SIGNALED_BIT,
>  	DMA_FENCE_FLAG_TIMESTAMP_BIT,
>  	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void);
>  struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp);
>  u64 dma_fence_context_alloc(unsigned num);
>  
> -extern const struct dma_fence_ops dma_fence_array_ops;
> -extern const struct dma_fence_ops dma_fence_chain_ops;
> -
>  /**
>   * dma_fence_is_array - check if a fence is from the array subclass
>   * @fence: the fence to test
> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
>   */
>  static inline bool dma_fence_is_array(struct dma_fence *fence)
>  {
> -	return fence->ops == &dma_fence_array_ops;
> +	return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags);
>  }
>  
>  /**
> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
>   */
>  static inline bool dma_fence_is_chain(struct dma_fence *fence)
>  {
> -	return fence->ops == &dma_fence_chain_ops;
> +	return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags);
>  }
>  
>  /**

Re: [RFC v2 05/13] dma-fence: Add helpers for accessing driver and timeline name

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> Add some helpers in order to enable preventing dma-fence users accessing
> the implementation details directly.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  include/linux/dma-fence.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 5bafd0a5f1f1..c814a86087f8 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -387,6 +387,16 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
>  			       struct dma_fence_cb *cb);
>  void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>  
> +static inline const char *dma_fence_driver_name(struct dma_fence *fence)
> +{
> +	return fence->ops->get_driver_name(fence);
> +}
> +
> +static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
> +{
> +	return fence->ops->get_timeline_name(fence);
> +}
> +
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
>   *                                is signaled yet.

Re: [RFC v2 06/13] dma-fence: Use driver and timeline name helpers internally

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> Make the implementation use the previously added helper for accessing the
> driver and timeline name. This will enable more coverage later when
> asserts will be added into them.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

I would squash that together with the previous patch. Since both patches are for the same component it doesn't make much sense to separate them.

Anyway Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/dma-buf/dma-fence.c      | 9 +++++----
>  include/trace/events/dma_fence.h | 4 ++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 33661658f684..dc2456f68685 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref)
>  	if (WARN(!list_empty(&fence->cb_list) &&
>  		 !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags),
>  		 "Fence %s:%s:%llx:%llx released with pending signals!\n",
> -		 fence->ops->get_driver_name(fence),
> -		 fence->ops->get_timeline_name(fence),
> +		 dma_fence_driver_name(fence),
> +		 dma_fence_timeline_name(fence),
>  		 fence->context, fence->seqno)) {
>  		unsigned long flags;
>  
> @@ -983,8 +983,9 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>  void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>  {
>  	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
> -		   fence->ops->get_driver_name(fence),
> -		   fence->ops->get_timeline_name(fence), fence->seqno,
> +		   dma_fence_driver_name(fence),
> +		   dma_fence_timeline_name(fence),
> +		   fence->seqno,
>  		   dma_fence_is_signaled(fence) ? "" : "un");
>  }
>  EXPORT_SYMBOL(dma_fence_describe);
> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
> index a4de3df8500b..84c83074ee81 100644
> --- a/include/trace/events/dma_fence.h
> +++ b/include/trace/events/dma_fence.h
> @@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence,
>  	TP_ARGS(fence),
>  
>  	TP_STRUCT__entry(
> -		__string(driver, fence->ops->get_driver_name(fence))
> -		__string(timeline, fence->ops->get_timeline_name(fence))
> +		__string(driver, dma_fence_driver_name(fence))
> +		__string(timeline, dma_fence_timeline_name(fence))
>  		__field(unsigned int, context)
>  		__field(unsigned int, seqno)
>  	),

Re: [RFC v2 07/13] sync_file: Use dma-fence driver and timeline name helpers

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> Access the dma-fence internals via the previously added helpers.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/dma-buf/sync_file.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index d9b1c1b2a72b..212df4b849fe 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -137,8 +137,8 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
>  		struct dma_fence *fence = sync_file->fence;
>  
>  		snprintf(buf, len, "%s-%s%llu-%lld",
> -			 fence->ops->get_driver_name(fence),
> -			 fence->ops->get_timeline_name(fence),
> +			 dma_fence_driver_name(fence),
> +			 dma_fence_timeline_name(fence),
>  			 fence->context,
>  			 fence->seqno);
>  	}
> @@ -262,9 +262,9 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>  static int sync_fill_fence_info(struct dma_fence *fence,
>  				 struct sync_fence_info *info)
>  {
> -	strscpy(info->obj_name, fence->ops->get_timeline_name(fence),
> +	strscpy(info->obj_name, dma_fence_timeline_name(fence),
>  		sizeof(info->obj_name));
> -	strscpy(info->driver_name, fence->ops->get_driver_name(fence),
> +	strscpy(info->driver_name, dma_fence_driver_name(fence),
>  		sizeof(info->driver_name));
>  
>  	info->status = dma_fence_get_status(fence);

Re: [RFC v2 08/13] drm/amdgpu: Use dma-fence driver and timeline name helpers

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> Access the dma-fence internals via the previously added helpers.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 11dd2e0f7979..8e5bf179a6c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -33,7 +33,7 @@
>  #define TRACE_INCLUDE_FILE amdgpu_trace
>  
>  #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
> -	 job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished)
> +	 dma_fence_timeline_name(&job->base.s_fence->finished)

I think you can nuke the macro now as well, the new function name is short enough.

Christian.

>  
>  TRACE_EVENT(amdgpu_device_rreg,
>  	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),

Re: [RFC v2 09/13] drm/i915: Use dma-fence driver and timeline name helpers

[Christian König]

On 5/9/25 17:33, Tvrtko Ursulin wrote:
> Access the dma-fence internals via the previously added helpers.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 4 ++--
>  drivers/gpu/drm/i915/i915_request.c         | 2 +-
>  drivers/gpu/drm/i915/i915_sw_fence.c        | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index d1a382dfaa1d..ae3557ed6c1e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -252,8 +252,8 @@ void intel_gt_watchdog_work(struct work_struct *work)
>  			struct dma_fence *f = &rq->fence;
>  
>  			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
> -				  f->ops->get_driver_name(f),
> -				  f->ops->get_timeline_name(f),
> +				  dma_fence_driver_name(f),
> +				  dma_fence_timeline_name(f),
>  				  f->seqno);
>  			i915_request_cancel(rq, -EINTR);
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index c3d27eadc0a7..4874c4f1e4ab 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -2184,7 +2184,7 @@ void i915_request_show(struct drm_printer *m,
>  		       const char *prefix,
>  		       int indent)
>  {
> -	const char *name = rq->fence.ops->get_timeline_name((struct dma_fence *)&rq->fence);
> +	const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
>  	char buf[80] = "";
>  	int x = 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 1d4cc91c0e40..e51ca7e50a4e 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -435,8 +435,8 @@ static void timer_i915_sw_fence_wake(struct timer_list *t)
>  		return;
>  
>  	pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n",
> -		  cb->dma->ops->get_driver_name(cb->dma),
> -		  cb->dma->ops->get_timeline_name(cb->dma),
> +		  dma_fence_driver_name(cb->dma),
> +		  dma_fence_timeline_name(cb->dma),
>  		  cb->dma->seqno,
>  		  i915_sw_fence_debug_hint(fence));
>  

Re: [RFC v2 06/13] dma-fence: Use driver and timeline name helpers internally

[Tvrtko Ursulin]

On 12/05/2025 09:22, Christian König wrote:
> On 5/9/25 17:33, Tvrtko Ursulin wrote:
>> Make the implementation use the previously added helper for accessing the
>> driver and timeline name. This will enable more coverage later when
>> asserts will be added into them.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> I would squash that together with the previous patch. Since both patches are for the same component it doesn't make much sense to separate them.
> 
> Anyway Reviewed-by: Christian König <christian.koenig@amd.com>

Squashed locally and I kept your r-b.

Regards,

Tvrtko

>> ---
>>   drivers/dma-buf/dma-fence.c      | 9 +++++----
>>   include/trace/events/dma_fence.h | 4 ++--
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 33661658f684..dc2456f68685 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref)
>>   	if (WARN(!list_empty(&fence->cb_list) &&
>>   		 !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags),
>>   		 "Fence %s:%s:%llx:%llx released with pending signals!\n",
>> -		 fence->ops->get_driver_name(fence),
>> -		 fence->ops->get_timeline_name(fence),
>> +		 dma_fence_driver_name(fence),
>> +		 dma_fence_timeline_name(fence),
>>   		 fence->context, fence->seqno)) {
>>   		unsigned long flags;
>>   
>> @@ -983,8 +983,9 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>>   void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>>   {
>>   	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>> -		   fence->ops->get_driver_name(fence),
>> -		   fence->ops->get_timeline_name(fence), fence->seqno,
>> +		   dma_fence_driver_name(fence),
>> +		   dma_fence_timeline_name(fence),
>> +		   fence->seqno,
>>   		   dma_fence_is_signaled(fence) ? "" : "un");
>>   }
>>   EXPORT_SYMBOL(dma_fence_describe);
>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>> index a4de3df8500b..84c83074ee81 100644
>> --- a/include/trace/events/dma_fence.h
>> +++ b/include/trace/events/dma_fence.h
>> @@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence,
>>   	TP_ARGS(fence),
>>   
>>   	TP_STRUCT__entry(
>> -		__string(driver, fence->ops->get_driver_name(fence))
>> -		__string(timeline, fence->ops->get_timeline_name(fence))
>> +		__string(driver, dma_fence_driver_name(fence))
>> +		__string(timeline, dma_fence_timeline_name(fence))
>>   		__field(unsigned int, context)
>>   		__field(unsigned int, seqno)
>>   	),
> 

Re: [RFC v2 08/13] drm/amdgpu: Use dma-fence driver and timeline name helpers

[Tvrtko Ursulin]

On 12/05/2025 09:27, Christian König wrote:
> On 5/9/25 17:33, Tvrtko Ursulin wrote:
>> Access the dma-fence internals via the previously added helpers.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> index 11dd2e0f7979..8e5bf179a6c8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> @@ -33,7 +33,7 @@
>>   #define TRACE_INCLUDE_FILE amdgpu_trace
>>   
>>   #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>> -	 job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished)
>> +	 dma_fence_timeline_name(&job->base.s_fence->finished)
> 
> I think you can nuke the macro now as well, the new function name is short enough.

Done.

Regards,

Tvrtko

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

[Tvrtko Ursulin]

On 12/05/2025 09:19, Christian König wrote:
> On 5/9/25 17:33, Tvrtko Ursulin wrote:
>> With the goal of reducing the need for drivers to touch fence->ops, we
>> add explicit flags for struct dma_fence_array and struct dma_fence_chain
>> and make the respective helpers (dma_fence_is_array() and
>> dma_fence_is_chain()) use them.
>>
>> This also allows us to remove the exported symbols for the respective
>> operation tables.
> 
> That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared.
> 
> Since the array and chain are always build in that should be completely unproblematic for driver unload.

You are right this is not strictly needed. Idea was just to reduce any 
access to ops as much as we can and this fell under that scope.

Another benefit one could perhaps argue is two fewer EXPORT_SYMBOLs, 
which is perhaps a little bit cleaner design (less exporting of 
implementation details to the outside), but it is not a super strong 
argument.

If we will not be going for this one then I would be taking 1/13 via 
drm-intel-gt-next.

Regards,

Tvrtko

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> ---
>>   drivers/dma-buf/dma-fence-array.c | 2 +-
>>   drivers/dma-buf/dma-fence-chain.c | 2 +-
>>   include/linux/dma-fence.h         | 9 ++++-----
>>   3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
>> index 6657d4b30af9..daf444f5d228 100644
>> --- a/drivers/dma-buf/dma-fence-array.c
>> +++ b/drivers/dma-buf/dma-fence-array.c
>> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
>>   	.release = dma_fence_array_release,
>>   	.set_deadline = dma_fence_array_set_deadline,
>>   };
>> -EXPORT_SYMBOL(dma_fence_array_ops);
>>   
>>   /**
>>    * dma_fence_array_alloc - Allocate a custom fence array
>> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array,
>>   	spin_lock_init(&array->lock);
>>   	dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
>>   		       context, seqno);
>> +	__set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags);
>>   	init_irq_work(&array->work, irq_dma_fence_array_work);
>>   
>>   	atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>> index a8a90acf4f34..f4abe41fb092 100644
>> --- a/drivers/dma-buf/dma-fence-chain.c
>> +++ b/drivers/dma-buf/dma-fence-chain.c
>> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = {
>>   	.release = dma_fence_chain_release,
>>   	.set_deadline = dma_fence_chain_set_deadline,
>>   };
>> -EXPORT_SYMBOL(dma_fence_chain_ops);
>>   
>>   /**
>>    * dma_fence_chain_init - initialize a fence chain
>> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>>   
>>   	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>>   			 context, seqno);
>> +	__set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags);
>>   
>>   	/*
>>   	 * Chaining dma_fence_chain container together is only allowed through
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index ac6535716dbe..5bafd0a5f1f1 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -98,6 +98,8 @@ struct dma_fence {
>>   
>>   enum dma_fence_flag_bits {
>>   	DMA_FENCE_FLAG_SEQNO64_BIT,
>> +	DMA_FENCE_FLAG_ARRAY_BIT,
>> +	DMA_FENCE_FLAG_CHAIN_BIT,
>>   	DMA_FENCE_FLAG_SIGNALED_BIT,
>>   	DMA_FENCE_FLAG_TIMESTAMP_BIT,
>>   	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void);
>>   struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp);
>>   u64 dma_fence_context_alloc(unsigned num);
>>   
>> -extern const struct dma_fence_ops dma_fence_array_ops;
>> -extern const struct dma_fence_ops dma_fence_chain_ops;
>> -
>>   /**
>>    * dma_fence_is_array - check if a fence is from the array subclass
>>    * @fence: the fence to test
>> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
>>    */
>>   static inline bool dma_fence_is_array(struct dma_fence *fence)
>>   {
>> -	return fence->ops == &dma_fence_array_ops;
>> +	return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags);
>>   }
>>   
>>   /**
>> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
>>    */
>>   static inline bool dma_fence_is_chain(struct dma_fence *fence)
>>   {
>> -	return fence->ops == &dma_fence_chain_ops;
>> +	return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags);
>>   }
>>   
>>   /**
> 

Re: [RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

[Tvrtko Ursulin]

On 12/05/2025 09:17, Christian König wrote:
> On 5/9/25 17:33, Tvrtko Ursulin wrote:
>> With the goal of reducing the need for drivers to touch (and dereference)
>> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to
>> the fence->flags.
>>
>> Drivers which were setting this flag are changed to use new
>> dma_fence_init64() instead of dma_fence_init().
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> My idea was to copy the flag from the ops during init, but that should work as well.
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks!

One alternative to dma_fence_init64() I was thinking about is to perhaps 
have add dma_fence_init_flags(). Especially in the context of patch 4/13 
where it would then look cleaner for callers such as 
dma_fence_chain_init(). 4/13 aside, how would dma_fence_init_flags look 
to you? It is a little bit more verbose is one thing. And if we are not 
doing 4/13, verbosity coupled with a single benefit perhaps it is best 
to leave dma_fence_init64() as is?

Regards,

Tvrtko

>> ---
>>   drivers/dma-buf/dma-fence-chain.c                | 5 ++---
>>   drivers/dma-buf/dma-fence.c                      | 9 +++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++---
>>   include/linux/dma-fence.h                        | 6 +++++-
>>   4 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>> index 90424f23fd73..a8a90acf4f34 100644
>> --- a/drivers/dma-buf/dma-fence-chain.c
>> +++ b/drivers/dma-buf/dma-fence-chain.c
>> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence,
>>   }
>>   
>>   const struct dma_fence_ops dma_fence_chain_ops = {
>> -	.use_64bit_seqno = true,
>>   	.get_driver_name = dma_fence_chain_get_driver_name,
>>   	.get_timeline_name = dma_fence_chain_get_timeline_name,
>>   	.enable_signaling = dma_fence_chain_enable_signaling,
>> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>>   			seqno = max(prev->seqno, seqno);
>>   	}
>>   
>> -	dma_fence_init(&chain->base, &dma_fence_chain_ops,
>> -		       &chain->lock, context, seqno);
>> +	dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>> +			 context, seqno);
>>   
>>   	/*
>>   	 * Chaining dma_fence_chain container together is only allowed through
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index f0cdd3e99d36..33661658f684 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -1023,3 +1023,12 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>> +		 spinlock_t *lock, u64 context, u64 seqno)
>> +{
>> +	dma_fence_init(fence, ops, lock, context, seqno);
>> +	__set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
>> +}
>> +EXPORT_SYMBOL(dma_fence_init64);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> index 51cddfa3f1e8..5d26797356a3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work)
>>   }
>>   
>>   static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>> -	.use_64bit_seqno = true,
>>   	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>   	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>   };
>> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm
>>   	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>   	spin_lock_init(&f->lock);
>>   
>> -	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>> -		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>> +	dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>> +			 vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>   
>>   	/* TODO: We probably need a separate wq here */
>>   	dma_fence_get(&f->base);
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 48b5202c531d..ac6535716dbe 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -97,6 +97,7 @@ struct dma_fence {
>>   };
>>   
>>   enum dma_fence_flag_bits {
>> +	DMA_FENCE_FLAG_SEQNO64_BIT,
>>   	DMA_FENCE_FLAG_SIGNALED_BIT,
>>   	DMA_FENCE_FLAG_TIMESTAMP_BIT,
>>   	DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>> @@ -262,6 +263,9 @@ 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_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>> +		      spinlock_t *lock, 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);
>> @@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2)
>>   	 * 32bit sequence numbers. Use a 64bit compare when the driver says to
>>   	 * do so.
>>   	 */
>> -	if (fence->ops->use_64bit_seqno)
>> +	if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags))
>>   		return f1 > f2;
>>   
>>   	return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;
> 

Re: [RFC v2 13/13] drm/xe: Make dma-fences compliant with the safe access rules

[Matthew Brost]

On Fri, May 09, 2025 at 04:33:52PM +0100, Tvrtko Ursulin wrote:
> Xe can free some of the data pointed to by the dma-fences it exports. Most
> notably the timeline name can get freed if userspace closes the associated
> submit queue. At the same time the fence could have been exported to a
> third party (for example a sync_fence fd) which will then cause an use-
> after-free on subsequent access.
> 
> To make this safe we need to make the driver compliant with the newly
> documented dma-fence rules. Driver has to ensure a RCU grace period
> between signalling a fence and freeing any data pointed to by said fence.
> 
> For the timeline name we simply make the queue be freed via kfree_rcu and
> for the shared lock associated with multiple queues we add a RCU grace
> period before freeing the per GT structure holding the lock.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

This makes sense in the context of the series (e.g. assuming patch #9 lands).

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_guc_exec_queue_types.h | 2 ++
>  drivers/gpu/drm/xe/xe_guc_submit.c           | 7 ++++++-
>  drivers/gpu/drm/xe/xe_hw_fence.c             | 3 +++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> index 4c39f01e4f52..a3f421e2adc0 100644
> --- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> @@ -20,6 +20,8 @@ struct xe_exec_queue;
>  struct xe_guc_exec_queue {
>  	/** @q: Backpointer to parent xe_exec_queue */
>  	struct xe_exec_queue *q;
> +	/** @rcu: For safe freeing of exported dma fences */
> +	struct rcu_head rcu;
>  	/** @sched: GPU scheduler for this xe_exec_queue */
>  	struct xe_gpu_scheduler sched;
>  	/** @entity: Scheduler entity for this xe_exec_queue */
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 369be36f7dc5..cda837ff0118 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1274,7 +1274,11 @@ static void __guc_exec_queue_fini_async(struct work_struct *w)
>  	xe_sched_entity_fini(&ge->entity);
>  	xe_sched_fini(&ge->sched);
>  
> -	kfree(ge);
> +	/*
> +	 * RCU free due sched being exported via DRM scheduler fences
> +	 * (timeline name).
> +	 */
> +	kfree_rcu(ge, rcu);
>  	xe_exec_queue_fini(q);
>  	xe_pm_runtime_put(guc_to_xe(guc));
>  }
> @@ -1457,6 +1461,7 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  
>  	q->guc = ge;
>  	ge->q = q;
> +	init_rcu_head(&ge->rcu);
>  	init_waitqueue_head(&ge->suspend_wait);
>  
>  	for (i = 0; i < MAX_STATIC_MSG_TYPE; ++i)
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> index 03eb8c6d1616..b2a0c46dfcd4 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> @@ -100,6 +100,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
>  		spin_unlock_irqrestore(&irq->lock, flags);
>  		dma_fence_end_signalling(tmp);
>  	}
> +
> +	/* Safe release of the irq->lock used in dma_fence_init. */
> +	synchronize_rcu();
>  }
>  
>  void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
> -- 
> 2.48.0
> 

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

[Christian König]

On 5/12/25 11:14, Tvrtko Ursulin wrote:
> 
> On 12/05/2025 09:19, Christian König wrote:
>> On 5/9/25 17:33, Tvrtko Ursulin wrote:
>>> With the goal of reducing the need for drivers to touch fence->ops, we
>>> add explicit flags for struct dma_fence_array and struct dma_fence_chain
>>> and make the respective helpers (dma_fence_is_array() and
>>> dma_fence_is_chain()) use them.
>>>
>>> This also allows us to remove the exported symbols for the respective
>>> operation tables.
>>
>> That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared.
>>
>> Since the array and chain are always build in that should be completely unproblematic for driver unload.
> 
> You are right this is not strictly needed. Idea was just to reduce any access to ops as much as we can and this fell under that scope.
> 
> Another benefit one could perhaps argue is two fewer EXPORT_SYMBOLs, which is perhaps a little bit cleaner design (less exporting of implementation details to the outside), but it is not a super strong argument.


I would rather say that using the symbols improves things. Background is that otherwise every driver could accidentally or because of malicious intend set this flag.

The symbol is not so easily changeable.

Regards,
Christian. 

> 
> If we will not be going for this one then I would be taking 1/13 via drm-intel-gt-next.
> 
> Regards,
> 
> Tvrtko
> 
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> ---
>>>   drivers/dma-buf/dma-fence-array.c | 2 +-
>>>   drivers/dma-buf/dma-fence-chain.c | 2 +-
>>>   include/linux/dma-fence.h         | 9 ++++-----
>>>   3 files changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
>>> index 6657d4b30af9..daf444f5d228 100644
>>> --- a/drivers/dma-buf/dma-fence-array.c
>>> +++ b/drivers/dma-buf/dma-fence-array.c
>>> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
>>>       .release = dma_fence_array_release,
>>>       .set_deadline = dma_fence_array_set_deadline,
>>>   };
>>> -EXPORT_SYMBOL(dma_fence_array_ops);
>>>     /**
>>>    * dma_fence_array_alloc - Allocate a custom fence array
>>> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array,
>>>       spin_lock_init(&array->lock);
>>>       dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock,
>>>                  context, seqno);
>>> +    __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags);
>>>       init_irq_work(&array->work, irq_dma_fence_array_work);
>>>         atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>>> index a8a90acf4f34..f4abe41fb092 100644
>>> --- a/drivers/dma-buf/dma-fence-chain.c
>>> +++ b/drivers/dma-buf/dma-fence-chain.c
>>> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = {
>>>       .release = dma_fence_chain_release,
>>>       .set_deadline = dma_fence_chain_set_deadline,
>>>   };
>>> -EXPORT_SYMBOL(dma_fence_chain_ops);
>>>     /**
>>>    * dma_fence_chain_init - initialize a fence chain
>>> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
>>>         dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock,
>>>                context, seqno);
>>> +    __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags);
>>>         /*
>>>        * Chaining dma_fence_chain container together is only allowed through
>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>> index ac6535716dbe..5bafd0a5f1f1 100644
>>> --- a/include/linux/dma-fence.h
>>> +++ b/include/linux/dma-fence.h
>>> @@ -98,6 +98,8 @@ struct dma_fence {
>>>     enum dma_fence_flag_bits {
>>>       DMA_FENCE_FLAG_SEQNO64_BIT,
>>> +    DMA_FENCE_FLAG_ARRAY_BIT,
>>> +    DMA_FENCE_FLAG_CHAIN_BIT,
>>>       DMA_FENCE_FLAG_SIGNALED_BIT,
>>>       DMA_FENCE_FLAG_TIMESTAMP_BIT,
>>>       DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>>> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void);
>>>   struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp);
>>>   u64 dma_fence_context_alloc(unsigned num);
>>>   -extern const struct dma_fence_ops dma_fence_array_ops;
>>> -extern const struct dma_fence_ops dma_fence_chain_ops;
>>> -
>>>   /**
>>>    * dma_fence_is_array - check if a fence is from the array subclass
>>>    * @fence: the fence to test
>>> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
>>>    */
>>>   static inline bool dma_fence_is_array(struct dma_fence *fence)
>>>   {
>>> -    return fence->ops == &dma_fence_array_ops;
>>> +    return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags);
>>>   }
>>>     /**
>>> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
>>>    */
>>>   static inline bool dma_fence_is_chain(struct dma_fence *fence)
>>>   {
>>> -    return fence->ops == &dma_fence_chain_ops;
>>> +    return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags);
>>>   }
>>>     /**
>>
> 

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Rob Clark]

On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>
> Dma-fence objects currently suffer from a potential use after free problem
> where fences exported to userspace and other drivers can outlive the
> exporting driver, or the associated data structures.
>
> The discussion on how to address this concluded that adding reference
> counting to all the involved objects is not desirable, since it would need
> to be very wide reaching and could cause unloadable drivers if another
> entity would be holding onto a signaled fence reference potentially
> indefinitely.
>
> This patch enables the safe access by introducing and documenting a
> contract between fence exporters and users. It documents a set of
> contraints and adds helpers which a) drivers with potential to suffer from
> the use after free must use and b) users of the dma-fence API must use as
> well.
>
> Premise of the design has multiple sides:
>
> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
> signalling a fence and freeing the driver private data associated with it.
>
> The grace period does not have to follow the signalling immediately but
> HAS to happen before data is freed.
>
> 2. Users of the dma-fence API marked with such requirement MUST contain
> the complete access to the data within a single code block guarded by the
> new dma_fence_access_begin() and dma_fence_access_end() helpers.
>
> The combination of the two ensures that whoever sees the
> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
> valid fence->lock and valid data potentially accessed by the fence->ops
> virtual functions, until the call to dma_fence_access_end().
>
> 3. Module unload (fence->ops) disappearing is for now explicitly not
> handled. That would required a more complex protection, possibly needing
> SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
> where race between dma_fence_enable_sw_signaling, signalling, and
> dereference of fence->ops->wait() would need a sleeping SRCU context.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
>  include/linux/dma-fence.h   | 32 ++++++++++++-----
>  2 files changed, 93 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index dc2456f68685..cfe1d7b79c22 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
>         struct dma_fence *fence =
>                 container_of(kref, struct dma_fence, refcount);
>
> +       dma_fence_access_begin();
>         trace_dma_fence_destroy(fence);
>
>         if (WARN(!list_empty(&fence->cb_list) &&
> @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
>                 fence->ops->release(fence);
>         else
>                 dma_fence_free(fence);
> +
> +       dma_fence_access_end();
>  }
>  EXPORT_SYMBOL(dma_fence_release);
>
> @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>   */
>  void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>  {
> +       dma_fence_access_begin();
>         seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>                    dma_fence_driver_name(fence),
>                    dma_fence_timeline_name(fence),
>                    fence->seqno,
>                    dma_fence_is_signaled(fence) ? "" : "un");
> +       dma_fence_access_end();
>  }
>  EXPORT_SYMBOL(dma_fence_describe);
>
> @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>         __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
>  }
>  EXPORT_SYMBOL(dma_fence_init64);
> +
> +/**
> + * dma_fence_driver_name - Access the driver name
> + * @fence: the fence to query
> + *
> + * Returns a driver name backing the dma-fence implementation.
> + *
> + * IMPORTANT CONSIDERATION:
> + * Dma-fence contract stipulates that access to driver provided data (data not
> + * directly embedded into the object itself), such as the &dma_fence.lock and
> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> + * after the fence has been signalled. Drivers are allowed to free that data,
> + * and some do.
> + *
> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> + * between signalling the fence and freeing said data.
> + *
> + * As such access to the driver name is only valid inside a RCU locked section.
> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> + */
> +const char *dma_fence_driver_name(struct dma_fence *fence)
> +{
> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +                        "rcu_read_lock() required for safe access to returned string");
> +
> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +               return fence->ops->get_driver_name(fence);
> +       else
> +               return "detached-driver";
> +}
> +EXPORT_SYMBOL(dma_fence_driver_name);
> +
> +/**
> + * dma_fence_timeline_name - Access the timeline name
> + * @fence: the fence to query
> + *
> + * Returns a timeline name provided by the dma-fence implementation.
> + *
> + * IMPORTANT CONSIDERATION:
> + * Dma-fence contract stipulates that access to driver provided data (data not
> + * directly embedded into the object itself), such as the &dma_fence.lock and
> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> + * after the fence has been signalled. Drivers are allowed to free that data,
> + * and some do.
> + *
> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> + * between signalling the fence and freeing said data.
> + *
> + * As such access to the driver name is only valid inside a RCU locked section.
> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> + */
> +const char *dma_fence_timeline_name(struct dma_fence *fence)
> +{
> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +                        "rcu_read_lock() required for safe access to returned string");
> +
> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +               return fence->ops->get_driver_name(fence);
> +       else
> +               return "signaled-timeline";

This means that trace_dma_fence_signaled() will get the wrong
timeline/driver name, which probably screws up perfetto and maybe
other tools.

Maybe it would work well enough just to move the
trace_dma_fence_signaled() call ahead of the test_and_set_bit()?  Idk
if some things will start getting confused if they see that trace
multiple times.

Maybe a better solution would be to add a new bit for this purpose,
which is set after the tracepoint in
dma_fence_signal_timestamp_locked().

(trace_dma_fence_destroy() will I guess be messed up regardless.. it
doesn't look like perfetto cares about this tracepoint, so maybe that
is ok.  It doesn't seem so useful.)

BR,
-R


> +}
> +EXPORT_SYMBOL(dma_fence_timeline_name);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index c814a86087f8..c8a9915eb360 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -387,15 +387,31 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
>                                struct dma_fence_cb *cb);
>  void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>
> -static inline const char *dma_fence_driver_name(struct dma_fence *fence)
> -{
> -       return fence->ops->get_driver_name(fence);
> -}
> +/**
> + * DOC: Safe external access to driver provided object members
> + *
> + * All data not stored directly in the dma-fence object, such as the
> + * &dma_fence.lock and memory potentially accessed by functions in the
> + * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled
> + * because after that point drivers are allowed to free it.
> + *
> + * All code accessing that data via the dma-fence API (or directly, which is
> + * discouraged), MUST make sure to contain the complete access within a
> + * &dma_fence_access_begin and &dma_fence_access_end pair.
> + *
> + * Some dma-fence API handles this automatically, while other, as for example
> + * &dma_fence_driver_name and &dma_fence_timeline_name, leave that
> + * responsibility to the caller.
> + *
> + * To enable this scheme to work drivers MUST ensure a RCU grace period elapses
> + * between signalling the fence and freeing the said data.
> + *
> + */
> +#define dma_fence_access_begin rcu_read_lock
> +#define dma_fence_access_end   rcu_read_unlock
>
> -static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
> -{
> -       return fence->ops->get_timeline_name(fence);
> -}
> +const char *dma_fence_driver_name(struct dma_fence *fence);
> +const char *dma_fence_timeline_name(struct dma_fence *fence);
>
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
> --
> 2.48.0
>

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Tvrtko Ursulin]

On 13/05/2025 15:16, Rob Clark wrote:
> On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>>
>> Dma-fence objects currently suffer from a potential use after free problem
>> where fences exported to userspace and other drivers can outlive the
>> exporting driver, or the associated data structures.
>>
>> The discussion on how to address this concluded that adding reference
>> counting to all the involved objects is not desirable, since it would need
>> to be very wide reaching and could cause unloadable drivers if another
>> entity would be holding onto a signaled fence reference potentially
>> indefinitely.
>>
>> This patch enables the safe access by introducing and documenting a
>> contract between fence exporters and users. It documents a set of
>> contraints and adds helpers which a) drivers with potential to suffer from
>> the use after free must use and b) users of the dma-fence API must use as
>> well.
>>
>> Premise of the design has multiple sides:
>>
>> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
>> signalling a fence and freeing the driver private data associated with it.
>>
>> The grace period does not have to follow the signalling immediately but
>> HAS to happen before data is freed.
>>
>> 2. Users of the dma-fence API marked with such requirement MUST contain
>> the complete access to the data within a single code block guarded by the
>> new dma_fence_access_begin() and dma_fence_access_end() helpers.
>>
>> The combination of the two ensures that whoever sees the
>> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
>> valid fence->lock and valid data potentially accessed by the fence->ops
>> virtual functions, until the call to dma_fence_access_end().
>>
>> 3. Module unload (fence->ops) disappearing is for now explicitly not
>> handled. That would required a more complex protection, possibly needing
>> SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
>> where race between dma_fence_enable_sw_signaling, signalling, and
>> dereference of fence->ops->wait() would need a sleeping SRCU context.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> ---
>>   drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-fence.h   | 32 ++++++++++++-----
>>   2 files changed, 93 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index dc2456f68685..cfe1d7b79c22 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
>>          struct dma_fence *fence =
>>                  container_of(kref, struct dma_fence, refcount);
>>
>> +       dma_fence_access_begin();
>>          trace_dma_fence_destroy(fence);
>>
>>          if (WARN(!list_empty(&fence->cb_list) &&
>> @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
>>                  fence->ops->release(fence);
>>          else
>>                  dma_fence_free(fence);
>> +
>> +       dma_fence_access_end();
>>   }
>>   EXPORT_SYMBOL(dma_fence_release);
>>
>> @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>>    */
>>   void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>>   {
>> +       dma_fence_access_begin();
>>          seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>>                     dma_fence_driver_name(fence),
>>                     dma_fence_timeline_name(fence),
>>                     fence->seqno,
>>                     dma_fence_is_signaled(fence) ? "" : "un");
>> +       dma_fence_access_end();
>>   }
>>   EXPORT_SYMBOL(dma_fence_describe);
>>
>> @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>>          __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
>>   }
>>   EXPORT_SYMBOL(dma_fence_init64);
>> +
>> +/**
>> + * dma_fence_driver_name - Access the driver name
>> + * @fence: the fence to query
>> + *
>> + * Returns a driver name backing the dma-fence implementation.
>> + *
>> + * IMPORTANT CONSIDERATION:
>> + * Dma-fence contract stipulates that access to driver provided data (data not
>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>> + * after the fence has been signalled. Drivers are allowed to free that data,
>> + * and some do.
>> + *
>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>> + * between signalling the fence and freeing said data.
>> + *
>> + * As such access to the driver name is only valid inside a RCU locked section.
>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
>> + */
>> +const char *dma_fence_driver_name(struct dma_fence *fence)
>> +{
>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> +                        "rcu_read_lock() required for safe access to returned string");
>> +
>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> +               return fence->ops->get_driver_name(fence);
>> +       else
>> +               return "detached-driver";
>> +}
>> +EXPORT_SYMBOL(dma_fence_driver_name);
>> +
>> +/**
>> + * dma_fence_timeline_name - Access the timeline name
>> + * @fence: the fence to query
>> + *
>> + * Returns a timeline name provided by the dma-fence implementation.
>> + *
>> + * IMPORTANT CONSIDERATION:
>> + * Dma-fence contract stipulates that access to driver provided data (data not
>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>> + * after the fence has been signalled. Drivers are allowed to free that data,
>> + * and some do.
>> + *
>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>> + * between signalling the fence and freeing said data.
>> + *
>> + * As such access to the driver name is only valid inside a RCU locked section.
>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
>> + */
>> +const char *dma_fence_timeline_name(struct dma_fence *fence)
>> +{
>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> +                        "rcu_read_lock() required for safe access to returned string");
>> +
>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> +               return fence->ops->get_driver_name(fence);
>> +       else
>> +               return "signaled-timeline";
> 
> This means that trace_dma_fence_signaled() will get the wrong
> timeline/driver name, which probably screws up perfetto and maybe
> other tools.

Do you think context and seqno are not enough for those tools and they 
actually rely on the names? It would sound weird if they decided to 
index anything on the names which are non-standardised between drivers, 
but I guess anything is possible.

> Maybe it would work well enough just to move the
> trace_dma_fence_signaled() call ahead of the test_and_set_bit()?  Idk
> if some things will start getting confused if they see that trace
> multiple times.

Another alternative is to make this tracepoint access the names 
directly. It is under the lock so guaranteed not to get freed with 
drivers which will be made compliant with the documented rules.

Regards,

Tvrtko

> 
> Maybe a better solution would be to add a new bit for this purpose,
> which is set after the tracepoint in
> dma_fence_signal_timestamp_locked().
> 
> (trace_dma_fence_destroy() will I guess be messed up regardless.. it
> doesn't look like perfetto cares about this tracepoint, so maybe that
> is ok.  It doesn't seem so useful.)
> 
> BR,
> -R
> 
> 
>> +}
>> +EXPORT_SYMBOL(dma_fence_timeline_name);
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index c814a86087f8..c8a9915eb360 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -387,15 +387,31 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
>>                                 struct dma_fence_cb *cb);
>>   void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>
>> -static inline const char *dma_fence_driver_name(struct dma_fence *fence)
>> -{
>> -       return fence->ops->get_driver_name(fence);
>> -}
>> +/**
>> + * DOC: Safe external access to driver provided object members
>> + *
>> + * All data not stored directly in the dma-fence object, such as the
>> + * &dma_fence.lock and memory potentially accessed by functions in the
>> + * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled
>> + * because after that point drivers are allowed to free it.
>> + *
>> + * All code accessing that data via the dma-fence API (or directly, which is
>> + * discouraged), MUST make sure to contain the complete access within a
>> + * &dma_fence_access_begin and &dma_fence_access_end pair.
>> + *
>> + * Some dma-fence API handles this automatically, while other, as for example
>> + * &dma_fence_driver_name and &dma_fence_timeline_name, leave that
>> + * responsibility to the caller.
>> + *
>> + * To enable this scheme to work drivers MUST ensure a RCU grace period elapses
>> + * between signalling the fence and freeing the said data.
>> + *
>> + */
>> +#define dma_fence_access_begin rcu_read_lock
>> +#define dma_fence_access_end   rcu_read_unlock
>>
>> -static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
>> -{
>> -       return fence->ops->get_timeline_name(fence);
>> -}
>> +const char *dma_fence_driver_name(struct dma_fence *fence);
>> +const char *dma_fence_timeline_name(struct dma_fence *fence);
>>
>>   /**
>>    * dma_fence_is_signaled_locked - Return an indication if the fence
>> --
>> 2.48.0
>>

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Rob Clark]

On Wed, May 14, 2025 at 3:01 AM Tvrtko Ursulin
<tvrtko.ursulin@igalia.com> wrote:
>
>
> On 13/05/2025 15:16, Rob Clark wrote:
> > On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
> >>
> >> Dma-fence objects currently suffer from a potential use after free problem
> >> where fences exported to userspace and other drivers can outlive the
> >> exporting driver, or the associated data structures.
> >>
> >> The discussion on how to address this concluded that adding reference
> >> counting to all the involved objects is not desirable, since it would need
> >> to be very wide reaching and could cause unloadable drivers if another
> >> entity would be holding onto a signaled fence reference potentially
> >> indefinitely.
> >>
> >> This patch enables the safe access by introducing and documenting a
> >> contract between fence exporters and users. It documents a set of
> >> contraints and adds helpers which a) drivers with potential to suffer from
> >> the use after free must use and b) users of the dma-fence API must use as
> >> well.
> >>
> >> Premise of the design has multiple sides:
> >>
> >> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
> >> signalling a fence and freeing the driver private data associated with it.
> >>
> >> The grace period does not have to follow the signalling immediately but
> >> HAS to happen before data is freed.
> >>
> >> 2. Users of the dma-fence API marked with such requirement MUST contain
> >> the complete access to the data within a single code block guarded by the
> >> new dma_fence_access_begin() and dma_fence_access_end() helpers.
> >>
> >> The combination of the two ensures that whoever sees the
> >> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
> >> valid fence->lock and valid data potentially accessed by the fence->ops
> >> virtual functions, until the call to dma_fence_access_end().
> >>
> >> 3. Module unload (fence->ops) disappearing is for now explicitly not
> >> handled. That would required a more complex protection, possibly needing
> >> SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
> >> where race between dma_fence_enable_sw_signaling, signalling, and
> >> dereference of fence->ops->wait() would need a sleeping SRCU context.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >> ---
> >>   drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
> >>   include/linux/dma-fence.h   | 32 ++++++++++++-----
> >>   2 files changed, 93 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> >> index dc2456f68685..cfe1d7b79c22 100644
> >> --- a/drivers/dma-buf/dma-fence.c
> >> +++ b/drivers/dma-buf/dma-fence.c
> >> @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
> >>          struct dma_fence *fence =
> >>                  container_of(kref, struct dma_fence, refcount);
> >>
> >> +       dma_fence_access_begin();
> >>          trace_dma_fence_destroy(fence);
> >>
> >>          if (WARN(!list_empty(&fence->cb_list) &&
> >> @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
> >>                  fence->ops->release(fence);
> >>          else
> >>                  dma_fence_free(fence);
> >> +
> >> +       dma_fence_access_end();
> >>   }
> >>   EXPORT_SYMBOL(dma_fence_release);
> >>
> >> @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
> >>    */
> >>   void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
> >>   {
> >> +       dma_fence_access_begin();
> >>          seq_printf(seq, "%s %s seq %llu %ssignalled\n",
> >>                     dma_fence_driver_name(fence),
> >>                     dma_fence_timeline_name(fence),
> >>                     fence->seqno,
> >>                     dma_fence_is_signaled(fence) ? "" : "un");
> >> +       dma_fence_access_end();
> >>   }
> >>   EXPORT_SYMBOL(dma_fence_describe);
> >>
> >> @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> >>          __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
> >>   }
> >>   EXPORT_SYMBOL(dma_fence_init64);
> >> +
> >> +/**
> >> + * dma_fence_driver_name - Access the driver name
> >> + * @fence: the fence to query
> >> + *
> >> + * Returns a driver name backing the dma-fence implementation.
> >> + *
> >> + * IMPORTANT CONSIDERATION:
> >> + * Dma-fence contract stipulates that access to driver provided data (data not
> >> + * directly embedded into the object itself), such as the &dma_fence.lock and
> >> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> >> + * after the fence has been signalled. Drivers are allowed to free that data,
> >> + * and some do.
> >> + *
> >> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> >> + * between signalling the fence and freeing said data.
> >> + *
> >> + * As such access to the driver name is only valid inside a RCU locked section.
> >> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> >> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> >> + */
> >> +const char *dma_fence_driver_name(struct dma_fence *fence)
> >> +{
> >> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> >> +                        "rcu_read_lock() required for safe access to returned string");
> >> +
> >> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >> +               return fence->ops->get_driver_name(fence);
> >> +       else
> >> +               return "detached-driver";
> >> +}
> >> +EXPORT_SYMBOL(dma_fence_driver_name);
> >> +
> >> +/**
> >> + * dma_fence_timeline_name - Access the timeline name
> >> + * @fence: the fence to query
> >> + *
> >> + * Returns a timeline name provided by the dma-fence implementation.
> >> + *
> >> + * IMPORTANT CONSIDERATION:
> >> + * Dma-fence contract stipulates that access to driver provided data (data not
> >> + * directly embedded into the object itself), such as the &dma_fence.lock and
> >> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> >> + * after the fence has been signalled. Drivers are allowed to free that data,
> >> + * and some do.
> >> + *
> >> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> >> + * between signalling the fence and freeing said data.
> >> + *
> >> + * As such access to the driver name is only valid inside a RCU locked section.
> >> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> >> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> >> + */
> >> +const char *dma_fence_timeline_name(struct dma_fence *fence)
> >> +{
> >> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> >> +                        "rcu_read_lock() required for safe access to returned string");
> >> +
> >> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >> +               return fence->ops->get_driver_name(fence);
> >> +       else
> >> +               return "signaled-timeline";
> >
> > This means that trace_dma_fence_signaled() will get the wrong
> > timeline/driver name, which probably screws up perfetto and maybe
> > other tools.
>
> Do you think context and seqno are not enough for those tools and they
> actually rely on the names? It would sound weird if they decided to
> index anything on the names which are non-standardised between drivers,
> but I guess anything is possible.

At some point perfetto uses the timeline name to put up a named fence
timeline, I'm not sure if it is using the name or context # for
subsequent fence events (namely, signalled).  I'd have to check the
code and get back to you.

There is also gpuvis, which I guess does something similar, but
haven't looked into it.  Idk if there are others.

> > Maybe it would work well enough just to move the
> > trace_dma_fence_signaled() call ahead of the test_and_set_bit()?  Idk
> > if some things will start getting confused if they see that trace
> > multiple times.
>
> Another alternative is to make this tracepoint access the names
> directly. It is under the lock so guaranteed not to get freed with
> drivers which will be made compliant with the documented rules.

I guess it would have been better if, other than dma_fence_init
tracepoint, later tracepoints didn't include the driver/timeline
name.. that would have forced the use of the context.  But I guess too
late for that.  Perhaps the least bad thing to do is use the locking?

BR,
-R

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Tvrtko Ursulin]

On 14/05/2025 14:57, Rob Clark wrote:
> On Wed, May 14, 2025 at 3:01 AM Tvrtko Ursulin
> <tvrtko.ursulin@igalia.com> wrote:
>>
>>
>> On 13/05/2025 15:16, Rob Clark wrote:
>>> On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>>>>
>>>> Dma-fence objects currently suffer from a potential use after free problem
>>>> where fences exported to userspace and other drivers can outlive the
>>>> exporting driver, or the associated data structures.
>>>>
>>>> The discussion on how to address this concluded that adding reference
>>>> counting to all the involved objects is not desirable, since it would need
>>>> to be very wide reaching and could cause unloadable drivers if another
>>>> entity would be holding onto a signaled fence reference potentially
>>>> indefinitely.
>>>>
>>>> This patch enables the safe access by introducing and documenting a
>>>> contract between fence exporters and users. It documents a set of
>>>> contraints and adds helpers which a) drivers with potential to suffer from
>>>> the use after free must use and b) users of the dma-fence API must use as
>>>> well.
>>>>
>>>> Premise of the design has multiple sides:
>>>>
>>>> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
>>>> signalling a fence and freeing the driver private data associated with it.
>>>>
>>>> The grace period does not have to follow the signalling immediately but
>>>> HAS to happen before data is freed.
>>>>
>>>> 2. Users of the dma-fence API marked with such requirement MUST contain
>>>> the complete access to the data within a single code block guarded by the
>>>> new dma_fence_access_begin() and dma_fence_access_end() helpers.
>>>>
>>>> The combination of the two ensures that whoever sees the
>>>> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
>>>> valid fence->lock and valid data potentially accessed by the fence->ops
>>>> virtual functions, until the call to dma_fence_access_end().
>>>>
>>>> 3. Module unload (fence->ops) disappearing is for now explicitly not
>>>> handled. That would required a more complex protection, possibly needing
>>>> SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
>>>> where race between dma_fence_enable_sw_signaling, signalling, and
>>>> dereference of fence->ops->wait() would need a sleeping SRCU context.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> ---
>>>>    drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
>>>>    include/linux/dma-fence.h   | 32 ++++++++++++-----
>>>>    2 files changed, 93 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>>> index dc2456f68685..cfe1d7b79c22 100644
>>>> --- a/drivers/dma-buf/dma-fence.c
>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>> @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
>>>>           struct dma_fence *fence =
>>>>                   container_of(kref, struct dma_fence, refcount);
>>>>
>>>> +       dma_fence_access_begin();
>>>>           trace_dma_fence_destroy(fence);
>>>>
>>>>           if (WARN(!list_empty(&fence->cb_list) &&
>>>> @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
>>>>                   fence->ops->release(fence);
>>>>           else
>>>>                   dma_fence_free(fence);
>>>> +
>>>> +       dma_fence_access_end();
>>>>    }
>>>>    EXPORT_SYMBOL(dma_fence_release);
>>>>
>>>> @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>>>>     */
>>>>    void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>>>>    {
>>>> +       dma_fence_access_begin();
>>>>           seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>>>>                      dma_fence_driver_name(fence),
>>>>                      dma_fence_timeline_name(fence),
>>>>                      fence->seqno,
>>>>                      dma_fence_is_signaled(fence) ? "" : "un");
>>>> +       dma_fence_access_end();
>>>>    }
>>>>    EXPORT_SYMBOL(dma_fence_describe);
>>>>
>>>> @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>>>>           __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
>>>>    }
>>>>    EXPORT_SYMBOL(dma_fence_init64);
>>>> +
>>>> +/**
>>>> + * dma_fence_driver_name - Access the driver name
>>>> + * @fence: the fence to query
>>>> + *
>>>> + * Returns a driver name backing the dma-fence implementation.
>>>> + *
>>>> + * IMPORTANT CONSIDERATION:
>>>> + * Dma-fence contract stipulates that access to driver provided data (data not
>>>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>>>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>>>> + * after the fence has been signalled. Drivers are allowed to free that data,
>>>> + * and some do.
>>>> + *
>>>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>>>> + * between signalling the fence and freeing said data.
>>>> + *
>>>> + * As such access to the driver name is only valid inside a RCU locked section.
>>>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>>>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
>>>> + */
>>>> +const char *dma_fence_driver_name(struct dma_fence *fence)
>>>> +{
>>>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>>>> +                        "rcu_read_lock() required for safe access to returned string");
>>>> +
>>>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>>> +               return fence->ops->get_driver_name(fence);
>>>> +       else
>>>> +               return "detached-driver";
>>>> +}
>>>> +EXPORT_SYMBOL(dma_fence_driver_name);
>>>> +
>>>> +/**
>>>> + * dma_fence_timeline_name - Access the timeline name
>>>> + * @fence: the fence to query
>>>> + *
>>>> + * Returns a timeline name provided by the dma-fence implementation.
>>>> + *
>>>> + * IMPORTANT CONSIDERATION:
>>>> + * Dma-fence contract stipulates that access to driver provided data (data not
>>>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>>>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>>>> + * after the fence has been signalled. Drivers are allowed to free that data,
>>>> + * and some do.
>>>> + *
>>>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>>>> + * between signalling the fence and freeing said data.
>>>> + *
>>>> + * As such access to the driver name is only valid inside a RCU locked section.
>>>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>>>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
>>>> + */
>>>> +const char *dma_fence_timeline_name(struct dma_fence *fence)
>>>> +{
>>>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>>>> +                        "rcu_read_lock() required for safe access to returned string");
>>>> +
>>>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>>> +               return fence->ops->get_driver_name(fence);
>>>> +       else
>>>> +               return "signaled-timeline";
>>>
>>> This means that trace_dma_fence_signaled() will get the wrong
>>> timeline/driver name, which probably screws up perfetto and maybe
>>> other tools.
>>
>> Do you think context and seqno are not enough for those tools and they
>> actually rely on the names? It would sound weird if they decided to
>> index anything on the names which are non-standardised between drivers,
>> but I guess anything is possible.
> 
> At some point perfetto uses the timeline name to put up a named fence
> timeline, I'm not sure if it is using the name or context # for
> subsequent fence events (namely, signalled).  I'd have to check the
> code and get back to you.

If you can it would be useful. Presumably it saves the names from the 
start edge of fence lifetime. But again, who knows.

> There is also gpuvis, which I guess does something similar, but
> haven't looked into it.  Idk if there are others.

I know GpuVis uses DRM sched tracepoints since Pierre-Eric was 
explaining me about those in the context of tracing rework he did there. 
I am not sure about dma-fence tracepoints.

+Pierre-Eric on the off chance you know from the top of your head how 
much GpuVis depends on them (dma-fence tracepoints).

>>> Maybe it would work well enough just to move the
>>> trace_dma_fence_signaled() call ahead of the test_and_set_bit()?  Idk
>>> if some things will start getting confused if they see that trace
>>> multiple times.
>>
>> Another alternative is to make this tracepoint access the names
>> directly. It is under the lock so guaranteed not to get freed with
>> drivers which will be made compliant with the documented rules.
> 
> I guess it would have been better if, other than dma_fence_init
> tracepoint, later tracepoints didn't include the driver/timeline
> name.. that would have forced the use of the context.  But I guess too
> late for that.  Perhaps the least bad thing to do is use the locking?

You mean this last alternative I mentioned? I think that will work fine. 
I'll wait a little bit longer for more potential comments before re-spi 
ning with that.

Were you able to test the series for your use case? Assuming it is not 
upstream msm since I don't immediately see a path in msm_fence which 
gets freed at runtime?

Regards,

Tvrtko

Re: [RFC v2 10/13] dma-fence: Add safe access helpers and document the rules

[Rob Clark]

On Wed, May 14, 2025 at 7:58 AM Tvrtko Ursulin
<tvrtko.ursulin@igalia.com> wrote:
>
>
> On 14/05/2025 14:57, Rob Clark wrote:
> > On Wed, May 14, 2025 at 3:01 AM Tvrtko Ursulin
> > <tvrtko.ursulin@igalia.com> wrote:
> >>
> >>
> >> On 13/05/2025 15:16, Rob Clark wrote:
> >>> On Fri, May 9, 2025 at 8:34 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
> >>>>
> >>>> Dma-fence objects currently suffer from a potential use after free problem
> >>>> where fences exported to userspace and other drivers can outlive the
> >>>> exporting driver, or the associated data structures.
> >>>>
> >>>> The discussion on how to address this concluded that adding reference
> >>>> counting to all the involved objects is not desirable, since it would need
> >>>> to be very wide reaching and could cause unloadable drivers if another
> >>>> entity would be holding onto a signaled fence reference potentially
> >>>> indefinitely.
> >>>>
> >>>> This patch enables the safe access by introducing and documenting a
> >>>> contract between fence exporters and users. It documents a set of
> >>>> contraints and adds helpers which a) drivers with potential to suffer from
> >>>> the use after free must use and b) users of the dma-fence API must use as
> >>>> well.
> >>>>
> >>>> Premise of the design has multiple sides:
> >>>>
> >>>> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
> >>>> signalling a fence and freeing the driver private data associated with it.
> >>>>
> >>>> The grace period does not have to follow the signalling immediately but
> >>>> HAS to happen before data is freed.
> >>>>
> >>>> 2. Users of the dma-fence API marked with such requirement MUST contain
> >>>> the complete access to the data within a single code block guarded by the
> >>>> new dma_fence_access_begin() and dma_fence_access_end() helpers.
> >>>>
> >>>> The combination of the two ensures that whoever sees the
> >>>> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
> >>>> valid fence->lock and valid data potentially accessed by the fence->ops
> >>>> virtual functions, until the call to dma_fence_access_end().
> >>>>
> >>>> 3. Module unload (fence->ops) disappearing is for now explicitly not
> >>>> handled. That would required a more complex protection, possibly needing
> >>>> SRCU instead of RCU to handle callers such as dma_fence_wait_timeout(),
> >>>> where race between dma_fence_enable_sw_signaling, signalling, and
> >>>> dereference of fence->ops->wait() would need a sleeping SRCU context.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >>>> ---
> >>>>    drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++++++++++++++
> >>>>    include/linux/dma-fence.h   | 32 ++++++++++++-----
> >>>>    2 files changed, 93 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> >>>> index dc2456f68685..cfe1d7b79c22 100644
> >>>> --- a/drivers/dma-buf/dma-fence.c
> >>>> +++ b/drivers/dma-buf/dma-fence.c
> >>>> @@ -533,6 +533,7 @@ void dma_fence_release(struct kref *kref)
> >>>>           struct dma_fence *fence =
> >>>>                   container_of(kref, struct dma_fence, refcount);
> >>>>
> >>>> +       dma_fence_access_begin();
> >>>>           trace_dma_fence_destroy(fence);
> >>>>
> >>>>           if (WARN(!list_empty(&fence->cb_list) &&
> >>>> @@ -560,6 +561,8 @@ void dma_fence_release(struct kref *kref)
> >>>>                   fence->ops->release(fence);
> >>>>           else
> >>>>                   dma_fence_free(fence);
> >>>> +
> >>>> +       dma_fence_access_end();
> >>>>    }
> >>>>    EXPORT_SYMBOL(dma_fence_release);
> >>>>
> >>>> @@ -982,11 +985,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
> >>>>     */
> >>>>    void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
> >>>>    {
> >>>> +       dma_fence_access_begin();
> >>>>           seq_printf(seq, "%s %s seq %llu %ssignalled\n",
> >>>>                      dma_fence_driver_name(fence),
> >>>>                      dma_fence_timeline_name(fence),
> >>>>                      fence->seqno,
> >>>>                      dma_fence_is_signaled(fence) ? "" : "un");
> >>>> +       dma_fence_access_end();
> >>>>    }
> >>>>    EXPORT_SYMBOL(dma_fence_describe);
> >>>>
> >>>> @@ -1033,3 +1038,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
> >>>>           __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags);
> >>>>    }
> >>>>    EXPORT_SYMBOL(dma_fence_init64);
> >>>> +
> >>>> +/**
> >>>> + * dma_fence_driver_name - Access the driver name
> >>>> + * @fence: the fence to query
> >>>> + *
> >>>> + * Returns a driver name backing the dma-fence implementation.
> >>>> + *
> >>>> + * IMPORTANT CONSIDERATION:
> >>>> + * Dma-fence contract stipulates that access to driver provided data (data not
> >>>> + * directly embedded into the object itself), such as the &dma_fence.lock and
> >>>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> >>>> + * after the fence has been signalled. Drivers are allowed to free that data,
> >>>> + * and some do.
> >>>> + *
> >>>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> >>>> + * between signalling the fence and freeing said data.
> >>>> + *
> >>>> + * As such access to the driver name is only valid inside a RCU locked section.
> >>>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> >>>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> >>>> + */
> >>>> +const char *dma_fence_driver_name(struct dma_fence *fence)
> >>>> +{
> >>>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> >>>> +                        "rcu_read_lock() required for safe access to returned string");
> >>>> +
> >>>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >>>> +               return fence->ops->get_driver_name(fence);
> >>>> +       else
> >>>> +               return "detached-driver";
> >>>> +}
> >>>> +EXPORT_SYMBOL(dma_fence_driver_name);
> >>>> +
> >>>> +/**
> >>>> + * dma_fence_timeline_name - Access the timeline name
> >>>> + * @fence: the fence to query
> >>>> + *
> >>>> + * Returns a timeline name provided by the dma-fence implementation.
> >>>> + *
> >>>> + * IMPORTANT CONSIDERATION:
> >>>> + * Dma-fence contract stipulates that access to driver provided data (data not
> >>>> + * directly embedded into the object itself), such as the &dma_fence.lock and
> >>>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
> >>>> + * after the fence has been signalled. Drivers are allowed to free that data,
> >>>> + * and some do.
> >>>> + *
> >>>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
> >>>> + * between signalling the fence and freeing said data.
> >>>> + *
> >>>> + * As such access to the driver name is only valid inside a RCU locked section.
> >>>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
> >>>> + * by the &dma_fence_access_being and &dma_fence_access_end pair.
> >>>> + */
> >>>> +const char *dma_fence_timeline_name(struct dma_fence *fence)
> >>>> +{
> >>>> +       RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> >>>> +                        "rcu_read_lock() required for safe access to returned string");
> >>>> +
> >>>> +       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >>>> +               return fence->ops->get_driver_name(fence);
> >>>> +       else
> >>>> +               return "signaled-timeline";
> >>>
> >>> This means that trace_dma_fence_signaled() will get the wrong
> >>> timeline/driver name, which probably screws up perfetto and maybe
> >>> other tools.
> >>
> >> Do you think context and seqno are not enough for those tools and they
> >> actually rely on the names? It would sound weird if they decided to
> >> index anything on the names which are non-standardised between drivers,
> >> but I guess anything is possible.
> >
> > At some point perfetto uses the timeline name to put up a named fence
> > timeline, I'm not sure if it is using the name or context # for
> > subsequent fence events (namely, signalled).  I'd have to check the
> > code and get back to you.
>
> If you can it would be useful. Presumably it saves the names from the
> start edge of fence lifetime. But again, who knows.

Ok, it looks like perfetto is ok... mostly..
DrmTracker::GetFenceTimelineByContext() will try to lookup the
timeline by context #, and then if that fails, create a new timeline
with the name from the trace event, and add it to the hashmap.

It might be that "signaled-timeline" shows up if the first event seen
is the fence-signaled event.

> > There is also gpuvis, which I guess does something similar, but
> > haven't looked into it.  Idk if there are others.
>
> I know GpuVis uses DRM sched tracepoints since Pierre-Eric was
> explaining me about those in the context of tracing rework he did there.
> I am not sure about dma-fence tracepoints.
>
> +Pierre-Eric on the off chance you know from the top of your head how
> much GpuVis depends on them (dma-fence tracepoints).
>
> >>> Maybe it would work well enough just to move the
> >>> trace_dma_fence_signaled() call ahead of the test_and_set_bit()?  Idk
> >>> if some things will start getting confused if they see that trace
> >>> multiple times.
> >>
> >> Another alternative is to make this tracepoint access the names
> >> directly. It is under the lock so guaranteed not to get freed with
> >> drivers which will be made compliant with the documented rules.
> >
> > I guess it would have been better if, other than dma_fence_init
> > tracepoint, later tracepoints didn't include the driver/timeline
> > name.. that would have forced the use of the context.  But I guess too
> > late for that.  Perhaps the least bad thing to do is use the locking?
>
> You mean this last alternative I mentioned? I think that will work fine.
> I'll wait a little bit longer for more potential comments before re-spi
> ning with that.

yes

> Were you able to test the series for your use case? Assuming it is not
> upstream msm since I don't immediately see a path in msm_fence which
> gets freed at runtime?

Not yet, but I think it should because it is the exact same problem
your igt test triggers.

This is with my VM_BIND series, which will dynamically create/teardown
sched entities

BR,
-R