[PATCH 1/2] dma-fence: Silence sparse warning in dma_fence_describe

[PATCH 1/2] dma-fence: Silence sparse warning in dma_fence_describe

[Tvrtko Ursulin]

Sparse complains about assigning a string to a __rcu annotated local
variable:

drivers/dma-buf/dma-fence.c:1040:38: warning: incorrect type in initializer (different address spaces)
drivers/dma-buf/dma-fence.c:1040:38:    expected char const [noderef] __rcu *timeline
drivers/dma-buf/dma-fence.c:1040:38:    got char *
drivers/dma-buf/dma-fence.c:1041:36: warning: incorrect type in initializer (different address spaces)
drivers/dma-buf/dma-fence.c:1041:36:    expected char const [noderef] __rcu *driver
drivers/dma-buf/dma-fence.c:1041:36:    got char *

It is harmless but lets silence it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: ac364014fd81 ("dma-buf: cleanup dma_fence_describe v3")
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-fence.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 1826ba73094c..a2aa82f4eedd 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1037,8 +1037,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
  */
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
 {
-	const char __rcu *timeline = "";
-	const char __rcu *driver = "";
+	const char __rcu *timeline = (const char __rcu *)"";
+	const char __rcu *driver = (const char __rcu *)"";
 	const char *signaled = "";
 
 	rcu_read_lock();
-- 
2.52.0

[PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Tvrtko Ursulin]

Trace_dma_fence_signaled, trace_dma_fence_wait_end and
trace_dma_fence_destroy can all currently dereference a null fence->ops
pointer after it has been reset on fence signalling.

Lets use the safe string getters for most tracepoints to avoid this class
of a problem, while for the signal tracepoint we move it to before ops are
cleared to avoid losing the driver and timeline name information. Apart
from moving it we also need to add a new tracepoint class to bypass the
safe name getters since the signaled bit is already set.

For dma_fence_init we also need to use the new tracepoint class since the
rcu read lock is not held there, and we can do the same for the enable
signaling since there we are certain the fence cannot be signaled while
we are holding the lock and have even validated the fence->ops.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
Cc: Christian König <christian.koenig@amd.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-fence.c      |  3 ++-
 include/trace/events/dma_fence.h | 40 +++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index a2aa82f4eedd..b3bfa6943a8e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 				      &fence->flags)))
 		return;
 
+	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
@@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	fence->timestamp = timestamp;
 	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
-	trace_dma_fence_signaled(fence);
 
 	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
 		INIT_LIST_HEAD(&cur->node);
diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
index 3abba45c0601..5b10a9e06fb4 100644
--- a/include/trace/events/dma_fence.h
+++ b/include/trace/events/dma_fence.h
@@ -9,12 +9,40 @@
 
 struct dma_fence;
 
+DECLARE_EVENT_CLASS(dma_fence,
+
+	TP_PROTO(struct dma_fence *fence),
+
+	TP_ARGS(fence),
+
+	TP_STRUCT__entry(
+		__string(driver, dma_fence_driver_name(fence))
+		__string(timeline, dma_fence_timeline_name(fence))
+		__field(unsigned int, context)
+		__field(unsigned int, seqno)
+	),
+
+	TP_fast_assign(
+		__assign_str(driver);
+		__assign_str(timeline);
+		__entry->context = fence->context;
+		__entry->seqno = fence->seqno;
+	),
+
+	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
+		  __get_str(driver), __get_str(timeline), __entry->context,
+		  __entry->seqno)
+);
+
 /*
  * Safe only for call sites which are guaranteed to not race with fence
- * signaling,holding the fence->lock and having checked for not signaled, or the
- * signaling path itself.
+ * signaling, holding the fence->lock and having checked for not signaled, or
+ * the signaling path itself.
+ *
+ * TODO: Remove the need for this event class when drivers switch to independent
+ *       fences.
  */
-DECLARE_EVENT_CLASS(dma_fence,
+DECLARE_EVENT_CLASS(dma_fence_ops,
 
 	TP_PROTO(struct dma_fence *fence),
 
@@ -46,7 +74,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_init,
+DEFINE_EVENT(dma_fence_ops, dma_fence_init,
 
 	TP_PROTO(struct dma_fence *fence),
 
@@ -60,14 +88,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
+DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
 
 	TP_PROTO(struct dma_fence *fence),
 
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_signaled,
+DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
 
 	TP_PROTO(struct dma_fence *fence),
 
-- 
2.52.0

[PATCH 1/2] dma-fence: Silence sparse warning in dma_fence_describe

[Tvrtko Ursulin]

Sparse complains about assigning a string to a __rcu annotated local
variable:

drivers/dma-buf/dma-fence.c:1040:38: warning: incorrect type in initializer (different address spaces)
drivers/dma-buf/dma-fence.c:1040:38:    expected char const [noderef] __rcu *timeline
drivers/dma-buf/dma-fence.c:1040:38:    got char *
drivers/dma-buf/dma-fence.c:1041:36: warning: incorrect type in initializer (different address spaces)
drivers/dma-buf/dma-fence.c:1041:36:    expected char const [noderef] __rcu *driver
drivers/dma-buf/dma-fence.c:1041:36:    got char *

It is harmless but lets silence it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: ac364014fd81 ("dma-buf: cleanup dma_fence_describe v3")
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/dma-buf/dma-fence.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 1826ba73094c..a2aa82f4eedd 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1037,8 +1037,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
  */
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
 {
-	const char __rcu *timeline = "";
-	const char __rcu *driver = "";
+	const char __rcu *timeline = (const char __rcu *)"";
+	const char __rcu *driver = (const char __rcu *)"";
 	const char *signaled = "";
 
 	rcu_read_lock();
-- 
2.52.0

[PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Tvrtko Ursulin]

Trace_dma_fence_signaled, trace_dma_fence_wait_end and
trace_dma_fence_destroy can all currently dereference a null fence->ops
pointer after it has been reset on fence signalling.

Lets use the safe string getters for most tracepoints to avoid this class
of a problem, while for the signal tracepoint we move it to before ops are
cleared to avoid losing the driver and timeline name information. Apart
from moving it we also need to add a new tracepoint class to bypass the
safe name getters since the signaled bit is already set.

For dma_fence_init we also need to use the new tracepoint class since the
rcu read lock is not held there, and we can do the same for the enable
signaling since there we are certain the fence cannot be signaled while
we are holding the lock and have even validated the fence->ops.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
Cc: Christian König <christian.koenig@amd.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/dma-buf/dma-fence.c      |  3 ++-
 include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index a2aa82f4eedd..b3bfa6943a8e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 				      &fence->flags)))
 		return;
 
+	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
@@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	fence->timestamp = timestamp;
 	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
-	trace_dma_fence_signaled(fence);
 
 	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
 		INIT_LIST_HEAD(&cur->node);
diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
index 3abba45c0601..9e0cb9ce2388 100644
--- a/include/trace/events/dma_fence.h
+++ b/include/trace/events/dma_fence.h
@@ -9,12 +9,37 @@
 
 struct dma_fence;
 
+DECLARE_EVENT_CLASS(dma_fence,
+
+	TP_PROTO(struct dma_fence *fence),
+
+	TP_ARGS(fence),
+
+	TP_STRUCT__entry(
+		__string(driver, dma_fence_driver_name(fence))
+		__string(timeline, dma_fence_timeline_name(fence))
+		__field(unsigned int, context)
+		__field(unsigned int, seqno)
+	),
+
+	TP_fast_assign(
+		__assign_str(driver);
+		__assign_str(timeline);
+		__entry->context = fence->context;
+		__entry->seqno = fence->seqno;
+	),
+
+	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
+		  __get_str(driver), __get_str(timeline), __entry->context,
+		  __entry->seqno)
+);
+
 /*
  * Safe only for call sites which are guaranteed to not race with fence
  * signaling,holding the fence->lock and having checked for not signaled, or the
  * signaling path itself.
  */
-DECLARE_EVENT_CLASS(dma_fence,
+DECLARE_EVENT_CLASS(dma_fence_ops,
 
 	TP_PROTO(struct dma_fence *fence),
 
@@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_init,
+DEFINE_EVENT(dma_fence_ops, dma_fence_init,
 
 	TP_PROTO(struct dma_fence *fence),
 
@@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
+DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
 
 	TP_PROTO(struct dma_fence *fence),
 
 	TP_ARGS(fence)
 );
 
-DEFINE_EVENT(dma_fence, dma_fence_signaled,
+DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
 
 	TP_PROTO(struct dma_fence *fence),
 
-- 
2.52.0

Re: [PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Christian König]

On 4/14/26 17:49, Tvrtko Ursulin wrote:
> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
> trace_dma_fence_destroy can all currently dereference a null fence->ops
> pointer after it has been reset on fence signalling.
> 
> Lets use the safe string getters for most tracepoints to avoid this class
> of a problem, while for the signal tracepoint we move it to before ops are
> cleared to avoid losing the driver and timeline name information. Apart
> from moving it we also need to add a new tracepoint class to bypass the
> safe name getters since the signaled bit is already set.
> 
> For dma_fence_init we also need to use the new tracepoint class since the
> rcu read lock is not held there, and we can do the same for the enable
> signaling since there we are certain the fence cannot be signaled while
> we are holding the lock and have even validated the fence->ops.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> ---
>  drivers/dma-buf/dma-fence.c      |  3 ++-
>  include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index a2aa82f4eedd..b3bfa6943a8e 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  				      &fence->flags)))
>  		return;
>  
> +	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
> @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	fence->timestamp = timestamp;
>  	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
> -	trace_dma_fence_signaled(fence);

I think this part here should be a separate patch.

>  
>  	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>  		INIT_LIST_HEAD(&cur->node);
> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
> index 3abba45c0601..9e0cb9ce2388 100644
> --- a/include/trace/events/dma_fence.h
> +++ b/include/trace/events/dma_fence.h
> @@ -9,12 +9,37 @@
>  
>  struct dma_fence;
>  
> +DECLARE_EVENT_CLASS(dma_fence,
> +
> +	TP_PROTO(struct dma_fence *fence),
> +
> +	TP_ARGS(fence),
> +
> +	TP_STRUCT__entry(
> +		__string(driver, dma_fence_driver_name(fence))
> +		__string(timeline, dma_fence_timeline_name(fence))
> +		__field(unsigned int, context)
> +		__field(unsigned int, seqno)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(driver);
> +		__assign_str(timeline);
> +		__entry->context = fence->context;
> +		__entry->seqno = fence->seqno;
> +	),
> +
> +	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
> +		  __get_str(driver), __get_str(timeline), __entry->context,
> +		  __entry->seqno)
> +);
> +

Mhm, I'm strongly in favor to just use this approach for all trace points.

The minimal extra overhead shouldn't really matter at all.

Regards,
Christian. 

>  /*
>   * Safe only for call sites which are guaranteed to not race with fence
>   * signaling,holding the fence->lock and having checked for not signaled, or the
>   * signaling path itself.
>   */
> -DECLARE_EVENT_CLASS(dma_fence,
> +DECLARE_EVENT_CLASS(dma_fence_ops,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
> @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_init,
> +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
> @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
> +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  
>  	TP_ARGS(fence)
>  );
>  
> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
>  
>  	TP_PROTO(struct dma_fence *fence),
>  

Re: [PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Tvrtko Ursulin]

On 14/04/2026 19:30, Christian König wrote:
> On 4/14/26 17:49, Tvrtko Ursulin wrote:
>> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
>> trace_dma_fence_destroy can all currently dereference a null fence->ops
>> pointer after it has been reset on fence signalling.
>>
>> Lets use the safe string getters for most tracepoints to avoid this class
>> of a problem, while for the signal tracepoint we move it to before ops are
>> cleared to avoid losing the driver and timeline name information. Apart
>> from moving it we also need to add a new tracepoint class to bypass the
>> safe name getters since the signaled bit is already set.
>>
>> For dma_fence_init we also need to use the new tracepoint class since the
>> rcu read lock is not held there, and we can do the same for the enable
>> signaling since there we are certain the fence cannot be signaled while
>> we are holding the lock and have even validated the fence->ops.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> ---
>>   drivers/dma-buf/dma-fence.c      |  3 ++-
>>   include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
>>   2 files changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index a2aa82f4eedd..b3bfa6943a8e 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>   				      &fence->flags)))
>>   		return;
>>   
>> +	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
>> @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>   
>>   	fence->timestamp = timestamp;
>>   	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>> -	trace_dma_fence_signaled(fence);
> 
> I think this part here should be a separate patch.

I had that in 
https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igalia.com/ 
but the discussion fizzled out before an rb.

> 
>>   
>>   	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>>   		INIT_LIST_HEAD(&cur->node);
>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>> index 3abba45c0601..9e0cb9ce2388 100644
>> --- a/include/trace/events/dma_fence.h
>> +++ b/include/trace/events/dma_fence.h
>> @@ -9,12 +9,37 @@
>>   
>>   struct dma_fence;
>>   
>> +DECLARE_EVENT_CLASS(dma_fence,
>> +
>> +	TP_PROTO(struct dma_fence *fence),
>> +
>> +	TP_ARGS(fence),
>> +
>> +	TP_STRUCT__entry(
>> +		__string(driver, dma_fence_driver_name(fence))
>> +		__string(timeline, dma_fence_timeline_name(fence))
>> +		__field(unsigned int, context)
>> +		__field(unsigned int, seqno)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__assign_str(driver);
>> +		__assign_str(timeline);
>> +		__entry->context = fence->context;
>> +		__entry->seqno = fence->seqno;
>> +	),
>> +
>> +	TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>> +		  __get_str(driver), __get_str(timeline), __entry->context,
>> +		  __entry->seqno)
>> +);
>> +
> 
> Mhm, I'm strongly in favor to just use this approach for all trace points.
> 
> The minimal extra overhead shouldn't really matter at all.

Yeah, I am a bit on the fence. It would required a bit of an ugly 
rcu_read_lock around trace_dma_fence_signal_init and 
trace_dma_fence_signaled would lose the driver/timeline info _unless_ 
name helpers would also be changed to look at fence->ops instead of "is 
signaled". Those have no memory barriers so not sure I want to think 
about racyness and how to solve it.

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
>>   /*
>>    * Safe only for call sites which are guaranteed to not race with fence
>>    * signaling,holding the fence->lock and having checked for not signaled, or the
>>    * signaling path itself.
>>    */
>> -DECLARE_EVENT_CLASS(dma_fence,
>> +DECLARE_EVENT_CLASS(dma_fence_ops,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>> @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_init,
>> +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>> @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>> +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
>>   
>>   	TP_PROTO(struct dma_fence *fence),
>>   
> 

Re: [PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Christian König]

On 4/15/26 09:58, Tvrtko Ursulin wrote:
> 
> On 14/04/2026 19:30, Christian König wrote:
>> On 4/14/26 17:49, Tvrtko Ursulin wrote:
>>> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
>>> trace_dma_fence_destroy can all currently dereference a null fence->ops
>>> pointer after it has been reset on fence signalling.
>>>
>>> Lets use the safe string getters for most tracepoints to avoid this class
>>> of a problem, while for the signal tracepoint we move it to before ops are
>>> cleared to avoid losing the driver and timeline name information. Apart
>>> from moving it we also need to add a new tracepoint class to bypass the
>>> safe name getters since the signaled bit is already set.
>>>
>>> For dma_fence_init we also need to use the new tracepoint class since the
>>> rcu read lock is not held there, and we can do the same for the enable
>>> signaling since there we are certain the fence cannot be signaled while
>>> we are holding the lock and have even validated the fence->ops.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Philipp Stanner <phasta@kernel.org>
>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>> Cc: linux-media@vger.kernel.org
>>> Cc: linaro-mm-sig@lists.linaro.org
>>> ---
>>>   drivers/dma-buf/dma-fence.c      |  3 ++-
>>>   include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
>>>   2 files changed, 31 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index a2aa82f4eedd..b3bfa6943a8e 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>                         &fence->flags)))
>>>           return;
>>>   +    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
>>> @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>         fence->timestamp = timestamp;
>>>       set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>>> -    trace_dma_fence_signaled(fence);
>>
>> I think this part here should be a separate patch.
> 
> I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igalia.com/ but the discussion fizzled out before an rb.
> 
>>
>>>         list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>>>           INIT_LIST_HEAD(&cur->node);
>>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>>> index 3abba45c0601..9e0cb9ce2388 100644
>>> --- a/include/trace/events/dma_fence.h
>>> +++ b/include/trace/events/dma_fence.h
>>> @@ -9,12 +9,37 @@
>>>     struct dma_fence;
>>>   +DECLARE_EVENT_CLASS(dma_fence,
>>> +
>>> +    TP_PROTO(struct dma_fence *fence),
>>> +
>>> +    TP_ARGS(fence),
>>> +
>>> +    TP_STRUCT__entry(
>>> +        __string(driver, dma_fence_driver_name(fence))
>>> +        __string(timeline, dma_fence_timeline_name(fence))
>>> +        __field(unsigned int, context)
>>> +        __field(unsigned int, seqno)
>>> +    ),
>>> +
>>> +    TP_fast_assign(
>>> +        __assign_str(driver);
>>> +        __assign_str(timeline);
>>> +        __entry->context = fence->context;
>>> +        __entry->seqno = fence->seqno;
>>> +    ),
>>> +
>>> +    TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>>> +          __get_str(driver), __get_str(timeline), __entry->context,
>>> +          __entry->seqno)
>>> +);
>>> +
>>
>> Mhm, I'm strongly in favor to just use this approach for all trace points.
>>
>> The minimal extra overhead shouldn't really matter at all.
> 
> Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init

I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.

I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.

> and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.

Mhm, that is a bit more problematic.

ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.

Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.

Thanks,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>>
>> Regards,
>> Christian.
>>
>>>   /*
>>>    * Safe only for call sites which are guaranteed to not race with fence
>>>    * signaling,holding the fence->lock and having checked for not signaled, or the
>>>    * signaling path itself.
>>>    */
>>> -DECLARE_EVENT_CLASS(dma_fence,
>>> +DECLARE_EVENT_CLASS(dma_fence_ops,
>>>         TP_PROTO(struct dma_fence *fence),
>>>   @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
>>>       TP_ARGS(fence)
>>>   );
>>>   -DEFINE_EVENT(dma_fence, dma_fence_init,
>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
>>>         TP_PROTO(struct dma_fence *fence),
>>>   @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>>>       TP_ARGS(fence)
>>>   );
>>>   -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
>>>         TP_PROTO(struct dma_fence *fence),
>>>         TP_ARGS(fence)
>>>   );
>>>   -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
>>>         TP_PROTO(struct dma_fence *fence),
>>>   
>>
> 

Re: [PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Tvrtko Ursulin]

On 15/04/2026 09:13, Christian König wrote:
> On 4/15/26 09:58, Tvrtko Ursulin wrote:
>>
>> On 14/04/2026 19:30, Christian König wrote:
>>> On 4/14/26 17:49, Tvrtko Ursulin wrote:
>>>> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
>>>> trace_dma_fence_destroy can all currently dereference a null fence->ops
>>>> pointer after it has been reset on fence signalling.
>>>>
>>>> Lets use the safe string getters for most tracepoints to avoid this class
>>>> of a problem, while for the signal tracepoint we move it to before ops are
>>>> cleared to avoid losing the driver and timeline name information. Apart
>>>> from moving it we also need to add a new tracepoint class to bypass the
>>>> safe name getters since the signaled bit is already set.
>>>>
>>>> For dma_fence_init we also need to use the new tracepoint class since the
>>>> rcu read lock is not held there, and we can do the same for the enable
>>>> signaling since there we are certain the fence cannot be signaled while
>>>> we are holding the lock and have even validated the fence->ops.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Philipp Stanner <phasta@kernel.org>
>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>> Cc: linux-media@vger.kernel.org
>>>> Cc: linaro-mm-sig@lists.linaro.org
>>>> ---
>>>>    drivers/dma-buf/dma-fence.c      |  3 ++-
>>>>    include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
>>>>    2 files changed, 31 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>>> index a2aa82f4eedd..b3bfa6943a8e 100644
>>>> --- a/drivers/dma-buf/dma-fence.c
>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>> @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>                          &fence->flags)))
>>>>            return;
>>>>    +    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
>>>> @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>          fence->timestamp = timestamp;
>>>>        set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>>>> -    trace_dma_fence_signaled(fence);
>>>
>>> I think this part here should be a separate patch.
>>
>> I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igalia.com/ but the discussion fizzled out before an rb.
>>
>>>
>>>>          list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>>>>            INIT_LIST_HEAD(&cur->node);
>>>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>>>> index 3abba45c0601..9e0cb9ce2388 100644
>>>> --- a/include/trace/events/dma_fence.h
>>>> +++ b/include/trace/events/dma_fence.h
>>>> @@ -9,12 +9,37 @@
>>>>      struct dma_fence;
>>>>    +DECLARE_EVENT_CLASS(dma_fence,
>>>> +
>>>> +    TP_PROTO(struct dma_fence *fence),
>>>> +
>>>> +    TP_ARGS(fence),
>>>> +
>>>> +    TP_STRUCT__entry(
>>>> +        __string(driver, dma_fence_driver_name(fence))
>>>> +        __string(timeline, dma_fence_timeline_name(fence))
>>>> +        __field(unsigned int, context)
>>>> +        __field(unsigned int, seqno)
>>>> +    ),
>>>> +
>>>> +    TP_fast_assign(
>>>> +        __assign_str(driver);
>>>> +        __assign_str(timeline);
>>>> +        __entry->context = fence->context;
>>>> +        __entry->seqno = fence->seqno;
>>>> +    ),
>>>> +
>>>> +    TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>>>> +          __get_str(driver), __get_str(timeline), __entry->context,
>>>> +          __entry->seqno)
>>>> +);
>>>> +
>>>
>>> Mhm, I'm strongly in favor to just use this approach for all trace points.
>>>
>>> The minimal extra overhead shouldn't really matter at all.
>>
>> Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init
> 
> I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.
> 
> I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.
> 
>> and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
> 
> Mhm, that is a bit more problematic.
> 
> ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.

Hm yes, that too.


> Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.

Thank you, I've sent an updated version with a comment to this effect 
placed to the event class definition. I put your r-b so please double 
check if you are happy with that version.

Regards,

Tvrtko
>>>>    /*
>>>>     * Safe only for call sites which are guaranteed to not race with fence
>>>>     * signaling,holding the fence->lock and having checked for not signaled, or the
>>>>     * signaling path itself.
>>>>     */
>>>> -DECLARE_EVENT_CLASS(dma_fence,
>>>> +DECLARE_EVENT_CLASS(dma_fence_ops,
>>>>          TP_PROTO(struct dma_fence *fence),
>>>>    @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
>>>>        TP_ARGS(fence)
>>>>    );
>>>>    -DEFINE_EVENT(dma_fence, dma_fence_init,
>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
>>>>          TP_PROTO(struct dma_fence *fence),
>>>>    @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>>>>        TP_ARGS(fence)
>>>>    );
>>>>    -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
>>>>          TP_PROTO(struct dma_fence *fence),
>>>>          TP_ARGS(fence)
>>>>    );
>>>>    -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
>>>>          TP_PROTO(struct dma_fence *fence),
>>>>    
>>>
>>
> 

Re: [PATCH 2/2] dma-fence: Fix potential tracepoint null pointer dereferences

[Christian König]

On 4/15/26 10:33, Tvrtko Ursulin wrote:
> 
> On 15/04/2026 09:13, Christian König wrote:
>> On 4/15/26 09:58, Tvrtko Ursulin wrote:
>>>
>>> On 14/04/2026 19:30, Christian König wrote:
>>>> On 4/14/26 17:49, Tvrtko Ursulin wrote:
>>>>> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
>>>>> trace_dma_fence_destroy can all currently dereference a null fence->ops
>>>>> pointer after it has been reset on fence signalling.
>>>>>
>>>>> Lets use the safe string getters for most tracepoints to avoid this class
>>>>> of a problem, while for the signal tracepoint we move it to before ops are
>>>>> cleared to avoid losing the driver and timeline name information. Apart
>>>>> from moving it we also need to add a new tracepoint class to bypass the
>>>>> safe name getters since the signaled bit is already set.
>>>>>
>>>>> For dma_fence_init we also need to use the new tracepoint class since the
>>>>> rcu read lock is not held there, and we can do the same for the enable
>>>>> signaling since there we are certain the fence cannot be signaled while
>>>>> we are holding the lock and have even validated the fence->ops.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>> Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3")
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>> Cc: Philipp Stanner <phasta@kernel.org>
>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>>> Cc: linux-media@vger.kernel.org
>>>>> Cc: linaro-mm-sig@lists.linaro.org
>>>>> ---
>>>>>    drivers/dma-buf/dma-fence.c      |  3 ++-
>>>>>    include/trace/events/dma_fence.h | 33 ++++++++++++++++++++++++++++----
>>>>>    2 files changed, 31 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>>>> index a2aa82f4eedd..b3bfa6943a8e 100644
>>>>> --- a/drivers/dma-buf/dma-fence.c
>>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>>> @@ -363,6 +363,8 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>>                          &fence->flags)))
>>>>>            return;
>>>>>    +    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
>>>>> @@ -377,7 +379,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>>          fence->timestamp = timestamp;
>>>>>        set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>>>>> -    trace_dma_fence_signaled(fence);
>>>>
>>>> I think this part here should be a separate patch.
>>>
>>> I had that in https://lore.kernel.org/dri-devel/20260330133623.17704-1-tvrtko.ursulin@igalia.com/ but the discussion fizzled out before an rb.
>>>
>>>>
>>>>>          list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>>>>>            INIT_LIST_HEAD(&cur->node);
>>>>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>>>>> index 3abba45c0601..9e0cb9ce2388 100644
>>>>> --- a/include/trace/events/dma_fence.h
>>>>> +++ b/include/trace/events/dma_fence.h
>>>>> @@ -9,12 +9,37 @@
>>>>>      struct dma_fence;
>>>>>    +DECLARE_EVENT_CLASS(dma_fence,
>>>>> +
>>>>> +    TP_PROTO(struct dma_fence *fence),
>>>>> +
>>>>> +    TP_ARGS(fence),
>>>>> +
>>>>> +    TP_STRUCT__entry(
>>>>> +        __string(driver, dma_fence_driver_name(fence))
>>>>> +        __string(timeline, dma_fence_timeline_name(fence))
>>>>> +        __field(unsigned int, context)
>>>>> +        __field(unsigned int, seqno)
>>>>> +    ),
>>>>> +
>>>>> +    TP_fast_assign(
>>>>> +        __assign_str(driver);
>>>>> +        __assign_str(timeline);
>>>>> +        __entry->context = fence->context;
>>>>> +        __entry->seqno = fence->seqno;
>>>>> +    ),
>>>>> +
>>>>> +    TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>>>>> +          __get_str(driver), __get_str(timeline), __entry->context,
>>>>> +          __entry->seqno)
>>>>> +);
>>>>> +
>>>>
>>>> Mhm, I'm strongly in favor to just use this approach for all trace points.
>>>>
>>>> The minimal extra overhead shouldn't really matter at all.
>>>
>>> Yeah, I am a bit on the fence. It would required a bit of an ugly rcu_read_lock around trace_dma_fence_signal_init
>>
>> I think as long as we only grab the RCU read side lock when the tracepoint is actually enabled then that shouldn't matter.
>>
>> I do remember patches flying by which optimized this use case for the whole trace subsystem but didn't took a closer look how to do that now.
>>
>>> and trace_dma_fence_signaled would lose the driver/timeline info _unless_ name helpers would also be changed to look at fence->ops instead of "is signaled". Those have no memory barriers so not sure I want to think about racyness and how to solve it.
>>
>> Mhm, that is a bit more problematic.
>>
>> ops is only set to NULL when neither free nor wait is specified, so checking is signaled is still the right thing to do for drivers which uses those callbacks but still want to have the RCU protection of the returned strings.
> 
> Hm yes, that too.
> 
> 
>> Ok, feel free to go ahead with this approach for now but please add a /* TODO: clean that up when most drivers switched to independent fences */.
> 
> Thank you, I've sent an updated version with a comment to this effect placed to the event class definition. I put your r-b so please double check if you are happy with that version.

Yeah works for me, feel free to push to drm-misc-next.

Thanks,
Christian.

> 
> Regards,
> 
> Tvrtko
>>>>>    /*
>>>>>     * Safe only for call sites which are guaranteed to not race with fence
>>>>>     * signaling,holding the fence->lock and having checked for not signaled, or the
>>>>>     * signaling path itself.
>>>>>     */
>>>>> -DECLARE_EVENT_CLASS(dma_fence,
>>>>> +DECLARE_EVENT_CLASS(dma_fence_ops,
>>>>>          TP_PROTO(struct dma_fence *fence),
>>>>>    @@ -46,7 +71,7 @@ DEFINE_EVENT(dma_fence, dma_fence_emit,
>>>>>        TP_ARGS(fence)
>>>>>    );
>>>>>    -DEFINE_EVENT(dma_fence, dma_fence_init,
>>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_init,
>>>>>          TP_PROTO(struct dma_fence *fence),
>>>>>    @@ -60,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>>>>>        TP_ARGS(fence)
>>>>>    );
>>>>>    -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_enable_signal,
>>>>>          TP_PROTO(struct dma_fence *fence),
>>>>>          TP_ARGS(fence)
>>>>>    );
>>>>>    -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>>>>> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
>>>>>          TP_PROTO(struct dma_fence *fence),
>>>>>    
>>>>
>>>
>>
>