Re: [PATCH perf-tools-next v4 2/4] perf trace: Auto-assign kernel symbol beautifier to function pointer fields

On Fri, Aug 21, 2026 at 08:58:14PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> perf trace: Auto-assign kernel symbol beautifier to function pointer fields
> 
> This commit enhances perf trace to automatically assign the SCA_KSYM
> beautifier to tracepoint fields conveying function pointers. It uses
> heuristics based on both type signatures and field names to detect these
> pointers.
> 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 0030489465032..0a12d7b37d8d4 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> [ ... ]
> > @@ -2198,38 +2210,47 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
> >  		    ((len >= 4 && strcmp(field->name + len - 4, "name") == 0) ||
> >  		     strstr(field->name, "path") != NULL)) {
> >  			arg->scnprintf = SCA_FILENAME;
> [ ... ]
> > +		} else if (field->type && (strstr(field->type, "(*)") != NULL ||
> > +					   strstr(field->type, "_func_t") != NULL ||
> > +					   strstr(field->type, "_fn") != NULL)) {
> > +			arg->scnprintf = SCA_KSYM;
> 
> [Severity: Medium]
> Will this unconditionally assign SCA_KSYM for matching types without checking
> if the field is an array?
> 
> If this encounters a tracepoint with an array field whose type matches
> the heuristic (like an array of function pointer typedefs),
> trace__fprintf_tp_fields() will assign a pointer to the tool's raw heap buffer
> to the value.
> 
> Since SCA_KSYM is applied without an array guard, it might attempt to resolve
> this user-space heap pointer as a kernel symbol, leaking the tool's memory
> address in hex instead of printing the trace data.
> 
> Could we add a check for !(field->flags & TEP_FIELD_IS_ARRAY) here as well?

At the time of writing, a search across all tracepoint definitions in
include/trace/events/ shows that existing tracepoints only use __array()
for byte buffers, UUIDs, filesystem block arrays (e.g., nid_t nid[3]), or
CPU masks. However, this could change.

The type-based heuristic in the 'else if' branch needs this check.
For SCA_KSYM, syscall_arg__scnprintf_ksym() expects arg->val to be an
actual kernel virtual address and passes it to
machine__find_kernel_symbol().

> 
> >  		} else {
> >  			const struct syscall_arg_fmt *fmt =
> >  				syscall_arg_fmt__find_by_name(field->name);
> >  
> >  			if (fmt) {
> > -				arg->scnprintf = fmt->scnprintf;
> > -				arg->strtoul   = fmt->strtoul;
> > +				if (fmt->scnprintf == SCA_KSYM) {
> > +					if ((field->flags & TEP_FIELD_IS_POINTER) ||
> > +					    (field->size == sizeof(void *) && !(field->flags & TEP_FIELD_IS_ARRAY))) {
> 
> [Severity: Medium]
> Does using sizeof(void *) here break cross-architecture analysis?
> 
> When using perf trace on a 64-bit host to process a 32-bit perf.data file for
> events that use unsigned long for instruction pointer fields (like caller_ip in
> XFS tracepoints), field->size in the trace will be 4.
> 
> This fails the sizeof(void *) check, which evaluates to 8 on the 64-bit host,
> preventing the expected symbol resolution. 
> 
> Can we rely on the trace data's pointer size instead of the host's, or use a
> different heuristic?
> 
> > +						arg->scnprintf = fmt->scnprintf;
> > +						arg->strtoul   = fmt->strtoul;
> > +					}
> > +				} else {
> > +					arg->scnprintf = fmt->scnprintf;
> > +					arg->strtoul   = fmt->strtoul;
> > +				}
> > +			}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Indeed. Perhaps we could use tep_get_long_size(), which retrieves the
target's recorded long size; which matches field->size. Therefore,
caller_ip is correctly recognised as an instruction pointer scalar and
symbolised with SCA_KSYM.

Kind regards,
-- 
Aaron Tomlin