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

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

[alison.schofield]

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")
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) : "")
 		__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
-- 
2.37.3

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

[Steven Rostedt]

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

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

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

[Alison Schofield]

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

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

[Steven Rostedt]

On Thu, 14 Mar 2024 14:05:00 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> > 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: n

The full warning didn't add any new information. The above was all I
needed. But I'm curious if you applied this patch if the warning will go
away. (Note I didn't test nor compile this).

-- Steve

diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
index e30a13be46ba..dfae18d8f4df 100644
--- a/include/trace/stages/stage5_get_offsets.h
+++ b/include/trace/stages/stage5_get_offsets.h
@@ -9,6 +9,13 @@
 #undef __entry
 #define __entry entry
 
+static inline const char *__string_src(const char *str)
+{
+	if (!str)
+		return "(null)";
+	return str;
+}
+
 /*
  * Fields should never declare an array: i.e. __field(int, arr[5])
  * If they do, it will cause issues in parsing and possibly corrupt the
@@ -47,7 +54,7 @@
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item,			\
-		    strlen((src) ? (const char *)(src) : "(null)") + 1)
+		    strlen(__string_src(src)) + 1)
 
 #undef __string_len
 #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)

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

[Alison Schofield]

On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2024 14:05:00 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > > 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: n
> 
> The full warning didn't add any new information. The above was all I
> needed. But I'm curious if you applied this patch if the warning will go
> away. (Note I didn't test nor compile this).
> 
> -- Steve
> 
> diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> index e30a13be46ba..dfae18d8f4df 100644
> --- a/include/trace/stages/stage5_get_offsets.h
> +++ b/include/trace/stages/stage5_get_offsets.h
> @@ -9,6 +9,13 @@
>  #undef __entry
>  #define __entry entry
>  

+#ifndef STAGE5_H_INCLUDED
+#define STAGE5_H_INCLUDED
> +static inline const char *__string_src(const char *str)
> +{
> +	if (!str)
> +		return "(null)";
> +	return str;
> +}
+#endif /* STAGE5_H_INCLUDED */

Happy to try it out...

Your diff, with the ifdef above, makes the compiler happy and it functions
as wanted - no garbage in the fields.

> +
>  /*
>   * Fields should never declare an array: i.e. __field(int, arr[5])
>   * If they do, it will cause issues in parsing and possibly corrupt the
> @@ -47,7 +54,7 @@
>  
>  #undef __string
>  #define __string(item, src) __dynamic_array(char, item,			\
> -		    strlen((src) ? (const char *)(src) : "(null)") + 1)
> +		    strlen(__string_src(src)) + 1)
>  
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)

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

[Steven Rostedt]

On Thu, 14 Mar 2024 15:36:48 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > index e30a13be46ba..dfae18d8f4df 100644
> > --- a/include/trace/stages/stage5_get_offsets.h
> > +++ b/include/trace/stages/stage5_get_offsets.h
> > @@ -9,6 +9,13 @@
> >  #undef __entry
> >  #define __entry entry
> >    
> 
> +#ifndef STAGE5_H_INCLUDED
> +#define STAGE5_H_INCLUDED
> > +static inline const char *__string_src(const char *str)
> > +{
> > +	if (!str)
> > +		return "(null)";
> > +	return str;
> > +}  
> +#endif /* STAGE5_H_INCLUDED */
> 
> Happy to try it out...
> 
> Your diff, with the ifdef above, makes the compiler happy and it functions
> as wanted - no garbage in the fields.

Hmm, I guess the:

 strlen((src ? (const char *)(src) : NULL) ? (src ? (const char *) src : NULL)) ? "(null)")) + 1

was too complex for the compiler, where as:

 strlen(__string_str((str ? (const char *)(src) : NULL))) + 1

is not.

Perhaps I should add this change. I would still submit your change with the
"" as that needs to be backported, but this probably does not.

Thanks for testing it out.

-- Steve

> 
> > +
> >  /*
> >   * Fields should never declare an array: i.e. __field(int, arr[5])
> >   * If they do, it will cause issues in parsing and possibly corrupt the
> > @@ -47,7 +54,7 @@
> >  
> >  #undef __string
> >  #define __string(item, src) __dynamic_array(char, item,			\
> > -		    strlen((src) ? (const char *)(src) : "(null)") + 1)
> > +		    strlen(__string_src(src)) + 1)
> >  
> >  #undef __string_len
> >  #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)  

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

[Steven Rostedt]

On Thu, 14 Mar 2024 15:36:48 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> > On Thu, 14 Mar 2024 14:05:00 -0700
> > Alison Schofield <alison.schofield@intel.com> wrote:
> >   
> > > > 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: n  
> > 
> > The full warning didn't add any new information. The above was all I
> > needed. But I'm curious if you applied this patch if the warning will go
> > away. (Note I didn't test nor compile this).
> > 
> > -- Steve
> > 
> > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > index e30a13be46ba..dfae18d8f4df 100644
> > --- a/include/trace/stages/stage5_get_offsets.h
> > +++ b/include/trace/stages/stage5_get_offsets.h
> > @@ -9,6 +9,13 @@
> >  #undef __entry
> >  #define __entry entry
> >    
> 
> +#ifndef STAGE5_H_INCLUDED
> +#define STAGE5_H_INCLUDED
> > +static inline const char *__string_src(const char *str)
> > +{
> > +	if (!str)
> > +		return "(null)";
> > +	return str;
> > +}  
> +#endif /* STAGE5_H_INCLUDED */
> 
> Happy to try it out...
> 
> Your diff, with the ifdef above, makes the compiler happy and it functions
> as wanted - no garbage in the fields.

I created the patch:

  https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home/

That adds this. But now the kernel will not build because it requires the
fix of this patch. Otherwise it fails with:

In file included from /work/build/trace/nobackup/linux-test.git/include/trace/define_trace.h:102,
                 from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.h:713,
                 from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.c:8:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
/work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h:660:34: error: passing argument 1 of ‘__string_src’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  660 |                 __string(region, region)
      |                                  ^~~~~~
      |                                  |
      |                                  struct cxl_region *

I like that the patch also catches other bugs where non strings are passed
to __string(). But I can't send this to Linus if it breaks the build.

I'm breaking my pull request up as there's some other places that break the
build with my new checks. I'll do a pull of all my updates first, and after
Linus pulls in my change I will base the next pull off of that merge commit.

But this requires that this fix is in Linus's tree before that. If not, can
I take this fix and add it to that later pull request with all the tests. I
want to make sure that the kernel always builds.

-- Steve


> 
> > +
> >  /*
> >   * Fields should never declare an array: i.e. __field(int, arr[5])
> >   * If they do, it will cause issues in parsing and possibly corrupt the
> > @@ -47,7 +54,7 @@
> >  
> >  #undef __string
> >  #define __string(item, src) __dynamic_array(char, item,			\
> > -		    strlen((src) ? (const char *)(src) : "(null)") + 1)
> > +		    strlen(__string_src(src)) + 1)
> >  
> >  #undef __string_len
> >  #define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)  

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

[Dan Williams]

Steven Rostedt wrote:
> On Thu, 14 Mar 2024 15:36:48 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > On Thu, Mar 14, 2024 at 05:17:37PM -0400, Steven Rostedt wrote:
> > > On Thu, 14 Mar 2024 14:05:00 -0700
> > > Alison Schofield <alison.schofield@intel.com> wrote:
> > >   
> > > > > 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: n  
> > > 
> > > The full warning didn't add any new information. The above was all I
> > > needed. But I'm curious if you applied this patch if the warning will go
> > > away. (Note I didn't test nor compile this).
> > > 
> > > -- Steve
> > > 
> > > diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
> > > index e30a13be46ba..dfae18d8f4df 100644
> > > --- a/include/trace/stages/stage5_get_offsets.h
> > > +++ b/include/trace/stages/stage5_get_offsets.h
> > > @@ -9,6 +9,13 @@
> > >  #undef __entry
> > >  #define __entry entry
> > >    
> > 
> > +#ifndef STAGE5_H_INCLUDED
> > +#define STAGE5_H_INCLUDED
> > > +static inline const char *__string_src(const char *str)
> > > +{
> > > +	if (!str)
> > > +		return "(null)";
> > > +	return str;
> > > +}  
> > +#endif /* STAGE5_H_INCLUDED */
> > 
> > Happy to try it out...
> > 
> > Your diff, with the ifdef above, makes the compiler happy and it functions
> > as wanted - no garbage in the fields.
> 
> I created the patch:
> 
>   https://lore.kernel.org/linux-trace-kernel/20240314232754.345cea82@rorschach.local.home/
> 
> That adds this. But now the kernel will not build because it requires the
> fix of this patch. Otherwise it fails with:
> 
> In file included from /work/build/trace/nobackup/linux-test.git/include/trace/define_trace.h:102,
>                  from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.h:713,
>                  from /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/trace.c:8:
> /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h: In function ‘trace_event_get_offsets_cxl_poison’:
> /work/build/trace/nobackup/linux-test.git/drivers/cxl/core/./trace.h:660:34: error: passing argument 1 of ‘__string_src’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>   660 |                 __string(region, region)
>       |                                  ^~~~~~
>       |                                  |
>       |                                  struct cxl_region *
> 
> I like that the patch also catches other bugs where non strings are passed
> to __string(). But I can't send this to Linus if it breaks the build.
> 
> I'm breaking my pull request up as there's some other places that break the
> build with my new checks. I'll do a pull of all my updates first, and after
> Linus pulls in my change I will base the next pull off of that merge commit.
> 
> But this requires that this fix is in Linus's tree before that. If not, can
> I take this fix and add it to that later pull request with all the tests. I
> want to make sure that the kernel always builds.

Acked-by: Dan Williams <dan.j.williams@intel.com>

...for you taking the fix through your tree, and yes, please Cc stable
on it as well.

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

[Steven Rostedt]

On Fri, 15 Mar 2024 09:18:04 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Acked-by: Dan Williams <dan.j.williams@intel.com>
> 
> ...for you taking the fix through your tree, and yes, please Cc stable
> on it as well.

Thanks, will do!

I'll pull it in and start running my tests on it.

-- Steve

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

[Ira Weiny]

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

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