Re: [PATCH 1/2] tracing: Do not let histogram values have some modifiers

[Mark Rutland]

On Wed, Mar 01, 2023 at 08:00:52PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> 
> Histogram values can not be strings, stacktraces, graphs, symbols,
> syscalls, or grouped in buckets or log. Give an error if a value is set to
> do so.
> 
> Note, the histogram code was not prepared to handle these modifiers for
> histograms and caused a bug.
 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
> Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>

Tested-by: Mark Rutland <mark.rutland@xxxxxxx>

I gave this a spin, and I see that the buckets modifier gerts rejected for
hitcount, but is usable for other values as it should be:

| # echo 'p:copy_to_user __arch_copy_to_user n=$arg3' >> /sys/kernel/tracing/kprobe_events
| # echo 'hist:keys=n:vals=hitcount.buckets=8:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
| sh: write error: Invalid argument
| # echo 'hist:keys=n.buckets=8:vals=hitcount:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
| # cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist
| # event histogram
| #
| # trigger info: hist:keys=n.buckets=8:vals=hitcount:sort=hitcount:size=2048 [active]
| #
| 
| { n: ~ 336-343 } hitcount:          1
| { n: ~ 16-23 } hitcount:          2
| { n: ~ 32-39 } hitcount:          2
| { n: ~ 832-839 } hitcount:          3
| { n: ~ 8-15 } hitcount:          3
| { n: ~ 128-135 } hitcount:          5
| { n: ~ 0-7 } hitcount:         57
| 
| Totals:
|     Hits: 73
|     Entries: 7
|     Dropped: 0

Thanks,
Mark.

> ---
>  kernel/trace/trace_events_hist.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 89877a18f933..6e8ab726a7b5 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4235,6 +4235,15 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
>  		goto out;
>  	}
>  
> +	/* Some types cannot be a value */
> +	if (hist_field->flags & (HIST_FIELD_FL_GRAPH | HIST_FIELD_FL_PERCENT |
> +				 HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
> +				 HIST_FIELD_FL_SYM | HIST_FIELD_FL_SYM_OFFSET |
> +				 HIST_FIELD_FL_SYSCALL | HIST_FIELD_FL_STACKTRACE)) {
> +		hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
> +		ret = -EINVAL;
> +	}
> +
>  	hist_data->fields[val_idx] = hist_field;
>  
>  	++hist_data->n_vals;
> -- 
> 2.39.1