Re: [PATCH] tracing/eprobe: Update cond flag before enabling trigger

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 18 Nov 2022 11:19:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Nov 2022 10:34:40 -0300
> Rafael Mendonca <rafaelmendsr@gmail.com> wrote:
> 
> > It did not trigger the NULL pointer issue to be more specific. When
> > creating event probe for all events I was unable to create any event for
> > the xhci-hcd system:
> > 
> > root@localhost:/sys/kernel/tracing# echo 'e xhci-hcd/xhci_add_endpoint' > dynamic_events 
> > -bash: echo: write error: Invalid argument
> > 
> > Debugging the issue it seems that the problem is in the is_good_name()
> > check, which returns false for "xhci-hcd". Should we sanitize it by
> 
> Ouch. I didn't realize that.

Maybe we need better error message so that user can notice which character
caused the error. 

> 
> > converting '-' into '_'?
> 
> Actually, it's just the system name that's an issue. I tested this patch
> and it appears to work.

Ah, the system name is more flexible than the event name because it has
TRACE_SYSTEM_VAR.

Steve, can you send me the below patch?

BTW, TRACE_DEFINE_ENUM() and TRACE_DEFINE_SIZEOF() macros are using
TRACE_SYSTEM instead of TRACE_SYSTEM_VAR. Should those use TRACE_SYSTEM_VAR
for defining a variable?

Thank you,

> 
> -- Steve
> 
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 54ee5711c729..a16fb4c9642e 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1955,17 +1955,30 @@ static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
>  }
>  
>  /* Check the name is good for event/group/fields */
> -static inline bool is_good_name(const char *name)
> +static inline bool __is_good_name(const char *name, bool hash_ok)
>  {
> -	if (!isalpha(*name) && *name != '_')
> +	if (!isalpha(*name) && *name != '_' && (!hash_ok || *name != '-'))
>  		return false;
>  	while (*++name != '\0') {
> -		if (!isalpha(*name) && !isdigit(*name) && *name != '_')
> +		if (!isalpha(*name) && !isdigit(*name) && *name != '_' &&
> +		    (!hash_ok || *name != '-'))
>  			return false;
>  	}
>  	return true;
>  }
>  
> +/* Check the name is good for event/group/fields */
> +static inline bool is_good_name(const char *name)
> +{
> +	return __is_good_name(name, false);
> +}
> +
> +/* Check the name is good for system */
> +static inline bool is_good_system_name(const char *name)
> +{
> +	return __is_good_name(name, true);
> +}
> +
>  /* Convert certain expected symbols into '_' when generating event names */
>  static inline void sanitize_event_name(char *name)
>  {
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 36dff277de46..bb2f95d7175c 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -246,7 +246,7 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>  			return -EINVAL;
>  		}
>  		strlcpy(buf, event, slash - event + 1);
> -		if (!is_good_name(buf)) {
> +		if (!is_good_system_name(buf)) {
>  			trace_probe_log_err(offset, BAD_GROUP_NAME);
>  			return -EINVAL;
>  		}


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>