[RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Philipp Stanner]

In our Rust code bases, I'm currently experiencing what we also
experience in C at many places: that the dma_fence contract's RCU
requirements make the API more fragile and put an unnecessary burden on
the API user. Most notably, in C the API behaving correctly now depends
on the API *user* waiting before free()s, waiting before unloads – all
because dma_fence is not properly synchronized.

The patches commit messages give more details about where I want to go
with this particular RFC.

My rough "vision" for dma_fence, where I think it should develop to, can
be described like this:

1. All users should use inline-lock.
2. The fence-signaled state should be our decoupling point.
3. The fence state must be consistently locked.
4. This means all callbacks are only invoked for unsignaled fences, and
   the fence cannot signal while a callback runs.
5. All fence API functions should be "atomically correct": they can
   always be called without any manual RCU guards in their proximity.
6. The fence lock should, in the long-term, become inaccessible for
   users.

Point №6 means that this:

dma_fence_lock_irqsave(…);
dma_fence_set_error(f, err);
dma_fence_signal_locked(f);
dma_fence_unlock_irqrestore(…);
synchronize_rcu();
kfree(driver_foo);

should simply become this:

dma_fence_signal(f, err);
kfree(driver_foo);

So we would end up with a simpler, more robust and more maintainable
API.

I gave this basic testing on a Nouveau desktop and it runs fine.
dma-buf-tests also run fine. I can do more tests on Xe and amdgpu, maybe
on Mali, too.

Since the series is mostly about locking the callbacks, all of which
except for set_deadline() are *already* locked, I am confident that this
can be done.

Feedback welcome

P.

Philipp Stanner (3):
  dma-fence: Remove RCU protection for fence->ops
  dma-fence: use lock to protect against module-unload
  dma-fence: Support unload for ops->get_XXX_name()

 drivers/dma-buf/dma-fence.c                 | 144 +++++++-------------
 drivers/dma-buf/sync_file.c                 |  29 ++--
 drivers/gpu/drm/i915/gt/intel_gt_requests.c |  16 +--
 drivers/gpu/drm/i915/i915_request.c         |   9 +-
 drivers/gpu/drm/i915/i915_sw_fence.c        |  15 +-
 include/linux/dma-fence.h                   |  63 ++++-----
 include/trace/events/dma_fence.h            |  12 +-
 7 files changed, 113 insertions(+), 175 deletions(-)

-- 
2.56.0

[RFC PATCH v2 1/3] dma-fence: Remove RCU protection for fence->ops

[Philipp Stanner]

One issue dma_fence suffers from is that the implementation can run into
the driver through dma_fence_ops pointers. The driver, however, might
have unloaded at this point, or the callbacks might run into other UAF
issues.

It has been attempted to solve this with RCU protection, creating a set
of sophisticated rules that the driver must follow. Notably, the driver
must delay unloading by an RCU grace period, and must delay freeing of
resources needed by the callbacks by a grace period also.

This is fragile because the dma_fence implementation has no control over
it.

Subsequent patches present a more robust implementation. Remove the RCU
protection for the backend-ops.

As an additional bonus, this allows users again to identify their fence
through the ops-pointer address, as is frequently done throughout the
kernel.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 78 +++++++------------------------------
 include/linux/dma-fence.h   | 18 +++------
 2 files changed, 19 insertions(+), 77 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index bd58688b81a7..5dab647e3f3f 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -353,7 +353,6 @@ void __dma_fence_might_wait(void)
 void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 				      ktime_t timestamp)
 {
-	const struct dma_fence_ops *ops;
 	struct dma_fence_cb *cur, *tmp;
 	struct list_head cb_list;
 
@@ -365,15 +364,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	trace_dma_fence_signaled(fence);
 
-	/*
-	 * When neither a release nor a wait operation is specified set the ops
-	 * pointer to NULL to allow the fence structure to become independent
-	 * from who originally issued it.
-	 */
-	ops = rcu_dereference_protected(fence->ops, true);
-	if (!ops->release && !ops->wait)
-		RCU_INIT_POINTER(fence->ops, NULL);
-
 	/* Stash the cb_list before replacing it with the timestamp */
 	list_replace(&fence->cb_list, &cb_list);
 
@@ -524,7 +514,7 @@ EXPORT_SYMBOL(dma_fence_signal);
 signed long
 dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 {
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 	signed long ret;
 
 	if (WARN_ON(timeout < 0))
@@ -537,19 +527,11 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 	dma_fence_enable_signaling(fence);
 
 	rcu_read_lock();
-	ops = rcu_dereference(fence->ops);
 	trace_dma_fence_wait_start(fence);
-	if (ops && ops->wait) {
-		/*
-		 * Implementing the wait ops is deprecated and not supported for
-		 * issuers of fences who need their lifetime to be independent
-		 * of their module after they signal, so it is ok to use the
-		 * ops outside the RCU protected section.
-		 */
-		rcu_read_unlock();
+	rcu_read_unlock();
+	if (ops->wait) {
 		ret = ops->wait(fence, intr, timeout);
 	} else {
-		rcu_read_unlock();
 		ret = dma_fence_default_wait(fence, intr, timeout);
 	}
 	if (trace_dma_fence_wait_end_enabled()) {
@@ -572,7 +554,7 @@ void dma_fence_release(struct kref *kref)
 {
 	struct dma_fence *fence =
 		container_of(kref, struct dma_fence, refcount);
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 
 	rcu_read_lock();
 	trace_dma_fence_destroy(fence);
@@ -604,8 +586,7 @@ void dma_fence_release(struct kref *kref)
 		dma_fence_unlock_irqrestore(fence, flags);
 	}
 
-	ops = rcu_dereference(fence->ops);
-	if (ops && ops->release)
+	if (ops->release)
 		ops->release(fence);
 	else
 		dma_fence_free(fence);
@@ -628,7 +609,7 @@ EXPORT_SYMBOL(dma_fence_free);
 
 static bool __dma_fence_enable_signaling(struct dma_fence *fence)
 {
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 	bool was_set;
 
 	dma_fence_assert_held(fence);
@@ -639,18 +620,14 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
 	if (dma_fence_test_signaled_flag(fence))
 		return false;
 
-	rcu_read_lock();
-	ops = rcu_dereference(fence->ops);
-	if (!was_set && ops && ops->enable_signaling) {
+	if (!was_set && ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!ops->enable_signaling(fence)) {
-			rcu_read_unlock();
 			dma_fence_signal_locked(fence);
 			return false;
 		}
 	}
-	rcu_read_unlock();
 
 	return true;
 }
@@ -1019,13 +996,10 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
  */
 void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
 {
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 
-	rcu_read_lock();
-	ops = rcu_dereference(fence->ops);
-	if (ops && ops->set_deadline && !dma_fence_is_signaled(fence))
+	if (ops->set_deadline && !dma_fence_is_signaled(fence))
 		ops->set_deadline(fence, deadline);
-	rcu_read_unlock();
 }
 EXPORT_SYMBOL(dma_fence_set_deadline);
 
@@ -1065,13 +1039,7 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
-	/*
-	 * While it is counter intuitive to protect a constant function pointer
-	 * table by RCU it allows modules to wait for an RCU grace period
-	 * before they unload, to make sure that nobody is executing their
-	 * functions any more.
-	 */
-	RCU_INIT_POINTER(fence->ops, ops);
+	fence->ops = ops;
 	INIT_LIST_HEAD(&fence->cb_list);
 	fence->context = context;
 	fence->seqno = seqno;
@@ -1166,17 +1134,8 @@ EXPORT_SYMBOL(dma_fence_init64);
  */
 const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
 {
-	const struct dma_fence_ops *ops;
-
-	/* RCU protection is required for safe access to returned string */
-	ops = rcu_dereference(fence->ops);
-
-	/*
-	 * Make load ordering irrelevant by checking both signaled state and ops
-	 * pointer and ops pointer is only set to NULL on newer implementations.
-	 */
-	if (!dma_fence_test_signaled_flag(fence) && ops)
-		return (const char __rcu *)ops->get_driver_name(fence);
+	if (!dma_fence_test_signaled_flag(fence))
+		return (const char __rcu *)fence->ops->get_driver_name(fence);
 	else
 		return (const char __rcu *)"detached-driver";
 }
@@ -1204,17 +1163,8 @@ EXPORT_SYMBOL(dma_fence_driver_name);
  */
 const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
 {
-	const struct dma_fence_ops *ops;
-
-	/* RCU protection is required for safe access to returned string */
-	ops = rcu_dereference(fence->ops);
-
-	/*
-	 * Make load ordering irrelevant by checking both signaled state and ops
-	 * pointer and ops pointer is only set to NULL on newer implementations.
-	 */
-	if (!dma_fence_test_signaled_flag(fence) && ops)
-		return (const char __rcu *)ops->get_timeline_name(fence);
+	if (!dma_fence_test_signaled_flag(fence))
+		return (const char __rcu *)fence->ops->get_timeline_name(fence);
 	else
 		return (const char __rcu *)"signaled-timeline";
 }
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index dd07d128adc3..874848a4aec4 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -72,7 +72,7 @@ struct dma_fence {
 		spinlock_t *extern_lock;
 		spinlock_t inline_lock;
 	};
-	const struct dma_fence_ops __rcu *ops;
+	const struct dma_fence_ops *ops;
 	/*
 	 * We clear the callback list on kref_put so that by the time we
 	 * release the fence it is unused. No one should be adding to the
@@ -516,19 +516,15 @@ dma_fence_test_signaled_flag(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled_locked(struct dma_fence *fence)
 {
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 
 	if (dma_fence_test_signaled_flag(fence))
 		return true;
 
-	rcu_read_lock();
-	ops = rcu_dereference(fence->ops);
 	if (ops && ops->signaled && ops->signaled(fence)) {
-		rcu_read_unlock();
 		dma_fence_signal_locked(fence);
 		return true;
 	}
-	rcu_read_unlock();
 
 	return false;
 }
@@ -552,19 +548,15 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
-	const struct dma_fence_ops *ops;
+	const struct dma_fence_ops *ops = fence->ops;
 
 	if (dma_fence_test_signaled_flag(fence))
 		return true;
 
-	rcu_read_lock();
-	ops = rcu_dereference(fence->ops);
 	if (ops && ops->signaled && ops->signaled(fence)) {
-		rcu_read_unlock();
 		dma_fence_signal(fence);
 		return true;
 	}
-	rcu_read_unlock();
 
 	return false;
 }
@@ -769,7 +761,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
  */
 static inline bool dma_fence_is_array(struct dma_fence *fence)
 {
-	return rcu_access_pointer(fence->ops) == &dma_fence_array_ops;
+	return fence->ops == &dma_fence_array_ops;
 }
 
 /**
@@ -780,7 +772,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
  */
 static inline bool dma_fence_is_chain(struct dma_fence *fence)
 {
-	return rcu_access_pointer(fence->ops) == &dma_fence_chain_ops;
+	return fence->ops == &dma_fence_chain_ops;
 }
 
 /**
-- 
2.56.0

[RFC PATCH v2 2/3] dma-fence: use lock to protect against module-unload

[Philipp Stanner]

dma_fence's lock already guards the signaled-state, and most fence
backend_ops callbacks are already called with the lock taken.

The contract already relies on the fact that a producer must signal all
fences before unloading. The callbacks then must never run on a fence
that is already signaled.

Currently, driver unload is supported by RCU-checking the fence->ops
pointer. However, the design ended up with two partially redundant
mechanisms, the other being the signaled-state:

	/*
	 * Make load ordering irrelevant by checking both signaled state and ops
	 * pointer and ops pointer is only set to NULL on newer implementations.
	 */
	if (!dma_fence_test_signaled_flag(fence) && ops)

We have seen multiple bugs, such as ordering issues, because of that,
for instance in:

035219a760ed ("dma-buf: dma-fence: Fix potential NULL pointer dereference")
3ed11c671ff7 ("dma-buf/dma-fence: fix checking signaling bit for timeline and driver name v3")

Additionally, our RCU requirements can by definition not be controled by
dma_fence, resulting in a burden on users and an unnecessary grace
period delay. Most recently, this came to light in CRTC:

https://lore.kernel.org/dri-devel/d54ba24b0a4748b5c00052d4542460ea014aa50d.camel@mailbox.org/

It appears to be the natural solution to guard the fence's signaled
state *and* accesses to the driver and its data through the lock.
However, in the past it has been argued that one cannot hold the
fence-lock while running a callback, because this might lead to
deadlocks, since drivers might want to access data protected by a driver
lock.

De-facto, it is already the case that ops callbacks are called with the
lock taken, and don't run into the driver if the fence is signaled
already. Thus, we merely need to make that rule official.

This commit proposes making it standard to guard against UAF and module
unload by holding the lock while running fence->ops.

The current situation of the ops is like this:

- release() and wait() are deprecated and can never work with module
  unload in any case, thus neither RCU nor lock help them anyways.

- enable_signaling() and is_signaled() are already being invoked with the
  lock, thus continue doing so is fine.

- get_driver_name() and get_timeline_name() can elegantly be solved by
  copying the name-strings. See subsequent patch.

- set_deadline() is the only callback for which a locked call would be
  new. It has very few implementors, so correctness is easy to verify.

For dma_fence_is_signaled(), dma_fence_is_signaled_locked()'s existence
proofs that we can lock the former's ops->signaled() call, too.

The fact that most callbacks are already invoked with the lock taken
proofs that the drivers support it, and doing it consistently will make
the entire API far more robust and consistent than RCU-guarding.

Current users keep working, and future users can be well-informed about
the locking considerations.

Let's go for full, consistent locking. Add lock guards around the two
missing paths running into non-deprecated backend_ops.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c |  7 ++++++-
 include/linux/dma-fence.h   | 21 +++++++++++++++++----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 5dab647e3f3f..e4350bce9cf9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -997,9 +997,14 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
 {
 	const struct dma_fence_ops *ops = fence->ops;
+	unsigned long flags;
 
-	if (ops->set_deadline && !dma_fence_is_signaled(fence))
+	dma_fence_lock_irqsave(fence, flags);
+
+	if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))
 		ops->set_deadline(fence, deadline);
+
+	dma_fence_unlock_irqrestore(fence, flags);
 }
 EXPORT_SYMBOL(dma_fence_set_deadline);
 
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 874848a4aec4..2223d75c5a49 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -549,16 +549,29 @@ static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
 	const struct dma_fence_ops *ops = fence->ops;
+	unsigned long flags;
+	bool ret = false;
 
+	/* Fast-path optimization. */
 	if (dma_fence_test_signaled_flag(fence))
 		return true;
 
-	if (ops && ops->signaled && ops->signaled(fence)) {
-		dma_fence_signal(fence);
-		return true;
+	dma_fence_lock_irqsave(fence, flags);
+
+	if (dma_fence_test_signaled_flag(fence)) {
+		ret = true;
+		goto out;
 	}
 
-	return false;
+	if (ops->signaled && ops->signaled(fence)) {
+		ret = true;
+		dma_fence_signal_locked(fence);
+	}
+
+out:
+	dma_fence_unlock_irqrestore(fence, flags);
+
+	return ret;
 }
 
 /**
-- 
2.56.0

[RFC PATCH v2 3/3] dma-fence: Support unload for ops->get_XXX_name()

[Philipp Stanner]

dma_fence's backend_ops get_driver_name() and get_timeline_name() have
an unnecessary and complicated RCU requirement.

Usage in sync_file, however, already shows how it can be done
significantly simpler: Copy the name strings to the user.

All that needs to be ensured for that is that the signaled-state is
checked and that it cannot change while the callback runs, i.e., the
lock must be held.

Adjust the two callbacks so that they deep-copy name parameters under
lock-protection.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c                 | 73 ++++++++++-----------
 drivers/dma-buf/sync_file.c                 | 29 ++++----
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 16 ++---
 drivers/gpu/drm/i915/i915_request.c         |  9 ++-
 drivers/gpu/drm/i915/i915_sw_fence.c        | 15 ++---
 include/linux/dma-fence.h                   | 22 ++-----
 include/trace/events/dma_fence.h            | 12 ++--
 7 files changed, 77 insertions(+), 99 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index e4350bce9cf9..d473d4ea601a 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -555,23 +555,22 @@ void dma_fence_release(struct kref *kref)
 	struct dma_fence *fence =
 		container_of(kref, struct dma_fence, refcount);
 	const struct dma_fence_ops *ops = fence->ops;
+	const unsigned short len = 64;
+	char timeline[len];
+	char driver[len];
 
-	rcu_read_lock();
 	trace_dma_fence_destroy(fence);
 
 	if (!list_empty(&fence->cb_list) &&
 	    !dma_fence_test_signaled_flag(fence)) {
-		const char __rcu *timeline;
-		const char __rcu *driver;
 		unsigned long flags;
 
-		driver = dma_fence_driver_name(fence);
-		timeline = dma_fence_timeline_name(fence);
+		dma_fence_driver_name(fence, driver, len);
+		dma_fence_timeline_name(fence, timeline, len);
 
 		WARN(1,
 		     "Fence %s:%s:%llx:%llx released with pending signals!\n",
-		     rcu_dereference(driver), rcu_dereference(timeline),
-		     fence->context, fence->seqno);
+		     driver, timeline, fence->context, fence->seqno);
 
 		/*
 		 * Failed to signal before release, likely a refcounting issue.
@@ -590,7 +589,6 @@ void dma_fence_release(struct kref *kref)
 		ops->release(fence);
 	else
 		dma_fence_free(fence);
-	rcu_read_unlock();
 }
 EXPORT_SYMBOL(dma_fence_release);
 
@@ -1017,23 +1015,20 @@ 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 *)"";
-	const char __rcu *driver = (const char __rcu *)"";
-	const char *signaled = "";
-
-	rcu_read_lock();
+	const unsigned short len = 64;
+	char timeline[len];
+	char driver[len];
+	char *signaled;
 
 	if (!dma_fence_is_signaled(fence)) {
-		timeline = dma_fence_timeline_name(fence);
-		driver = dma_fence_driver_name(fence);
+		dma_fence_timeline_name(fence, timeline, len);
+		dma_fence_driver_name(fence, driver, len);
 		signaled = "un";
 	}
 
 	seq_printf(seq, "%llu:%llu %s %s %ssignalled\n",
 		   fence->context, fence->seqno, timeline, driver,
 		   signaled);
-
-	rcu_read_unlock();
 }
 EXPORT_SYMBOL(dma_fence_describe);
 
@@ -1129,20 +1124,22 @@ EXPORT_SYMBOL(dma_fence_init64);
  * 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)
+void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short len)
 {
+	unsigned long flags;
+	const char *tmp;
+
+	dma_fence_lock_irqsave(fence, flags);
+
 	if (!dma_fence_test_signaled_flag(fence))
-		return (const char __rcu *)fence->ops->get_driver_name(fence);
+		tmp = fence->ops->get_driver_name(fence);
 	else
-		return (const char __rcu *)"detached-driver";
+		tmp = "detached-driver";
+
+	strscpy(s, tmp, len);
+
+	dma_fence_unlock_irqrestore(fence, flags);
 }
 EXPORT_SYMBOL(dma_fence_driver_name);
 
@@ -1158,19 +1155,21 @@ EXPORT_SYMBOL(dma_fence_driver_name);
  * 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)
+void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned short len)
 {
+	unsigned long flags;
+	const char *tmp;
+
+	dma_fence_lock_irqsave(fence, flags);
+
 	if (!dma_fence_test_signaled_flag(fence))
-		return (const char __rcu *)fence->ops->get_timeline_name(fence);
+		tmp = fence->ops->get_timeline_name(fence);
 	else
-		return (const char __rcu *)"signaled-timeline";
+		tmp = "signaled-timeline";
+
+	strscpy(s, tmp, len);
+
+	dma_fence_unlock_irqrestore(fence, flags);
 }
 EXPORT_SYMBOL(dma_fence_timeline_name);
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2166bbdf7e4a..0f08713b3b5e 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -18,6 +18,8 @@
 #include <linux/sync_file.h>
 #include <uapi/linux/sync_file.h>
 
+#define FENCE_NAME_LEN 64
+
 static const struct file_operations sync_file_fops;
 
 static struct sync_file *sync_file_alloc(void)
@@ -131,22 +133,21 @@ EXPORT_SYMBOL(sync_file_get_fence);
  */
 char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
 {
+	char timeline[FENCE_NAME_LEN];
+	char driver[FENCE_NAME_LEN];
+
 	if (sync_file->user_name[0]) {
 		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);
+		dma_fence_driver_name(fence, driver, FENCE_NAME_LEN);
+		dma_fence_timeline_name(fence, timeline, FENCE_NAME_LEN);
 		snprintf(buf, len, "%s-%s%llu-%lld",
-			 rcu_dereference(driver),
-			 rcu_dereference(timeline),
+			 driver,
+			 timeline,
 			 fence->context,
 			 fence->seqno);
-		rcu_read_unlock();
 	}
 
 	return buf;
@@ -268,19 +269,11 @@ 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)
 {
-	const char __rcu *timeline;
-	const char __rcu *driver;
+	dma_fence_timeline_name(fence, info->obj_name, sizeof(info->obj_name));
+	dma_fence_driver_name(fence, info->driver_name, sizeof(info->driver_name));
 
 	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, rcu_dereference(driver),
-		sizeof(info->driver_name));
-
 	info->status = dma_fence_get_status(fence);
 	info->timestamp_ns =
 		dma_fence_is_signaled(fence) ?
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 93298820bee2..a37069ca6d2a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -240,8 +240,12 @@ void intel_gt_watchdog_work(struct work_struct *work)
 {
 	struct intel_gt *gt =
 		container_of(work, typeof(*gt), watchdog.work);
+	const unsigned short len = 64;
 	struct i915_request *rq, *rn;
 	struct llist_node *first;
+	char timeline[len];
+	char driver[len];
+
 
 	first = llist_del_all(&gt->watchdog.list);
 	if (!first)
@@ -250,17 +254,13 @@ 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);
+			dma_fence_driver_name(f, driver, len);
+			dma_fence_timeline_name(f, timeline, len);
 			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
-				  rcu_dereference(driver),
-				  rcu_dereference(timeline),
+				  driver,
+				  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 d2c7b1090df0..c81902d8a89a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2185,7 +2185,8 @@ void i915_request_show(struct drm_printer *m,
 		       const char *prefix,
 		       int indent)
 {
-	const char __rcu *timeline;
+	const unsigned short len = 64;
+	char timeline[len];
 	char buf[80] = "";
 	int x = 0;
 
@@ -2221,8 +2222,7 @@ 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);
+	dma_fence_timeline_name((struct dma_fence *)&rq->fence, timeline, len);
 	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
 		   prefix, indent, "                ",
 		   queue_status(rq),
@@ -2231,8 +2231,7 @@ void i915_request_show(struct drm_printer *m,
 		   fence_status(rq),
 		   buf,
 		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
-		   rcu_dereference(timeline));
-	rcu_read_unlock();
+		   timeline);
 }
 
 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 f24f616e23ee..08b5f96ae2a5 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -429,23 +429,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);
+	const unsigned short len = 64;
 	struct i915_sw_fence *fence;
-	const char __rcu *timeline;
-	const char __rcu *driver;
+	char timeline[len];
+	char driver[len];
 
 	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);
+	dma_fence_driver_name(cb->dma, driver, len);
+	dma_fence_timeline_name(cb->dma, timeline, len);
 	pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n",
-		  rcu_dereference(driver),
-		  rcu_dereference(timeline),
+		  driver,
+		  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);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 2223d75c5a49..08ecbc101f4c 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -459,25 +459,11 @@ void dma_fence_enable_signaling(struct dma_fence *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.
- *
+ * Provides naming information strings about the fence if it is not yet signaled.
+ * Otherwise, provides default strings.
  */
-const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
-const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
+void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short len);
+void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned short len);
 
 /*
  * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet.
diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
index 5b10a9e06fb4..f3b929b98cf4 100644
--- a/include/trace/events/dma_fence.h
+++ b/include/trace/events/dma_fence.h
@@ -9,6 +9,8 @@
 
 struct dma_fence;
 
+#define FENCE_NAME_LEN 64
+
 DECLARE_EVENT_CLASS(dma_fence,
 
 	TP_PROTO(struct dma_fence *fence),
@@ -16,21 +18,21 @@ DECLARE_EVENT_CLASS(dma_fence,
 	TP_ARGS(fence),
 
 	TP_STRUCT__entry(
-		__string(driver, dma_fence_driver_name(fence))
-		__string(timeline, dma_fence_timeline_name(fence))
+		__array(char, driver, FENCE_NAME_LEN)
+		__array(char, timeline, FENCE_NAME_LEN)
 		__field(unsigned int, context)
 		__field(unsigned int, seqno)
 	),
 
 	TP_fast_assign(
-		__assign_str(driver);
-		__assign_str(timeline);
+		dma_fence_driver_name(fence, __entry->driver, FENCE_NAME_LEN);
+		dma_fence_timeline_name(fence, __entry->timeline, FENCE_NAME_LEN);
 		__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->driver, __entry->timeline, __entry->context,
 		  __entry->seqno)
 );
 
-- 
2.56.0

Re: [RFC PATCH v2 2/3] dma-fence: use lock to protect against module-unload

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dma-fence: deadlock in dma_fence_set_deadline() holding fence lock
- [High] dma-fence: recursive lock deadlock in dma_fence_is_signaled()
- [High] dma-fence: module .rodata UAF on ops in dma_fence_set_deadline()
--

commit be24351242cf77e69e189b5c2659a451b6892af0
Author: Philipp Stanner <phasta@kernel.org>

dma-fence: use lock to protect against module-unload

This commit acquires the dma_fence spinlock in dma_fence_set_deadline()
and dma_fence_is_signaled() to serialize fence->ops callback invocations
against signaling and module unload.

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 5dab647e3f3f9..e4350bce9cf9b 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -997,9 +997,14 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
>  void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>  {
>  	const struct dma_fence_ops *ops = fence->ops;
> +	unsigned long flags;
> +
> +	dma_fence_lock_irqsave(fence, flags);
>  
> -	if (ops->set_deadline && !dma_fence_is_signaled(fence))
> +	if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))
>  		ops->set_deadline(fence, deadline);
> +
> +	dma_fence_unlock_irqrestore(fence, flags);
>  }
>  EXPORT_SYMBOL(dma_fence_set_deadline);

[Severity: High]
Will holding the fence spinlock across ops->set_deadline() here cause
deadlocks in the existing set_deadline implementations?

The comment on struct dma_fence_ops in include/linux/dma-fence.h states:

	 * This is called without &dma_fence.lock held, it can be called
	 * multiple times and from any context.  Locking is up to the callee
	 * if it has some state to manage.

All three non-container implementations of set_deadline acquire the
fence's own spinlock inside the callback, which will self-deadlock on the
same CPU when called on an unsignaled fence (for example via the
SYNC_IOC_SET_DEADLINE ioctl or DRM atomic commit paths):

1. drivers/gpu/drm/scheduler/sched_fence.c:
   drm_sched_fence_set_deadline_finished() calls
   dma_fence_lock_irqsave(f, flags) on the same fence:

	dma_fence_lock_irqsave(f, flags);

	/* If we already have an earlier deadline, keep it: */
	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
	    ktime_before(fence->deadline, deadline)) {
		dma_fence_unlock_irqrestore(f, flags);
		return;
	}

   In addition, drm_sched_fence_set_deadline_finished() then propagates
   the deadline to the hardware parent fence:

	parent = smp_load_acquire(&fence->parent);
	if (parent)
		dma_fence_set_deadline(parent, deadline);

   If the outer dma_fence_set_deadline() holds s_fence->finished's lock
   across ops->set_deadline(), calling dma_fence_set_deadline(parent, ...)
   acquires parent's lock under s_fence->finished's lock. However, job
   completion in drivers/gpu/drm/scheduler/sched_main.c runs
   dma_fence_signal(parent) -> drm_sched_job_done_cb() ->
   drm_sched_job_done() -> drm_sched_fence_finished() ->
   dma_fence_signal(&s_fence->finished), which acquires
   s_fence->finished's lock while holding parent's lock, creating an
   AB-BA lock-order inversion.

2. drivers/gpu/drm/msm/msm_fence.c:
   msm_fence_set_deadline() acquires &fctx->spinlock:

	spin_lock_irqsave(&fctx->spinlock, flags);

   which is the fence's extern_lock passed to dma_fence_init() in
   msm_fence_init().

3. drivers/dma-buf/sw_sync.c:
   timeline_fence_set_deadline() acquires the fence lock directly:

	dma_fence_lock_irqsave(fence, flags);

[Severity: High]
Also in dma_fence_set_deadline() above, can this still access freed
driver memory or unmapped module .rodata when called on an already
signaled fence after module unload?

First, because there is no unlocked dma_fence_test_signaled_flag(fence)
check before dma_fence_lock_irqsave(fence, flags), calling
dma_fence_set_deadline() on a signaled fence unconditionally touches
dma_fence_spinlock(fence), which may be an extern_lock freed by the
driver after signaling.

Second, inside the lock:

	if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))

evaluates ops->set_deadline before checking
!dma_fence_is_signaled_locked(fence). Since fence->ops is no longer set to
NULL on signaling (after parent commit 4607587506fc ("dma-fence: Remove
RCU protection for fence->ops")), calling dma_fence_set_deadline() on a
signaled fence after the producer module has unloaded will dereference
ops->set_deadline in the unmapped module's .rodata section.

Similarly, the commit message notes that release() and wait() are
deprecated, but even when a modular driver leaves .release and .wait as
NULL in its struct dma_fence_ops table, dma_fence_release():

	if (ops->release)
		ops->release(fence);
	else
		dma_fence_free(fence);

and dma_fence_wait_timeout():

	if (ops->wait) {
		ret = ops->wait(fence, intr, timeout);
	} else {
		ret = dma_fence_default_wait(fence, intr, timeout);
	}

still dereference the ops table in the module's .rodata to check whether
those pointers are NULL when a consumer waits on or drops the last
reference to a signaled fence after the module has unloaded.

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 874848a4aec46..2223d75c5a496 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -549,16 +549,29 @@ static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
>  	const struct dma_fence_ops *ops = fence->ops;
> +	unsigned long flags;
> +	bool ret = false;
>  
> +	/* Fast-path optimization. */
>  	if (dma_fence_test_signaled_flag(fence))
>  		return true;
>  
> -	if (ops && ops->signaled && ops->signaled(fence)) {
> -		dma_fence_signal(fence);
> -		return true;
> +	dma_fence_lock_irqsave(fence, flags);
> +
> +	if (dma_fence_test_signaled_flag(fence)) {
> +		ret = true;
> +		goto out;
>  	}
>  
> -	return false;
> +	if (ops->signaled && ops->signaled(fence)) {
> +		ret = true;
> +		dma_fence_signal_locked(fence);
> +	}
> +
> +out:
> +	dma_fence_unlock_irqrestore(fence, flags);
> +
> +	return ret;
>  }

[Severity: High]
Can acquiring dma_fence_lock_irqsave() unconditionally in
dma_fence_is_signaled() cause a recursive spinlock deadlock in
amdkfd_fence_enable_signaling()?

In drivers/dma-buf/dma-fence.c, __dma_fence_enable_signaling() holds the
fence spinlock and calls ops->enable_signaling(fence) when the fence is
not yet signaled:

	dma_fence_assert_held(fence);

	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
				   &fence->flags);

	if (dma_fence_test_signaled_flag(fence))
		return false;

	if (!was_set && ops->enable_signaling) {
		trace_dma_fence_enable_signal(fence);

		if (!ops->enable_signaling(fence)) {

In drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c,
amdkfd_fence_enable_signaling() (where amdkfd_fence_ops.signaled is NULL)
calls dma_fence_is_signaled(f) and
kgd2kfd_schedule_evict_and_restore_process():

static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
{
	struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);

	if (!fence)
		return false;

	if (dma_fence_is_signaled(f))
		return true;

	if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))
		return true;

and kgd2kfd_schedule_evict_and_restore_process() in
drivers/gpu/drm/amd/amdkfd/kfd_device.c also checks:

	if (dma_fence_is_signaled(fence))
		return 0;

Previously, dma_fence_is_signaled(f) saw that ops->signaled was NULL and
returned false without taking the lock. With this change, when
DMA_FENCE_FLAG_SIGNALED_BIT is not set, dma_fence_is_signaled(f) attempts
to acquire f's spinlock while __dma_fence_enable_signaling(f) already
holds it on the same CPU.

!]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020100034.759183-2-phasta@kernel.org?part=2

Re: [RFC PATCH v2 1/3] dma-fence: Remove RCU protection for fence->ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dma-fence: broken KUnit test_signaling() after keeping fence->ops on signal
- [Low] dma-fence: sparse warnings from remaining rcu_access_pointer() on fence->ops
--

commit 4607587506fc5e0428c9d4ca148fc5a0c94b0003
Author: Philipp Stanner <phasta@kernel.org>

dma-fence: Remove RCU protection for fence->ops

This commit removes the RCU protection and __rcu annotation for
struct dma_fence::ops, and stops clearing fence->ops to NULL when
signaling fences without release or wait callbacks.

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index bd58688b81a72..5dab647e3f3f9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -353,7 +353,6 @@ void __dma_fence_might_wait(void)
>  void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  				      ktime_t timestamp)
>  {
> -	const struct dma_fence_ops *ops;
>  	struct dma_fence_cb *cur, *tmp;
>  	struct list_head cb_list;
>  
> @@ -365,15 +364,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	trace_dma_fence_signaled(fence);
>  
> -	/*
> -	 * When neither a release nor a wait operation is specified set the ops
> -	 * pointer to NULL to allow the fence structure to become independent
> -	 * from who originally issued it.
> -	 */
> -	ops = rcu_dereference_protected(fence->ops, true);
> -	if (!ops->release && !ops->wait)
> -		RCU_INIT_POINTER(fence->ops, NULL);
> -

[Severity: Medium]
Does keeping fence->ops set here break test_signaling() in
drivers/dma-buf/st-dma-fence.c?

In drivers/dma-buf/st-dma-fence.c, test_signaling() (added in commit
3695d754ffbb ("dma-buf/selftests: test RCU ops and inline lock v2"))
creates a mock fence using mock_ops (which defines neither .release nor
.wait), signals it via dma_fence_check_and_signal(), and then checks:

	if (rcu_dereference_protected(f->ops, true)) {
		KUNIT_FAIL(test, "Fence ops not cleared on signal");
		goto err_free;
	}

Since dma_fence_signal_timestamp_locked() no longer clears fence->ops to
NULL, won't this KUnit check unconditionally fail?  Additionally, passing
the non-__rcu f->ops pointer to rcu_dereference_protected() there will
trigger a Sparse address-space warning.

>  	/* Stash the cb_list before replacing it with the timestamp */
>  	list_replace(&fence->cb_list, &cb_list);
>  

[ ... ]

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index dd07d128adc3b..874848a4aec46 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -72,7 +72,7 @@ struct dma_fence {
>  		spinlock_t *extern_lock;
>  		spinlock_t inline_lock;
>  	};
> -	const struct dma_fence_ops __rcu *ops;
> +	const struct dma_fence_ops *ops;

[Severity: Low]
Should the remaining rcu_access_pointer(fence->ops) callers in the tree
also be updated now that the __rcu annotation is removed from
struct dma_fence::ops?

While dma_fence_is_array() and dma_fence_is_chain() are updated below,
several other functions converted in commits f4cc3ab824d6, 764f241ad227,
and 7519ba791b00 still wrap fence->ops in rcu_access_pointer(), which
causes Sparse to emit address-space mismatch warnings when building with
C=1:

drivers/gpu/drm/drm_crtc.c:fence_to_crtc():
	BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops);

drivers/gpu/drm/scheduler/sched_fence.c:to_drm_sched_fence():
	if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_scheduled)
		return container_of(f, struct drm_sched_fence, scheduled);

	if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_finished)
		return container_of(f, struct drm_sched_fence, finished);

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:to_amdgpu_amdkfd_fence():
	if (rcu_access_pointer(f->ops) == &amdkfd_fence_ops)
		return fence;

drivers/gpu/host1x/fence.c:host1x_fence_extract():
	if (rcu_access_pointer(fence->ops) != &host1x_syncpt_fence_ops)
		return -EINVAL;

>  	/*
>  	 * We clear the callback list on kref_put so that by the time we
>  	 * release the fence it is unused. No one should be adding to the

[ ... ]

> @@ -769,7 +761,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
>   */
>  static inline bool dma_fence_is_array(struct dma_fence *fence)
>  {
> -	return rcu_access_pointer(fence->ops) == &dma_fence_array_ops;
> +	return fence->ops == &dma_fence_array_ops;
>  }
>  
>  /**
> @@ -780,7 +772,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
>   */
>  static inline bool dma_fence_is_chain(struct dma_fence *fence)
>  {
> -	return rcu_access_pointer(fence->ops) == &dma_fence_chain_ops;
> +	return fence->ops == &dma_fence_chain_ops;
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020100034.759183-2-phasta@kernel.org?part=1

Re: [RFC PATCH v2 3/3] dma-fence: Support unload for ops->get_XXX_name()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] dma-fence: use-after-free on extern_lock in dma_fence_driver_name()
- [High] dma-fence: uninitialized locals in dma_fence_describe() when signaled
- [High] dma-fence: recursive spinlock deadlock via trace_dma_fence_destroy()
- [High] drm/i915: rq->lock ABBA deadlock in i915_request_show()
- [Low] dma-fence: stack VLAs in dma_fence and i915 fence name callers
- [Low] dma-fence: leftover rcu_read_lock() and outdated kernel-doc in name helpers
--

commit 9ca3a4e9e4ca7aa50b6bddada4c64fc3ed8e2861
Author: Philipp Stanner <phasta@kernel.org>

dma-fence: Support unload for ops->get_XXX_name()

This commit replaces the RCU-protected return values of
dma_fence_driver_name() and dma_fence_timeline_name() with callers
passing destination buffers that are populated under the fence lock.

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index e4350bce9cf9b..d473d4ea601a7 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -555,23 +555,22 @@ void dma_fence_release(struct kref *kref)
>  	struct dma_fence *fence =
>  		container_of(kref, struct dma_fence, refcount);
>  	const struct dma_fence_ops *ops = fence->ops;
> +	const unsigned short len = 64;
> +	char timeline[len];
> +	char driver[len];

[Severity: Low]
In C11/gnu11, a const-qualified variable is not an integer constant
expression, so does declaring timeline[len] and driver[len] create stack
VLAs that trigger compiler warnings (and build failures with CONFIG_WERROR
or CONFIG_DRM_I915_WERROR) under -Wvla-larger-than=1 in
scripts/Makefile.warn?

The same pattern appears in dma_fence_describe(), intel_gt_watchdog_work(),
i915_request_show(), and timer_i915_sw_fence_wake(). Also, in
dma_fence_release(), these buffers were moved out of the unlikely error
block to the top of the function.

>  
> -	rcu_read_lock();
>  	trace_dma_fence_destroy(fence);

[ ... ]

> @@ -1017,23 +1015,20 @@ 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 *)"";
> -	const char __rcu *driver = (const char __rcu *)"";
> -	const char *signaled = "";
> -
> -	rcu_read_lock();
> +	const unsigned short len = 64;
> +	char timeline[len];
> +	char driver[len];
> +	char *signaled;
>  
>  	if (!dma_fence_is_signaled(fence)) {
> -		timeline = dma_fence_timeline_name(fence);
> -		driver = dma_fence_driver_name(fence);
> +		dma_fence_timeline_name(fence, timeline, len);
> +		dma_fence_driver_name(fence, driver, len);
>  		signaled = "un";
>  	}
>  
>  	seq_printf(seq, "%llu:%llu %s %s %ssignalled\n",
>  		   fence->context, fence->seqno, timeline, driver,
>  		   signaled);
> -
> -	rcu_read_unlock();
>  }
>  EXPORT_SYMBOL(dma_fence_describe);

[Severity: High]
What happens in dma_fence_describe() when dma_fence_is_signaled(fence)
returns true?

Previously, timeline, driver, and signaled were initialized to "" at
declaration. Now they are only assigned inside the
if (!dma_fence_is_signaled(fence)) block. When the fence is already
signaled, won't seq_printf() dereference the uninitialized stack pointer
signaled and read the uninitialized stack arrays timeline and driver?

Also, if the fence signals concurrently right after
!dma_fence_is_signaled(fence) evaluates to true,
dma_fence_timeline_name() and dma_fence_driver_name() will populate
"signaled-timeline" and "detached-driver" while signaled is set to "un".

> @@ -1129,20 +1124,22 @@ EXPORT_SYMBOL(dma_fence_init64);
>   * 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)
> +void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short len)
>  {
> +	unsigned long flags;
> +	const char *tmp;
> +
> +	dma_fence_lock_irqsave(fence, flags);
> +
>  	if (!dma_fence_test_signaled_flag(fence))
> -		return (const char __rcu *)fence->ops->get_driver_name(fence);
> +		tmp = fence->ops->get_driver_name(fence);
>  	else
> -		return (const char __rcu *)"detached-driver";
> +		tmp = "detached-driver";
> +
> +	strscpy(s, tmp, len);
> +
> +	dma_fence_unlock_irqrestore(fence, flags);
>  }
>  EXPORT_SYMBOL(dma_fence_driver_name);
>  
> @@ -1158,19 +1155,21 @@ EXPORT_SYMBOL(dma_fence_driver_name);
>   * 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)
> +void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned short len)
>  {
> +	unsigned long flags;
> +	const char *tmp;
> +
> +	dma_fence_lock_irqsave(fence, flags);
> +
>  	if (!dma_fence_test_signaled_flag(fence))
> -		return (const char __rcu *)fence->ops->get_timeline_name(fence);
> +		tmp = fence->ops->get_timeline_name(fence);
>  	else
> -		return (const char __rcu *)"signaled-timeline";
> +		tmp = "signaled-timeline";
> +
> +	strscpy(s, tmp, len);
> +
> +	dma_fence_unlock_irqrestore(fence, flags);
>  }
>  EXPORT_SYMBOL(dma_fence_timeline_name);

[Severity: Critical]
Is it safe for dma_fence_driver_name() and dma_fence_timeline_name() to
unconditionally acquire dma_fence_lock_irqsave(fence, flags) before
checking if the fence is already signaled?

As noted in the kernel-doc comment above these functions, external memory
not embedded in struct dma_fence (including fence->extern_lock) must not
be accessed after the fence has signaled because drivers are allowed to
free the structure containing the lock once the fence is signaled:

  drivers/gpu/drm/panthor/panthor_sched.c:group_free_queue()
    dma_fence_put(queue->fence_ctx.last_fence);
    kfree(queue); /* frees queue->fence_ctx.lock */

  drivers/gpu/drm/msm/msm_fence.c:msm_fence_context_free()
    kfree(fctx); /* frees fctx->spinlock */

  drivers/gpu/drm/xe/xe_hw_fence.c:xe_hw_fence_irq_finish()
    synchronize_rcu(); /* before freeing irq containing irq->lock */

If a signaled fence is still held (for example in a sync_file) after the
driver frees the structure containing fence->extern_lock, and userspace
calls SYNC_IOC_FILE_INFO -> sync_fill_fence_info() ->
dma_fence_timeline_name(), won't dma_fence_lock_irqsave() dereference and
modify the freed spinlock?

[Severity: High]
Can acquiring dma_fence_lock_irqsave() inside dma_fence_driver_name() and
dma_fence_timeline_name() also cause a recursive spinlock deadlock when
trace_dma_fence_destroy is enabled?

In dma_fence_release(), trace_dma_fence_destroy(fence) is invoked when the
refcount drops to zero, which calls dma_fence_driver_name() and
dma_fence_timeline_name() in TP_fast_assign():

  dma_fence_put(fence)
    -> dma_fence_release()
      -> trace_dma_fence_destroy(fence)
        -> dma_fence_driver_name()
          -> dma_fence_lock_irqsave(fence, flags)

However, several paths in the kernel call dma_fence_put() while already
holding the fence's spinlock:

1. dma_fence_signal_timestamp_locked() holds dma_fence_spinlock(fence)
   while invoking callbacks such as dma_fence_chain_cb(), which calls
   dma_fence_put(f).

2. Drivers with shared external fence locks call dma_fence_put() while
   holding that lock, such as virtio_gpu_fence_event_process() (holding
   drv->lock), nouveau_fence_signal() (holding fctx->lock), and
   hw_fence_irq_run_cb() (holding irq->lock).

If the last reference is dropped on any of these paths while the
dma_fence_destroy tracepoint is active, won't dma_fence_lock_irqsave()
deadlock trying to acquire the already-held lock on the same CPU?

[Severity: Low]
Also, should the kernel-doc comments for dma_fence_driver_name() and
dma_fence_timeline_name() in drivers/dma-buf/dma-fence.c be updated to
document the new @s and @len parameters and void return, as well as the
get_driver_name and get_timeline_name comments in struct dma_fence_ops in
include/linux/dma-fence.h (which still state that the returned string is
RCU protected) and the leftover rcu_read_lock() wrappers around
trace_dma_fence_wait_start() and trace_dma_fence_wait_end() in
dma_fence_wait_timeout()?

> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2166bbdf7e4a4..0f08713b3b5e2 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c

[ ... ]

> @@ -268,19 +269,11 @@ 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)
>  {
> -	const char __rcu *timeline;
> -	const char __rcu *driver;
> +	dma_fence_timeline_name(fence, info->obj_name, sizeof(info->obj_name));
> +	dma_fence_driver_name(fence, info->driver_name, sizeof(info->driver_name));
>  
>  	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, rcu_dereference(driver),
> -		sizeof(info->driver_name));
> -
>  	info->status = dma_fence_get_status(fence);
>  	info->timestamp_ns =
>  		dma_fence_is_signaled(fence) ?

[Severity: Low]
In sync_fill_fence_info(), now that dma_fence_timeline_name() and
dma_fence_driver_name() have been moved outside the RCU critical section,
is the remaining rcu_read_lock() / rcu_read_unlock() pair around
dma_fence_get_status() and dma_fence_is_signaled() still needed?

> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index f66f8efc70629..ce9aff50c0885 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -2183,7 +2183,8 @@ void i915_request_show(struct drm_printer *m,
>  		       const char *prefix,
>  		       int indent)
>  {
> -	const char __rcu *timeline;
> +	const unsigned short len = 64;
> +	char timeline[len];
>  	char buf[80] = "";
>  	int x = 0;
>  
> @@ -2219,8 +2220,7 @@ 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);
> +	dma_fence_timeline_name((struct dma_fence *)&rq->fence, timeline, len);
>  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
>  		   prefix, indent, "                ",
>  		   queue_status(rq),
> @@ -2229,8 +2229,7 @@ void i915_request_show(struct drm_printer *m,
>  		   fence_status(rq),
>  		   buf,
>  		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
> -		   rcu_dereference(timeline));
> -	rcu_read_unlock();
> +		   timeline);
>  }
>  
>  static bool engine_match_ring(struct intel_engine_cs *engine, struct i915_request *rq)

[Severity: High]
Does calling dma_fence_timeline_name() in i915_request_show() introduce a
lock ordering inversion between rq->lock and sched_engine->lock (or
ce->guc_state.lock)?

Each i915_request initializes rq->fence with &rq->lock. During request
signaling in i915_request_retire() and signal_irq_work(), rq->lock is held
while invoking fence callbacks:

  i915_request_retire() / signal_irq_work() [holds rq->lock]
    -> dma_fence_signal_locked(&rq->fence)
      -> dma_i915_sw_fence_wake()
        -> submit_notify()
          -> execlists_submit_request() [acquires sched_engine->lock]
          -> guc_submit_request() [acquires ce->guc_state.lock]

However, i915_request_show() is called while holding sched_engine->lock or
ce->guc_state.lock in paths such as:

  intel_execlists_show_requests() / intel_execlists_dump_active_requests() [holds sched_engine->lock]
    -> i915_request_show()
      -> dma_fence_timeline_name() [acquires rq->lock]

  intel_guc_dump_active_requests() [holds ce->guc_state.lock]
    -> intel_engine_dump_active_requests()
      -> engine_dump_request()
        -> i915_request_show()
          -> dma_fence_timeline_name() [acquires rq->lock]

Can this cause an AB-BA deadlock when engine state is dumped concurrently
with request retirement or breadcrumb signaling?

[ ... ]

> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
> index 5b10a9e06fb4e..f3b929b98cf42 100644
> --- a/include/trace/events/dma_fence.h
> +++ b/include/trace/events/dma_fence.h
> @@ -9,6 +9,8 @@
>  
>  struct dma_fence;
>  
> +#define FENCE_NAME_LEN 64
> +
>  DECLARE_EVENT_CLASS(dma_fence,
>  
>  	TP_PROTO(struct dma_fence *fence),
> @@ -16,21 +18,21 @@ DECLARE_EVENT_CLASS(dma_fence,
>  	TP_ARGS(fence),
>  
>  	TP_STRUCT__entry(
> -		__string(driver, dma_fence_driver_name(fence))
> -		__string(timeline, dma_fence_timeline_name(fence))
> +		__array(char, driver, FENCE_NAME_LEN)
> +		__array(char, timeline, FENCE_NAME_LEN)
>  		__field(unsigned int, context)
>  		__field(unsigned int, seqno)
>  	),
>  
>  	TP_fast_assign(
> -		__assign_str(driver);
> -		__assign_str(timeline);
> +		dma_fence_driver_name(fence, __entry->driver, FENCE_NAME_LEN);
> +		dma_fence_timeline_name(fence, __entry->timeline, FENCE_NAME_LEN);
>  		__entry->context = fence->context;
>  		__entry->seqno = fence->seqno;
>  	),

[Severity: High]
As noted above in dma_fence_driver_name(), calling dma_fence_driver_name()
and dma_fence_timeline_name() inside TP_fast_assign() acquires the fence
spinlock during trace_dma_fence_destroy(fence), which can deadlock when
dma_fence_put() is called under the fence lock.

>  
>  	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
> -		  __get_str(driver), __get_str(timeline), __entry->context,
> +		  __entry->driver, __entry->timeline, __entry->context,
>  		  __entry->seqno)
>  );
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020100034.759183-2-phasta@kernel.org?part=3

Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Christian König]

On 10/20/26 12:00, Philipp Stanner wrote:
> In our Rust code bases, I'm currently experiencing what we also
> experience in C at many places: that the dma_fence contract's RCU
> requirements make the API more fragile and put an unnecessary burden on
> the API user. Most notably, in C the API behaving correctly now depends
> on the API *user* waiting before free()s, waiting before unloads – all
> because dma_fence is not properly synchronized.
> 
> The patches commit messages give more details about where I want to go
> with this particular RFC.
> 
> My rough "vision" for dma_fence, where I think it should develop to, can
> be described like this:
> 
> 1. All users should use inline-lock.
> 2. The fence-signaled state should be our decoupling point.
> 3. The fence state must be consistently locked.


> 4. This means all callbacks are only invoked for unsignaled fences, and
>    the fence cannot signal while a callback runs.

First of all that prevents concurrent execution of the callbacks which for a lot of use cases is a must have.

Then as I pointed out before the second problem is that this again makes the existing lock inversion between the signaling by polling and signaling by interrupts even worse than it already is.

What we could use is an SRCU, but a spinlock or any kind of lock is clearly the wrong approach.

So as far as I can see that is a big no-go and we should really just use RCU for everything and remove leaking the fence look into the callbacks like I outlined before.

Regards,
Christian.

> 5. All fence API functions should be "atomically correct": they can
>    always be called without any manual RCU guards in their proximity.
> 6. The fence lock should, in the long-term, become inaccessible for
>    users.
> 
> Point №6 means that this:
> 
> dma_fence_lock_irqsave(…);
> dma_fence_set_error(f, err);
> dma_fence_signal_locked(f);
> dma_fence_unlock_irqrestore(…);
> synchronize_rcu();
> kfree(driver_foo);
> 
> should simply become this:
> 
> dma_fence_signal(f, err);
> kfree(driver_foo);
> 
> So we would end up with a simpler, more robust and more maintainable
> API.
> 
> I gave this basic testing on a Nouveau desktop and it runs fine.
> dma-buf-tests also run fine. I can do more tests on Xe and amdgpu, maybe
> on Mali, too.
> 
> Since the series is mostly about locking the callbacks, all of which
> except for set_deadline() are *already* locked, I am confident that this
> can be done.
> 
> Feedback welcome
> 
> P.
> 
> Philipp Stanner (3):
>   dma-fence: Remove RCU protection for fence->ops
>   dma-fence: use lock to protect against module-unload
>   dma-fence: Support unload for ops->get_XXX_name()
> 
>  drivers/dma-buf/dma-fence.c                 | 144 +++++++-------------
>  drivers/dma-buf/sync_file.c                 |  29 ++--
>  drivers/gpu/drm/i915/gt/intel_gt_requests.c |  16 +--
>  drivers/gpu/drm/i915/i915_request.c         |   9 +-
>  drivers/gpu/drm/i915/i915_sw_fence.c        |  15 +-
>  include/linux/dma-fence.h                   |  63 ++++-----
>  include/trace/events/dma_fence.h            |  12 +-
>  7 files changed, 113 insertions(+), 175 deletions(-)
> 

Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Philipp Stanner]

On Tue, 2026-10-20 at 14:02 +0200, Christian König wrote:
> On 10/20/26 12:00, Philipp Stanner wrote:
> > In our Rust code bases, I'm currently experiencing what we also
> > experience in C at many places: that the dma_fence contract's RCU
> > requirements make the API more fragile and put an unnecessary burden on
> > the API user. Most notably, in C the API behaving correctly now depends
> > on the API *user* waiting before free()s, waiting before unloads – all
> > because dma_fence is not properly synchronized.
> > 
> > The patches commit messages give more details about where I want to go
> > with this particular RFC.
> > 
> > My rough "vision" for dma_fence, where I think it should develop to, can
> > be described like this:
> > 
> > 1. All users should use inline-lock.
> > 2. The fence-signaled state should be our decoupling point.
> > 3. The fence state must be consistently locked.
> 
> 
> > 4. This means all callbacks are only invoked for unsignaled fences, and
> >    the fence cannot signal while a callback runs.
> 
> First of all that prevents concurrent execution of the callbacks
> which for a lot of use cases is a must have.

Currently only set_deadline() can execute in parallel.

IIUC in our past discussions, your wish has been that the driver can
take its own locks with free locking-order in the callbacks.

Thus, the callback is invoked under lock in any case, just a few
instructions earlier or later. Thus, no callback ever truly executes
concurrently.

> 
> Then as I pointed out before the second problem is that this again
> makes the existing lock inversion between the signaling by polling
> and signaling by interrupts even worse than it already is.

We asked for examples in our last discussions, and the only one ever
presented was the list-access requiring the fctx-lock in
nouveau_fence_no_signaling(). Nouveau is often not a good example.

Can you provide examples in the kernel where deadlock would occur?

Note that I pointed out that all callbacks are already invoked with the
lock taken, which proofs that it's doable. Future users with inline-
lock could employ various techniques for dealing with the lock-order.

Only exception currently is set_deadline(). The only user, MSM,
acquires the lock manually and then nevertheless triggers work on a
kthread, examplifying just one technique.

> 
> What we could use is an SRCU, but a spinlock or any kind of lock is
> clearly the wrong approach.
> 
> So as far as I can see that is a big no-go and we should really just
> use RCU for everything and remove leaking the fence look into the
> callbacks like I outlined before.

Look at patch 3. We might want to add some legacy support for shared-
lock, but it is evident how much more simple the code gets.

Also:

> > 
> > Point №6 means that this:
> > 
> > dma_fence_lock_irqsave(…);

I even forgot that you also need a

if (!dma_fence_is_signaled_locked(…))

here

> > dma_fence_set_error(f, err);
> > dma_fence_signal_locked(f);
> > dma_fence_unlock_irqrestore(…);
> > synchronize_rcu();
> > kfree(driver_foo);
> > 
> > should simply become this:
> > 
> > dma_fence_signal(f, err);
> > kfree(driver_foo);


:)

Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Christian König]

On 10/20/26 16:41, Philipp Stanner wrote:
> On Tue, 2026-10-20 at 14:02 +0200, Christian König wrote:
>> On 10/20/26 12:00, Philipp Stanner wrote:
>>> In our Rust code bases, I'm currently experiencing what we also
>>> experience in C at many places: that the dma_fence contract's RCU
>>> requirements make the API more fragile and put an unnecessary burden on
>>> the API user. Most notably, in C the API behaving correctly now depends
>>> on the API *user* waiting before free()s, waiting before unloads – all
>>> because dma_fence is not properly synchronized.
>>>
>>> The patches commit messages give more details about where I want to go
>>> with this particular RFC.
>>>
>>> My rough "vision" for dma_fence, where I think it should develop to, can
>>> be described like this:
>>>
>>> 1. All users should use inline-lock.
>>> 2. The fence-signaled state should be our decoupling point.
>>> 3. The fence state must be consistently locked.
>>
>>
>>> 4. This means all callbacks are only invoked for unsignaled fences, and
>>>    the fence cannot signal while a callback runs.
>>
>> First of all that prevents concurrent execution of the callbacks
>> which for a lot of use cases is a must have.
> 
> Currently only set_deadline() can execute in parallel.

Especially the is_signaling() callback is executed in parallel by tons of use cases.

> 
> IIUC in our past discussions, your wish has been that the driver can
> take its own locks with free locking-order in the callbacks.
> 
> Thus, the callback is invoked under lock in any case, just a few
> instructions earlier or later. Thus, no callback ever truly executes
> concurrently.

And that is fine. Taking the look when you actually need it to protect data structures is perfectly correct.

But what you propose here is to abuse the look as a barrier, e.g. to find out if something has finished or not and that is a completely different story.

>>
>> Then as I pointed out before the second problem is that this again
>> makes the existing lock inversion between the signaling by polling
>> and signaling by interrupts even worse than it already is.
> 
> We asked for examples in our last discussions, and the only one ever
> presented was the list-access requiring the fctx-lock in
> nouveau_fence_no_signaling(). Nouveau is often not a good example.
> 
> Can you provide examples in the kernel where deadlock would occur?

Just exercise the simple example of a driver supporting both pull and interrupt:

Interrupt: Handler takes lock protecting a linked list, finds fence and signals it.

Polling: is_signaled is called with fence lock held, can't take list lock and so needs to work around that somehow.

This pattern repeats multiple times all over the different dma_fence implementation. Nouveu is just the tip of the iceberg, we have cases of this in amdgpu as well as i915 as well as XE.

> Note that I pointed out that all callbacks are already invoked with the
> lock taken, which proofs that it's doable.

Again, no it isn't. The shared lock was invented exactly because drivers ran into this problem.

The common pattern is the same, implementations just dance around the fact that the callbacks are called with the fence lock held by delegating cleanup work and/or using lockless containers etc...

A common component like dma_fence should never enforce users to do such workarounds just to solve it's own lifetime issues.

> Future users with inline-
> lock could employ various techniques for dealing with the lock-order.

And that's exactly what I'm trying to avoid.

> 
> Only exception currently is set_deadline(). The only user, MSM,
> acquires the lock manually and then nevertheless triggers work on a
> kthread, examplifying just one technique.
> 
>>
>> What we could use is an SRCU, but a spinlock or any kind of lock is
>> clearly the wrong approach.
>>
>> So as far as I can see that is a big no-go and we should really just
>> use RCU for everything and remove leaking the fence look into the
>> callbacks like I outlined before.
> 
> Look at patch 3. We might want to add some legacy support for shared-
> lock, but it is evident how much more simple the code gets.

To be honest I don't think that this is simpler at all.

Regards,
Christian.

> 
> Also:
> 
>>>
>>> Point №6 means that this:
>>>
>>> dma_fence_lock_irqsave(…);
> 
> I even forgot that you also need a
> 
> if (!dma_fence_is_signaled_locked(…))
> 
> here
> 
>>> dma_fence_set_error(f, err);
>>> dma_fence_signal_locked(f);
>>> dma_fence_unlock_irqrestore(…);
>>> synchronize_rcu();
>>> kfree(driver_foo);
>>>
>>> should simply become this:
>>>
>>> dma_fence_signal(f, err);
>>> kfree(driver_foo);
> 
> 
> :)

Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Philipp Stanner]

On Tue, 2026-10-20 at 17:07 +0200, Christian König wrote:
> On 10/20/26 16:41, Philipp Stanner wrote:
> > On Tue, 2026-10-20 at 14:02 +0200, Christian König wrote:
> > > On 10/20/26 12:00, Philipp Stanner wrote:
> > > > In our Rust code bases, I'm currently experiencing what we also
> > > > experience in C at many places: that the dma_fence contract's RCU
> > > > requirements make the API more fragile and put an unnecessary burden on
> > > > the API user. Most notably, in C the API behaving correctly now depends
> > > > on the API *user* waiting before free()s, waiting before unloads – all
> > > > because dma_fence is not properly synchronized.
> > > > 
> > > > The patches commit messages give more details about where I want to go
> > > > with this particular RFC.
> > > > 
> > > > My rough "vision" for dma_fence, where I think it should develop to, can
> > > > be described like this:
> > > > 
> > > > 1. All users should use inline-lock.
> > > > 2. The fence-signaled state should be our decoupling point.
> > > > 3. The fence state must be consistently locked.
> > > 
> > > 
> > > > 4. This means all callbacks are only invoked for unsignaled fences, and
> > > >    the fence cannot signal while a callback runs.
> > > 
> > > First of all that prevents concurrent execution of the callbacks
> > > which for a lot of use cases is a must have.
> > 
> > Currently only set_deadline() can execute in parallel.
> 
> Especially the is_signaling() callback is executed in parallel by tons of use cases.

ops->signaled() you mean.

Anyways, only one will succeed at having the fence marked as signaled
anyways, and then all other callers of dma_fence_is_signaled() will be
guarded by the fastpath bit-check (which btw is also broken; but we
want to address that in a different patch. For now my series proposes
keeping the fastpath).

> 
> > 
> > IIUC in our past discussions, your wish has been that the driver can
> > take its own locks with free locking-order in the callbacks.
> > 
> > Thus, the callback is invoked under lock in any case, just a few
> > instructions earlier or later. Thus, no callback ever truly executes
> > concurrently.
> 
> And that is fine. Taking the look when you actually need it to
> protect data structures is perfectly correct.
> 
> But what you propose here is to abuse the look as a barrier, e.g. to
> find out if something has finished or not and that is a completely
> different story.

Nope, I want to protect shared data, which is the signaled-state. Which
is currently a race-condition worked around with RCU (and sometimes
with manually doing the right thing (locking!)) [1], stemming from the
fact that the fence's spinlock is not used consistently.

> 
> > > 
> > > Then as I pointed out before the second problem is that this again
> > > makes the existing lock inversion between the signaling by polling
> > > and signaling by interrupts even worse than it already is.
> > 
> > We asked for examples in our last discussions, and the only one ever
> > presented was the list-access requiring the fctx-lock in
> > nouveau_fence_no_signaling(). Nouveau is often not a good example.
> > 
> > Can you provide examples in the kernel where deadlock would occur?
> 
> Just exercise the simple example of a driver supporting both pull and interrupt:
> 
> Interrupt: Handler takes lock protecting a linked list, finds fence
> and signals it.
> 
> Polling: is_signaled is called with fence lock held, can't take list
> lock and so needs to work around that somehow.

signaled() neither needs a list lock nor any other lock.

It needs to read a sequence number from somewhere, so a u64. And then
it returns true or false to the fence_backend.

The u64 can easily be an atomic. Also ioremaped reads should work
locklessly.

No need for any lock.

> 
> This pattern repeats multiple times all over the different dma_fence
> implementation. Nouveu is just the tip of the iceberg, we have cases
> of this in amdgpu as well as i915 as well as XE.

I guess my question then becomes what complicated locked tasks someone
has to do within its callback that locks are required, locks that
cannot be partitioned in a smart way to avoid that.

We have 3 callbacks that are relevant and not deprecated (the name-
functions are trivially solved by copying the data):

- is_signaled() ⇒ solved, see above
- enable_signaling()
- set_deadline()

The latter two might want to talk to their hardware, either enabling an
interrupt or telling it to clock the frequency up (see MSM). They might
need a driver-lock for that, fair enough.

Where does such a driver-lock need to be held while operating on a
fence, opening the deadlock-path?


Note that, as I pointed out many times, all callbacks are already
called under lock-protection. Especially enable_signaling(), and
is_signaled() must be lock-resistant, proven by
dma_fence_is_signaled_locked() existing.

Regardless, what I propose would only make a potential difference for
*new* users, with inline-lock, who can then cleanly design to avoid
locking problems.


> 
> > Note that I pointed out that all callbacks are already invoked with the
> > lock taken, which proofs that it's doable.
> 
> Again, no it isn't. The shared lock was invented exactly because
> drivers ran into this problem.
> 
> The common pattern is the same, implementations just dance around the
> fact that the callbacks are called with the fence lock held by
> delegating cleanup work and/or using lockless containers etc...

I suspect more that drivers attempt to do unnecessarily many things in
their callbacks which are not supposed to be done there.

> A common component like dma_fence should never enforce users to do
> such workarounds just to solve it's own lifetime issues.

Our common component forces all users to take our locks manually and
plaster everything with RCU. If they forget an RCU delay, outside our
API, things explode :(


P.


[1] https://elixir.bootlin.com/linux/v7.3/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L2787

>