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

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 13/04/2026 12:32, Christian König wrote:
> On 4/13/26 12:05, Tvrtko Ursulin wrote:
>> Trace_dma_fence_signaled, trace_dma_fence_wait_end and
>> trace_dma_fence_destroy can all dereference a null fence->ops pointer
>> after it has been reset on fence signalling.
>>
>> Lets use the safe string getters for most tracepoints to a void this.
>>
>> But for the signalling tracepoint, we move it to before the fence->ops
>> is reset and special case its definition in order to avoid losing the
>> driver and timeline information.
>>
>> 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 | 29 +++++++++++++++++++++++++++--
>>   2 files changed, 29 insertions(+), 3 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..220bf71446e8 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))
> 
> 
> That requires that we hold the RCU read side lock while doing the trace.

True, and actually 3/3 needs to be squashed in 2/3 and then it is all 
fine. Then all tracepoints which use dma_fence_*_name() helpers are 
inside rcu_read_lock, and ones which do not use the dma_fence_ops class 
where they are guaranteed to be be safe.

> Not sure if that can be done inside the DECLARE_EVENT_CLASS() macro.

No idea.

>> +		__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),
>>   
>> @@ -67,7 +92,7 @@ DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>>   	TP_ARGS(fence)
>>   );
>>   
>> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>> +DEFINE_EVENT(dma_fence_ops, dma_fence_signaled,
> 
> The signal trace event is actually unproblematic. The question is more what to do with the release event.

Yes, unproblematic after fixed in this patch, or you mean something else?

trace_dma_fence_destroy is fine after this patch AFAICS and I did test it.

Regards,

Tvrtko

>>   	TP_PROTO(struct dma_fence *fence),
>>   
>