Re: [PATCH] drm/xe: Drop trace_xe_hw_fence_free

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Jul 08, 2024 at 05:34:31PM -0400, Rodrigo Vivi wrote:
> On Mon, Jul 08, 2024 at 02:10:08PM -0700, Matthew Brost wrote:
> > fence->ctx may be stale memory when trace_xe_hw_fence_free is called
> > resuling UAF bug when deriving the device name. This tracepoint is not
> > all that useful, so just drop it.
> 
> the placement of it seems strange anyway, but if it is not useful,
> let's indeed drop it.
> 

Yes, the lifetime of fence can outlive anything it is attached to (in
this case the LRCs hw fence ctx) as the fence cleanup is quite lazy in
places (e.g. in syncobjs, dma-resvs, etc...) where it can be installed.

I think the only reason I added this in the first place was very early
in the driver coding I had ref counting problems where xe_hw_fences were
getting leaked and used ftrace to debug that issue. We have this correct
now so not all that useful. I'd rather have the device id in hw fence
tracepoints and I'd rather not special case this tracepoint to not try
to get the device id.

Matt 

> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > 
> > Fixes: 501c4255c409 ("drm/xe/trace: Print device_id in xe_trace events")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_hw_fence.c | 1 -
> >  drivers/gpu/drm/xe/xe_trace.h    | 5 -----
> >  2 files changed, 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> > index 35c0063a831a..45a9789cf501 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> > @@ -187,7 +187,6 @@ static void xe_hw_fence_release(struct dma_fence *dma_fence)
> >  {
> >  	struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence);
> >  
> > -	trace_xe_hw_fence_free(fence);
> >  	XE_WARN_ON(!list_empty(&fence->irq_link));
> >  	call_rcu(&dma_fence->rcu, fence_free);
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h
> > index 09ca1ad057b0..baba14fb1e32 100644
> > --- a/drivers/gpu/drm/xe/xe_trace.h
> > +++ b/drivers/gpu/drm/xe/xe_trace.h
> > @@ -341,11 +341,6 @@ DEFINE_EVENT(xe_hw_fence, xe_hw_fence_try_signal,
> >  	     TP_ARGS(fence)
> >  );
> >  
> > -DEFINE_EVENT(xe_hw_fence, xe_hw_fence_free,
> > -	     TP_PROTO(struct xe_hw_fence *fence),
> > -	     TP_ARGS(fence)
> > -);
> > -
> >  TRACE_EVENT(xe_reg_rw,
> >  	TP_PROTO(struct xe_gt *gt, bool write, u32 reg, u64 val, int len),
> >  
> > -- 
> > 2.34.1
> >