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

>