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

[Ira Weiny <ira.weiny@intel.com>]

Alison Schofield wrote:
> 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.

Ah ok.  With that added detail.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> 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.
>