Re: [PATCH 1/4] tracing/histogram: Don't use strlen to find length of stacktrace variables

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

On Fri, 10 Feb 2023 15:33:03 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> Because stacktraces are saved in dynamic strings,
> trace_event_raw_event_synth() uses strlen to determine the length of
> the stack.  Stacktraces may contain 0-bytes, though, in the saved
> addresses, so the length found and passed to reserve() will be too
> small.

Good catch!

> 
> Fix this by using the first unsigned long in the stack variables to
> store the actual number of elements in the stack and have
> trace_event_raw_event_synth() use that to determine the length of the
> stack.
> 
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---
>  kernel/trace/trace_events_hist.c  | 12 ++++++++----
>  kernel/trace/trace_events_synth.c |  7 ++++++-
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 888b7a394ce5..76bd105988c6 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -3135,13 +3135,15 @@ static inline void __update_field_vars(struct tracing_map_elt *elt,
>  				size = min(val->size, STR_VAR_LEN_MAX);
>  				strscpy(str, val_str, size);
>  			} else {
> +				char *stack_start = str + sizeof(unsigned long);
>  				int e;
>  
> -				e = stack_trace_save((void *)str,
> +				e = stack_trace_save((void *)stack_start,
>  						     HIST_STACKTRACE_DEPTH,
>  						     HIST_STACKTRACE_SKIP);

BTW, the size of "str" is enough to store HIST_STACKTRACE_DEPTH?
In string case, 

size = min(val->size, STR_VAR_LEN_MAX);

will limit the max size.

Thank you,

>  				if (e < HIST_STACKTRACE_DEPTH - 1)
> -					((unsigned long *)str)[e] = 0;
> +					((unsigned long *)stack_start)[e] = 0;
> +				*((unsigned long *)str) = e;
>  			}
>  			var_val = (u64)(uintptr_t)str;
>  		}
> @@ -5133,13 +5135,15 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
>  					size = min(hist_field->size, STR_VAR_LEN_MAX);
>  					strscpy(str, val_str, size);
>  				} else {
> +					char *stack_start = str + sizeof(unsigned long);
>  					int e;
>  
> -					e = stack_trace_save((void *)str,
> +					e = stack_trace_save((void *)stack_start,
>  							     HIST_STACKTRACE_DEPTH,
>  							     HIST_STACKTRACE_SKIP);
>  					if (e < HIST_STACKTRACE_DEPTH - 1)
> -						((unsigned long *)str)[e] = 0;
> +						((unsigned long *)stack_start)[e] = 0;
> +					*((unsigned long *)str) = e;
>  				}
>  				hist_val = (u64)(uintptr_t)str;
>  			}
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index d458d7a0dfd7..6209b23c863f 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -538,7 +538,12 @@ static notrace void trace_event_raw_event_synth(void *__data,
>  		val_idx = var_ref_idx[field_pos];
>  		str_val = (char *)(long)var_ref_vals[val_idx];
>  
> -		len = kern_fetch_store_strlen((unsigned long)str_val);
> +		if (event->dynamic_fields[i]->is_stack) {
> +			len = *((unsigned long *)str_val);
> +			len *= sizeof(unsigned long);
> +		} else {
> +			len = kern_fetch_store_strlen((unsigned long)str_val);
> +		}
>  
>  		fields_size += len;
>  	}
> -- 
> 2.34.1
> 


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