Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Tue, 2026-04-07 at 21:05 -0400, Steven Rostedt wrote:
> 
> Tom,
> 
> On Wed,  1 Apr 2026 19:22:23 +0800
> Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:
> 
> > hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> > qualified variable-reference names, but it currently appends into that
> > buffer with strcat() without rebuilding it first. As a result, repeated
> > calls append a new "system.event.field" name onto the previous one,
> > which can eventually run past the end of full_name.
> > 
> > Build the name with snprintf() on each call and return NULL if the fully
> > qualified name does not fit in MAX_FILTER_STR_VAL.
> > 
> > Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > ---
> > Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/
> > 
> > - rebuild full_name on each call instead of falling back to field->name
> > - return NULL on overflow as suggested
> > - split out the snprintf() length check instead of using an inline if
> > 
> >  kernel/trace/trace_events_hist.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 73ea180cad55..f9c8a4f078ea 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
> >  		 field->flags & HIST_FIELD_FL_VAR_REF) {
> >  		if (field->system) {
> >  			static char full_name[MAX_FILTER_STR_VAL];
> > +			int len;
> > +
> > +			len = snprintf(full_name, sizeof(full_name), "%s.%s.%s",
> > +				       field->system, field->event_name,
> > +				       field->name);
> > +			if (len >= sizeof(full_name))
> > +				return NULL;
> >  
> > -			strcat(full_name, field->system);
> > -			strcat(full_name, ".");
> > -			strcat(full_name, field->event_name);
> > -			strcat(full_name, ".");
> > -			strcat(full_name, field->name);
> >  			field_name = full_name;
> 
> I wanted to test this but I can't find anything that triggers this path.
> How does a field here get its ->system set?
> 

->system is set when using fully-qualified variable names. For
instance:

echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> sys/kernel/debug/tracing/events/sched/sched_waking/trigger
echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
echo 'hist:keys=next_pid:lat0=common_timestamp.usecs-sched.sched_waking.$ts0:lat1=common_timestamp.usecs-sched.sched_wakeup.$ts0' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
echo 'hist:keys=next_pid:vals=$lat0,$lat1' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger

Here, the sched_switch trigger would error out if the unqualified $ts0
variables were used instead of the fully-qualified ones because there's
no way to distinguish which $ts0 was meant.

Tom



> If there's no way to hit this path, I much rather remove it than "fix" it.
> 
> -- Steve
> 
> 
> >  		} else
> >  			field_name = field->name;
>