Re: [PATCH v2] cxl/trace: Properly initialize cxl_poison region name

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

On Thu, Mar 14, 2024 at 04:41:36PM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2024 13:12:17 -0700
> alison.schofield@intel.com 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 properly initialized
> > otherwise it's length is wrong and garbage chars can appear in
> > the kernel trace output: /sys/kernel/tracing/trace
> > 
> > The bad initialization was due in part to a naming conflict with
> > the parameter: struct cxl_region *region. The field 'region' is
> > already exposed externally as the region name, so changing that
> > to something logical, like 'region_name' is not an option. Instead
> > rename the internal only struct cxl_region to the commonly used
> > 'cxlr'.
> > 
> > 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")
> 
> Probably should add Cc: stable, as passing in region as a string was a real
> bug.
> 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> > 
> > Changes in v2:
> > - Initialize the region name with an assignment (Steve)
> > - Rename struct cxl_region 'cxlr' instead of overusing 'region' identifier
> > - Update commit message & log
> > 
> > 
> >  drivers/cxl/core/trace.h | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index bdf117a33744..e5f13260fc52 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -646,18 +646,18 @@ u64 cxl_trace_hpa(struct cxl_region *cxlr, struct cxl_memdev *memdev, u64 dpa);
> >  
> >  TRACE_EVENT(cxl_poison,
> >  
> > -	TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *region,
> > +	TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *cxlr,
> >  		 const struct cxl_poison_record *record, u8 flags,
> >  		 __le64 overflow_ts, enum cxl_poison_trace_type trace_type),
> >  
> > -	TP_ARGS(cxlmd, region, record, flags, overflow_ts, trace_type),
> > +	TP_ARGS(cxlmd, cxlr, record, flags, overflow_ts, trace_type),
> >  
> >  	TP_STRUCT__entry(
> >  		__string(memdev, dev_name(&cxlmd->dev))
> >  		__string(host, dev_name(cxlmd->dev.parent))
> >  		__field(u64, serial)
> >  		__field(u8, trace_type)
> > -		__string(region, region)
> > +		__string(region, cxlr ? dev_name(&cxlr->dev) : "")
> 
> I'm still curious to why NULL didn't work. I guess it may never have as I
> noticed there's nothing else doing that. There are cases that a variable
> returns NULL and the __string() handles it. But I guess the compiler gets
> confused if the NULL is a possible return to the condition in __string().

Here's the full warning spew:

In file included from ./include/trace/define_trace.h:102,
                 from drivers/cxl/core/trace.h:713,
                 from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null where non-null expected [-Wnonnull]
   50 |                     strlen((src) ? (const char *)(src) : "(null)") + 1)
      |                     ^~~~~~
./include/trace/trace_events.h:263:9: note: in definition of macro ‘DECLARE_EVENT_CLASS’
  263 |         tstruct;                                                        \
      |         ^~~~~~~
./include/trace/trace_events.h:43:30: note: in expansion of macro ‘PARAMS’
   43 |                              PARAMS(tstruct),                  \
      |                              ^~~~~~
drivers/cxl/core/./trace.h:647:1: note: in expansion of macro ‘TRACE_EVENT’
  647 | TRACE_EVENT(cxl_poison,
      | ^~~~~~~~~~~
drivers/cxl/core/./trace.h:655:9: note: in expansion of macro ‘TP_STRUCT__entry’
  655 |         TP_STRUCT__entry(
      |         ^~~~~~~~~~~~~~~~
./include/trace/stages/stage5_get_offsets.h:49:29: note: in expansion of macro ‘__dynamic_array’
   49 | #define __string(item, src) __dynamic_array(char, item,                 \
      |                             ^~~~~~~~~~~~~~~
drivers/cxl/core/./trace.h:660:17: note: in expansion of macro ‘__string’
  660 |                 __string(region, cxlr ? dev_name(&cxlr->dev) : NULL)
      |                 ^~~~~~~~
In file included from ./include/linux/uuid.h:11,
                 from ./include/linux/libnvdimm.h:12,
                 from ./drivers/cxl/cxl.h:7,
                 from drivers/cxl/core/trace.c:4:
./include/linux/string.h:126:24: note: in a call to function ‘strlen’ declared ‘nonnull’
  126 | extern __kernel_size_t strlen(const char *);
      |                        ^~~~~~


> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> 
> -- Steve
> 
> 
> 
> >  		__field(u64, overflow_ts)
> >  		__field(u64, hpa)
> >  		__field(u64, dpa)
> > @@ -677,10 +677,10 @@ TRACE_EVENT(cxl_poison,
> >  		__entry->source = cxl_poison_record_source(record);
> >  		__entry->trace_type = trace_type;
> >  		__entry->flags = flags;
> > -		if (region) {
> > -			__assign_str(region, dev_name(&region->dev));
> > -			memcpy(__entry->uuid, &region->params.uuid, 16);
> > -			__entry->hpa = cxl_trace_hpa(region, cxlmd,
> > +		if (cxlr) {
> > +			__assign_str(region, dev_name(&cxlr->dev));
> > +			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> > +			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> >  						     __entry->dpa);
> >  		} else {
> >  			__assign_str(region, "");
> > 
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> 
>