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

["Christian König" <christian.koenig@amd.com>]

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),
>