Re: [PATCH] cxl/trace: Initialize cxl_poison region name to NULL

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Mar 14, 2024 at 06:53:47AM -0700, Ira Weiny wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > The TP_STRUCT__entry that gets assigned the region name, or an
> > empty string if no region is present, is erroneously initialized
> > to the cxl_region pointer. It needs to be initialized to NULL
> > otherwise its length is wrong and garbage chars can appear in
> > the kernel trace output: /sys/kernel/tracing/trace
> > 
> > Impact is that tooling depending on that trace data can miss
> > picking up a valid event when searching by region name. The
> > TP_printk() output, if enabled, does emit the correct region
> > names in the dmesg log.
> > 
> > This was found during testing of the cxl-list option to report
> > media-errors for a region.
> > 
> > Fixes: ddf49d57b841 ("cxl/trace: Add TRACE support for CXL media-error records")
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/trace.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index bdf117a33744..bc5ca4d530d1 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -657,7 +657,7 @@ TRACE_EVENT(cxl_poison,
> >  		__string(host, dev_name(cxlmd->dev.parent))
> >  		__field(u64, serial)
> >  		__field(u8, trace_type)
> > -		__string(region, region)
> > +		__string(region, NULL)
> 
> Couldn't this be "" instead of NULL then remove the __assign_str() if
> region is NULL?
> 
> Ira

Thanks for the review Ira.

That doesn't work because it inits with too short of a length and the
region names all get truncated.

Adding Steve to this thread. He has upcoming changes to this space, but
I also should note that I think this fix may want to be applied
backwards, ie to stable, so we cannot count on Steves upcoming changes.

> 
> >  		__field(u64, overflow_ts)
> >  		__field(u64, hpa)
> >  		__field(u64, dpa)
> > 
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> > -- 
> > 2.37.3
> > 
> 
> k