[PATCH v6 0/4] Fixing some dma-fence use-after-free

[PATCH v6 0/4] Fixing some dma-fence use-after-free

[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"

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
patches merged earlier 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. 

If this approach is acceptable the next steps will be to see if any other
drivers will need similar changes. 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.

v3:
 * Dropped the dma_fence_is_array|chain ops to flags conversion.
 * Dropped the i915 cleanup patch which is now independent.
 * Squashed dma-fence helpers with internal usage patches.
 * Restored missing hunk in "dma-fence: Use a flag for 64-bit seqnos".
 * Removed the AMDGPU_JOB_GET_TIMELINE_NAME macro.
 * Applied some r-b tags.

v4:
 * Tidied 64-bit seqno flags patch and fixed for amdgpu user queues which landed
   since.
 * Adjusted some dma-fence tracepoints to avoid asserts.
 * Protected tracepoints in dma_fence_wait_timeout() with the safe access
   annotations.
 * Dropped driver/timeline helper usage from amdgpu_trace.h.
 * Dropped signaled fence protection from i915 timeline name vfunc.

v5:
 * Rebased after prep patches merged.
 * Move dma_fence_access_end() earlier in dma_fence_release(). (Christian)

v6:
 * Replaced dma_fence_access_begin/end with rcu_read_(un)lock. (Christian)
 * Added __rcu annotation to dma_fence_driver|timeline_name() helpers. (Christian)
 * Re-ordered patches to accomodate above changes.

Tvrtko Ursulin (4):
  sync_file: Protect access to driver and timeline name
  drm/i915: Protect access to driver and timeline name
  dma-fence: Add safe access helpers and document the rules
  drm/xe: Make dma-fences compliant with the safe access rules

 drivers/dma-buf/dma-fence.c                  | 111 +++++++++++++++++--
 drivers/dma-buf/sync_file.c                  |  24 +++-
 drivers/gpu/drm/i915/gt/intel_gt_requests.c  |  10 +-
 drivers/gpu/drm/i915/i915_request.c          |   7 +-
 drivers/gpu/drm/i915/i915_sw_fence.c         |  10 +-
 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 +
 include/linux/dma-fence.h                    |  31 ++++--
 include/trace/events/dma_fence.h             |  38 ++++++-
 10 files changed, 209 insertions(+), 34 deletions(-)

-- 
2.48.0

[PATCH v6 1/4] 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.

This prepares the code for incoming dma-fence API changes which will start
asserting these accesses are done from a RCU locked section.

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

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 212df4b849fe..747e377fb954 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -135,12 +135,18 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
 		strscpy(buf, sync_file->user_name, len);
 	} else {
 		struct dma_fence *fence = sync_file->fence;
+		const char __rcu *timeline;
+		const char __rcu *driver;
 
+		rcu_read_lock();
+		driver = dma_fence_driver_name(fence);
+		timeline = dma_fence_timeline_name(fence);
 		snprintf(buf, len, "%s-%s%llu-%lld",
-			 dma_fence_driver_name(fence),
-			 dma_fence_timeline_name(fence),
+			 rcu_dereference(driver),
+			 rcu_dereference(timeline),
 			 fence->context,
 			 fence->seqno);
+		rcu_read_unlock();
 	}
 
 	return buf;
@@ -262,9 +268,17 @@ 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, dma_fence_timeline_name(fence),
+	const char __rcu *timeline;
+	const char __rcu *driver;
+
+	rcu_read_lock();
+
+	driver = dma_fence_driver_name(fence);
+	timeline = dma_fence_timeline_name(fence);
+
+	strscpy(info->obj_name, rcu_dereference(timeline),
 		sizeof(info->obj_name));
-	strscpy(info->driver_name, dma_fence_driver_name(fence),
+	strscpy(info->driver_name, rcu_dereference(driver),
 		sizeof(info->driver_name));
 
 	info->status = dma_fence_get_status(fence);
@@ -273,6 +287,8 @@ static int sync_fill_fence_info(struct dma_fence *fence,
 			ktime_to_ns(dma_fence_timestamp(fence)) :
 			ktime_set(0, 0);
 
+	rcu_read_unlock();
+
 	return info->status;
 }
 
-- 
2.48.0

[PATCH v6 2/4] 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.

This prepares the code for incoming dma-fence API changes which will start
asserting these accesses are done from a RCU locked section.

Now that the safe access is handled in the dma-fence API, the external
callers such as sync_file, and our internal code paths, we can drop the
similar protection from i915_fence_get_timeline_name().

This prepares the code for incoming dma-fence API changes which will start
asserting these accesses are done from a RCU locked section.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> # v1
---
v2:
 * Rebased for dma-fence API change.
---
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 10 ++++++++--
 drivers/gpu/drm/i915/i915_request.c         |  7 +++++--
 drivers/gpu/drm/i915/i915_sw_fence.c        | 10 ++++++++--
 3 files changed, 21 insertions(+), 6 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..93298820bee2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -250,11 +250,17 @@ void intel_gt_watchdog_work(struct work_struct *work)
 	llist_for_each_entry_safe(rq, rn, first, watchdog.link) {
 		if (!i915_request_completed(rq)) {
 			struct dma_fence *f = &rq->fence;
+			const char __rcu *timeline;
+			const char __rcu *driver;
 
+			rcu_read_lock();
+			driver = dma_fence_driver_name(f);
+			timeline = dma_fence_timeline_name(f);
 			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
-				  dma_fence_driver_name(f),
-				  dma_fence_timeline_name(f),
+				  rcu_dereference(driver),
+				  rcu_dereference(timeline),
 				  f->seqno);
+			rcu_read_unlock();
 			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..b9a2b2194c8f 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 = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
+	const char __rcu *timeline;
 	char buf[80] = "";
 	int x = 0;
 
@@ -2220,6 +2220,8 @@ void i915_request_show(struct drm_printer *m,
 
 	x = print_sched_attr(&rq->sched.attr, buf, x, sizeof(buf));
 
+	rcu_read_lock();
+	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
 	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
 		   prefix, indent, "                ",
 		   queue_status(rq),
@@ -2228,7 +2230,8 @@ void i915_request_show(struct drm_printer *m,
 		   fence_status(rq),
 		   buf,
 		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
-		   name);
+		   rcu_dereference(timeline));
+	rcu_read_unlock();
 }
 
 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 9edf659d18db..73e89b168fc3 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -430,16 +430,22 @@ static void timer_i915_sw_fence_wake(struct timer_list *t)
 	struct i915_sw_dma_fence_cb_timer *cb = timer_container_of(cb, t,
 								   timer);
 	struct i915_sw_fence *fence;
+	const char __rcu *timeline;
+	const char __rcu *driver;
 
 	fence = xchg(&cb->base.fence, NULL);
 	if (!fence)
 		return;
 
+	rcu_read_lock();
+	driver = dma_fence_driver_name(cb->dma);
+	timeline = dma_fence_timeline_name(cb->dma);
 	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),
+		  rcu_dereference(driver),
+		  rcu_dereference(timeline),
 		  cb->dma->seqno,
 		  i915_sw_fence_debug_hint(fence));
+	rcu_read_unlock();
 
 	i915_sw_fence_set_error_once(fence, -ETIMEDOUT);
 	i915_sw_fence_complete(fence);
-- 
2.48.0

[PATCH v6 3/4] 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
rcu_read_lock() and rcu_read_unlock().

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 rcu_read_unlock().

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_release() and
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      | 111 ++++++++++++++++++++++++++++---
 include/linux/dma-fence.h        |  31 ++++++---
 include/trace/events/dma_fence.h |  38 +++++++++--
 3 files changed, 157 insertions(+), 23 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 74f9e4b665e3..3f78c56b58dc 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 
 	dma_fence_enable_sw_signaling(fence);
 
-	trace_dma_fence_wait_start(fence);
+	if (trace_dma_fence_wait_start_enabled()) {
+		rcu_read_lock();
+		trace_dma_fence_wait_start(fence);
+		rcu_read_unlock();
+	}
 	if (fence->ops->wait)
 		ret = fence->ops->wait(fence, intr, timeout);
 	else
 		ret = dma_fence_default_wait(fence, intr, timeout);
-	trace_dma_fence_wait_end(fence);
+	if (trace_dma_fence_wait_end_enabled()) {
+		rcu_read_lock();
+		trace_dma_fence_wait_end(fence);
+		rcu_read_unlock();
+	}
 	return ret;
 }
 EXPORT_SYMBOL(dma_fence_wait_timeout);
@@ -533,16 +541,23 @@ void dma_fence_release(struct kref *kref)
 	struct dma_fence *fence =
 		container_of(kref, struct dma_fence, refcount);
 
+	rcu_read_lock();
 	trace_dma_fence_destroy(fence);
 
-	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",
-		 dma_fence_driver_name(fence),
-		 dma_fence_timeline_name(fence),
-		 fence->context, fence->seqno)) {
+	if (!list_empty(&fence->cb_list) &&
+	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		const char __rcu *timeline;
+		const char __rcu *driver;
 		unsigned long flags;
 
+		driver = dma_fence_driver_name(fence);
+		timeline = dma_fence_timeline_name(fence);
+
+		WARN(1,
+		     "Fence %s:%s:%llx:%llx released with pending signals!\n",
+		     rcu_dereference(driver), rcu_dereference(timeline),
+		     fence->context, fence->seqno);
+
 		/*
 		 * Failed to signal before release, likely a refcounting issue.
 		 *
@@ -556,6 +571,8 @@ void dma_fence_release(struct kref *kref)
 		spin_unlock_irqrestore(fence->lock, flags);
 	}
 
+	rcu_read_unlock();
+
 	if (fence->ops->release)
 		fence->ops->release(fence);
 	else
@@ -982,11 +999,21 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
  */
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
 {
+	const char __rcu *timeline;
+	const char __rcu *driver;
+
+	rcu_read_lock();
+
+	timeline = dma_fence_timeline_name(fence);
+	driver = dma_fence_driver_name(fence);
+
 	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
-		   dma_fence_driver_name(fence),
-		   dma_fence_timeline_name(fence),
+		   rcu_dereference(driver),
+		   rcu_dereference(timeline),
 		   fence->seqno,
 		   dma_fence_is_signaled(fence) ? "" : "un");
+
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL(dma_fence_describe);
 
@@ -1055,3 +1082,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
 			 BIT(DMA_FENCE_FLAG_SEQNO64_BIT));
 }
 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 &rcu_read_lock and &rcu_read_unlock pair.
+ */
+const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
+{
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "RCU protection is 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 &rcu_read_lock and &rcu_read_unlock pair.
+ */
+const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
+{
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "RCU protection is 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 10a849cb2d3f..64639e104110 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -378,15 +378,28 @@ 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);
-}
+/**
+ * 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
+ * &rcu_read_lock and &rcu_read_unlock 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.
+ *
+ */
+const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
+const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
index 84c83074ee81..4814a65b68dc 100644
--- a/include/trace/events/dma_fence.h
+++ b/include/trace/events/dma_fence.h
@@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence,
 		  __entry->seqno)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_emit,
+/*
+ * Safe only for call sites which are guaranteed to not race with fence
+ * signaling,holding the fence->lock and having checked for not signaled, or the
+ * signaling path itself.
+ */
+DECLARE_EVENT_CLASS(dma_fence_unsignaled,
+
+	TP_PROTO(struct dma_fence *fence),
+
+	TP_ARGS(fence),
+
+	TP_STRUCT__entry(
+		__string(driver, fence->ops->get_driver_name(fence))
+		__string(timeline, fence->ops->get_timeline_name(fence))
+		__field(unsigned int, context)
+		__field(unsigned int, seqno)
+	),
+
+	TP_fast_assign(
+		__assign_str(driver);
+		__assign_str(timeline);
+		__entry->context = fence->context;
+		__entry->seqno = fence->seqno;
+	),
+
+	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
+		  __get_str(driver), __get_str(timeline), __entry->context,
+		  __entry->seqno)
+);
+
+DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit,
 
 	TP_PROTO(struct dma_fence *fence),
 
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_init,
+DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init,
 
 	TP_PROTO(struct dma_fence *fence),
 
@@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
+DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal,
 
 	TP_PROTO(struct dma_fence *fence),
 
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_signaled,
+DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled,
 
 	TP_PROTO(struct dma_fence *fence),
 
-- 
2.48.0

[PATCH v6 4/4] 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>
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 2b61d017eeca..c7f45800ec6a 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1299,7 +1299,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));
 }
@@ -1482,6 +1486,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: [PATCH v6 3/4] dma-fence: Add safe access helpers and document the rules

[Christian König]

On 6/10/25 18:42, Tvrtko Ursulin 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
> rcu_read_lock() and rcu_read_unlock().
> 
> 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 rcu_read_unlock().
> 
> 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_release() and
> 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>

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

> ---
>  drivers/dma-buf/dma-fence.c      | 111 ++++++++++++++++++++++++++++---
>  include/linux/dma-fence.h        |  31 ++++++---
>  include/trace/events/dma_fence.h |  38 +++++++++--
>  3 files changed, 157 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 74f9e4b665e3..3f78c56b58dc 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>  
>  	dma_fence_enable_sw_signaling(fence);
>  
> -	trace_dma_fence_wait_start(fence);
> +	if (trace_dma_fence_wait_start_enabled()) {
> +		rcu_read_lock();
> +		trace_dma_fence_wait_start(fence);
> +		rcu_read_unlock();
> +	}
>  	if (fence->ops->wait)
>  		ret = fence->ops->wait(fence, intr, timeout);
>  	else
>  		ret = dma_fence_default_wait(fence, intr, timeout);
> -	trace_dma_fence_wait_end(fence);
> +	if (trace_dma_fence_wait_end_enabled()) {
> +		rcu_read_lock();
> +		trace_dma_fence_wait_end(fence);
> +		rcu_read_unlock();
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL(dma_fence_wait_timeout);
> @@ -533,16 +541,23 @@ void dma_fence_release(struct kref *kref)
>  	struct dma_fence *fence =
>  		container_of(kref, struct dma_fence, refcount);
>  
> +	rcu_read_lock();
>  	trace_dma_fence_destroy(fence);
>  
> -	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",
> -		 dma_fence_driver_name(fence),
> -		 dma_fence_timeline_name(fence),
> -		 fence->context, fence->seqno)) {
> +	if (!list_empty(&fence->cb_list) &&
> +	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +		const char __rcu *timeline;
> +		const char __rcu *driver;
>  		unsigned long flags;
>  
> +		driver = dma_fence_driver_name(fence);
> +		timeline = dma_fence_timeline_name(fence);
> +
> +		WARN(1,
> +		     "Fence %s:%s:%llx:%llx released with pending signals!\n",
> +		     rcu_dereference(driver), rcu_dereference(timeline),
> +		     fence->context, fence->seqno);
> +
>  		/*
>  		 * Failed to signal before release, likely a refcounting issue.
>  		 *
> @@ -556,6 +571,8 @@ void dma_fence_release(struct kref *kref)
>  		spin_unlock_irqrestore(fence->lock, flags);
>  	}
>  
> +	rcu_read_unlock();
> +
>  	if (fence->ops->release)
>  		fence->ops->release(fence);
>  	else
> @@ -982,11 +999,21 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>   */
>  void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>  {
> +	const char __rcu *timeline;
> +	const char __rcu *driver;
> +
> +	rcu_read_lock();
> +
> +	timeline = dma_fence_timeline_name(fence);
> +	driver = dma_fence_driver_name(fence);
> +
>  	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
> -		   dma_fence_driver_name(fence),
> -		   dma_fence_timeline_name(fence),
> +		   rcu_dereference(driver),
> +		   rcu_dereference(timeline),
>  		   fence->seqno,
>  		   dma_fence_is_signaled(fence) ? "" : "un");
> +
> +	rcu_read_unlock();
>  }
>  EXPORT_SYMBOL(dma_fence_describe);
>  
> @@ -1055,3 +1082,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>  			 BIT(DMA_FENCE_FLAG_SEQNO64_BIT));
>  }
>  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 &rcu_read_lock and &rcu_read_unlock pair.
> + */
> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
> +{
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +			 "RCU protection is 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 &rcu_read_lock and &rcu_read_unlock pair.
> + */
> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
> +{
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +			 "RCU protection is 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 10a849cb2d3f..64639e104110 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -378,15 +378,28 @@ 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);
> -}
> +/**
> + * 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
> + * &rcu_read_lock and &rcu_read_unlock 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.
> + *
> + */
> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
> index 84c83074ee81..4814a65b68dc 100644
> --- a/include/trace/events/dma_fence.h
> +++ b/include/trace/events/dma_fence.h
> @@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence,
>  		  __entry->seqno)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_emit,
> +/*
> + * Safe only for call sites which are guaranteed to not race with fence
> + * signaling,holding the fence->lock and having checked for not signaled, or the
> + * signaling path itself.
> + */
> +DECLARE_EVENT_CLASS(dma_fence_unsignaled,
> +
> +	TP_PROTO(struct dma_fence *fence),
> +
> +	TP_ARGS(fence),
> +
> +	TP_STRUCT__entry(
> +		__string(driver, fence->ops->get_driver_name(fence))
> +		__string(timeline, fence->ops->get_timeline_name(fence))
> +		__field(unsigned int, context)
> +		__field(unsigned int, seqno)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(driver);
> +		__assign_str(timeline);
> +		__entry->context = fence->context;
> +		__entry->seqno = fence->seqno;
> +	),
> +
> +	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
> +		  __get_str(driver), __get_str(timeline), __entry->context,
> +		  __entry->seqno)
> +);
> +
> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_init,
> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
> @@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  

Re: [PATCH v6 1/4] sync_file: Protect access to driver and timeline name

[Christian König]

On 6/10/25 18:42, Tvrtko Ursulin wrote:
> Protect the access to driver and timeline name which otherwise could be
> freed as dma-fence exported is signalling fences.
> 
> This prepares the code for incoming dma-fence API changes which will start
> asserting these accesses are done from a RCU locked section.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

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

> ---
>  drivers/dma-buf/sync_file.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 212df4b849fe..747e377fb954 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -135,12 +135,18 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
>  		strscpy(buf, sync_file->user_name, len);
>  	} else {
>  		struct dma_fence *fence = sync_file->fence;
> +		const char __rcu *timeline;
> +		const char __rcu *driver;
>  
> +		rcu_read_lock();
> +		driver = dma_fence_driver_name(fence);
> +		timeline = dma_fence_timeline_name(fence);
>  		snprintf(buf, len, "%s-%s%llu-%lld",
> -			 dma_fence_driver_name(fence),
> -			 dma_fence_timeline_name(fence),
> +			 rcu_dereference(driver),
> +			 rcu_dereference(timeline),
>  			 fence->context,
>  			 fence->seqno);
> +		rcu_read_unlock();
>  	}
>  
>  	return buf;
> @@ -262,9 +268,17 @@ 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, dma_fence_timeline_name(fence),
> +	const char __rcu *timeline;
> +	const char __rcu *driver;
> +
> +	rcu_read_lock();
> +
> +	driver = dma_fence_driver_name(fence);
> +	timeline = dma_fence_timeline_name(fence);
> +
> +	strscpy(info->obj_name, rcu_dereference(timeline),
>  		sizeof(info->obj_name));
> -	strscpy(info->driver_name, dma_fence_driver_name(fence),
> +	strscpy(info->driver_name, rcu_dereference(driver),
>  		sizeof(info->driver_name));
>  
>  	info->status = dma_fence_get_status(fence);
> @@ -273,6 +287,8 @@ static int sync_fill_fence_info(struct dma_fence *fence,
>  			ktime_to_ns(dma_fence_timestamp(fence)) :
>  			ktime_set(0, 0);
>  
> +	rcu_read_unlock();
> +
>  	return info->status;
>  }
>  

Re: [PATCH v6 3/4] dma-fence: Add safe access helpers and document the rules

[Tvrtko Ursulin]

On 11/06/2025 12:43, Christian König wrote:
> On 6/10/25 18:42, Tvrtko Ursulin 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
>> rcu_read_lock() and rcu_read_unlock().
>>
>> 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 rcu_read_unlock().
>>
>> 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_release() and
>> 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>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks Christian!

I have pinged xe maintainers for acks to merge the whole series via 
drm-misc-next and pending positive reply I can push it all.

Regards,

Tvrtko

> 
>> ---
>>   drivers/dma-buf/dma-fence.c      | 111 ++++++++++++++++++++++++++++---
>>   include/linux/dma-fence.h        |  31 ++++++---
>>   include/trace/events/dma_fence.h |  38 +++++++++--
>>   3 files changed, 157 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 74f9e4b665e3..3f78c56b58dc 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>>   
>>   	dma_fence_enable_sw_signaling(fence);
>>   
>> -	trace_dma_fence_wait_start(fence);
>> +	if (trace_dma_fence_wait_start_enabled()) {
>> +		rcu_read_lock();
>> +		trace_dma_fence_wait_start(fence);
>> +		rcu_read_unlock();
>> +	}
>>   	if (fence->ops->wait)
>>   		ret = fence->ops->wait(fence, intr, timeout);
>>   	else
>>   		ret = dma_fence_default_wait(fence, intr, timeout);
>> -	trace_dma_fence_wait_end(fence);
>> +	if (trace_dma_fence_wait_end_enabled()) {
>> +		rcu_read_lock();
>> +		trace_dma_fence_wait_end(fence);
>> +		rcu_read_unlock();
>> +	}
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL(dma_fence_wait_timeout);
>> @@ -533,16 +541,23 @@ void dma_fence_release(struct kref *kref)
>>   	struct dma_fence *fence =
>>   		container_of(kref, struct dma_fence, refcount);
>>   
>> +	rcu_read_lock();
>>   	trace_dma_fence_destroy(fence);
>>   
>> -	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",
>> -		 dma_fence_driver_name(fence),
>> -		 dma_fence_timeline_name(fence),
>> -		 fence->context, fence->seqno)) {
>> +	if (!list_empty(&fence->cb_list) &&
>> +	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>> +		const char __rcu *timeline;
>> +		const char __rcu *driver;
>>   		unsigned long flags;
>>   
>> +		driver = dma_fence_driver_name(fence);
>> +		timeline = dma_fence_timeline_name(fence);
>> +
>> +		WARN(1,
>> +		     "Fence %s:%s:%llx:%llx released with pending signals!\n",
>> +		     rcu_dereference(driver), rcu_dereference(timeline),
>> +		     fence->context, fence->seqno);
>> +
>>   		/*
>>   		 * Failed to signal before release, likely a refcounting issue.
>>   		 *
>> @@ -556,6 +571,8 @@ void dma_fence_release(struct kref *kref)
>>   		spin_unlock_irqrestore(fence->lock, flags);
>>   	}
>>   
>> +	rcu_read_unlock();
>> +
>>   	if (fence->ops->release)
>>   		fence->ops->release(fence);
>>   	else
>> @@ -982,11 +999,21 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>>    */
>>   void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>>   {
>> +	const char __rcu *timeline;
>> +	const char __rcu *driver;
>> +
>> +	rcu_read_lock();
>> +
>> +	timeline = dma_fence_timeline_name(fence);
>> +	driver = dma_fence_driver_name(fence);
>> +
>>   	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>> -		   dma_fence_driver_name(fence),
>> -		   dma_fence_timeline_name(fence),
>> +		   rcu_dereference(driver),
>> +		   rcu_dereference(timeline),
>>   		   fence->seqno,
>>   		   dma_fence_is_signaled(fence) ? "" : "un");
>> +
>> +	rcu_read_unlock();
>>   }
>>   EXPORT_SYMBOL(dma_fence_describe);
>>   
>> @@ -1055,3 +1082,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>>   			 BIT(DMA_FENCE_FLAG_SEQNO64_BIT));
>>   }
>>   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 &rcu_read_lock and &rcu_read_unlock pair.
>> + */
>> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>> +{
>> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> +			 "RCU protection is 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 &rcu_read_lock and &rcu_read_unlock pair.
>> + */
>> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
>> +{
>> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> +			 "RCU protection is 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 10a849cb2d3f..64639e104110 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -378,15 +378,28 @@ 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);
>> -}
>> +/**
>> + * 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
>> + * &rcu_read_lock and &rcu_read_unlock 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.
>> + *
>> + */
>> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>>   
>>   /**
>>    * dma_fence_is_signaled_locked - Return an indication if the fence
>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>> index 84c83074ee81..4814a65b68dc 100644
>> --- a/include/trace/events/dma_fence.h
>> +++ b/include/trace/events/dma_fence.h
>> @@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence,
>>   		  __entry->seqno)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_emit,
>> +/*
>> + * Safe only for call sites which are guaranteed to not race with fence
>> + * signaling,holding the fence->lock and having checked for not signaled, or the
>> + * signaling path itself.
>> + */
>> +DECLARE_EVENT_CLASS(dma_fence_unsignaled,
>> +
>> +	TP_PROTO(struct dma_fence *fence),
>> +
>> +	TP_ARGS(fence),
>> +
>> +	TP_STRUCT__entry(
>> +		__string(driver, fence->ops->get_driver_name(fence))
>> +		__string(timeline, fence->ops->get_timeline_name(fence))
>> +		__field(unsigned int, context)
>> +		__field(unsigned int, seqno)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__assign_str(driver);
>> +		__assign_str(timeline);
>> +		__entry->context = fence->context;
>> +		__entry->seqno = fence->seqno;
>> +	),
>> +
>> +	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>> +		  __get_str(driver), __get_str(timeline), __entry->context,
>> +		  __entry->seqno)
>> +);
>> +
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_init,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>> @@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
> 

Re: [PATCH v6 4/4] drm/xe: Make dma-fences compliant with the safe access rules

[Lucas De Marchi]

On Tue, Jun 10, 2025 at 05:42:26PM +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>
>Reviewed-by: Matthew Brost <matthew.brost@intel.com>


Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

for merging this through drm-misc tree.

Lucas De Marchi

Re: [PATCH v6 4/4] drm/xe: Make dma-fences compliant with the safe access rules

[Tvrtko Ursulin]

On 12/06/2025 18:49, Lucas De Marchi wrote:
> On Tue, Jun 10, 2025 at 05:42:26PM +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>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> 
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> for merging this through drm-misc tree.

Thanks!

I've now pushed the series drm-misc-next.

Btw there is also an IGT for xe I wrote ages ago^1, if you want to ping 
someone to review it or take it over. Might be useful to have permanent 
verification the UAF keeps being resolved.


Regards,

Tvrtko

1) https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2