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

On Sat, Aug 15, 2026 at 11:47:37PM +0000, [email protected] wrote:
> --- Patch [2]: [PATCH perf-tools-next 2/3] perf trace: Auto-assign kernel symbol beautifier to function pointer fields ---
> commit e41ca802a20df2b74611c38302bea036b4cc9f1f
> Author: Aaron Tomlin <[email protected]>
> 
> perf trace: Auto-assign kernel symbol beautifier to function pointer fields
> 
> This patch enhances syscall_arg_fmt__init_array() to detect kernel function
> pointers and assign SCA_KSYM as their default beautifier. It also registers
> common function pointer field names in the syscall_arg_fmts__by_name lookup
> table to automatically symbolize them.
> 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 102221b301a54..825b16581581b 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -2091,8 +2091,13 @@ static int syscall__alloc_arg_fmts(struct syscall *sc, int nr_args)
> >  }
> >  
> >  static const struct syscall_arg_fmt syscall_arg_fmts__by_name[] = {
> > +	{ .name = "action",	.scnprintf = SCA_KSYM, },
> > +	{ .name = "callsite",	.scnprintf = SCA_KSYM, },
> > +	{ .name = "fn",		.scnprintf = SCA_KSYM, },
> > +	{ .name = "function",	.scnprintf = SCA_KSYM, },
> 
> [Severity: Low]
> Are these lookup table entries reachable? 
> 
> In syscall_arg_fmt__init_array(), there is an else-if block added by this
> patch that explicitly intercepts these exact field names. It appears these
> lookup table entries would never be evaluated for these strings.

Indeed syscall_arg_fmt__init_array() intercepts these field names in an
earlier else-if branch, these entries in syscall_arg_fmts__by_name[] were
dead code and never evaluated. I'll remove the redundant entries
(i.e., "action", "callsite", "fn", "function") from
syscall_arg_fmts__by_name[] for v2.


> >  	{ .name = "msr",	.scnprintf = SCA_X86_MSR,	  .strtoul = STUL_X86_MSR,	   },
> >  	{ .name = "vector",	.scnprintf = SCA_X86_IRQ_VECTORS, .strtoul = STUL_X86_IRQ_VECTORS, },
> > +	{ .name = "work",	.scnprintf = SCA_KSYM, },
> 
> [Severity: Medium]
> Does this correctly symbolize work pointers, and could it inadvertently
> corrupt unrelated non-pointer integer fields?
> 
> For workqueue tracepoints where the work field is a pointer, it appears it
> will be intercepted earlier in syscall_arg_fmt__init_array() by the
> (field->flags & TEP_FIELD_IS_POINTER) check. That check assigns SCA_PTR 
> instead of allowing the field to reach this lookup table.
> 
> Conversely, for tracepoints like napi_poll that have a non-pointer integer
> field named "work", the pointer check is skipped, allowing it to hit this
> lookup table. Wouldn't this assign SCA_KSYM to the integer, causing it to
> erroneously print as a hex pointer or NULL instead of a decimal number?

For workqueue events, work is a struct work_struct * data structure pointer
(formatted via SCA_PTR), whereas function (work_func_t) is the actual
function pointer.

Adding "work" to syscall_arg_fmts__by_name[] had no effect on workqueue
pointers (which hit TEP_FIELD_IS_POINTER first), but it incorrectly forced
non-pointer integer fields named "work" (such as napi_poll.work) to be
formatted via SCA_KSYM.

I'll remove "work" from the lookup table for v2 so integer fields continue
to be formatted as decimal numbers.


> > @@ -2193,6 +2198,14 @@ 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 (strcmp(field->name, "fn") == 0 ||
> > +			   strcmp(field->name, "function") == 0 ||
> > +			   strcmp(field->name, "callsite") == 0 ||
> > +			   strcmp(field->name, "action") == 0 ||
> > +			   (field->type && (strstr(field->type, "(*)") != NULL ||
> > +					    strstr(field->type, "_func_t") != NULL ||
> > +					    strstr(field->type, "_fn") != NULL))) {
> > +			arg->scnprintf = SCA_KSYM;
> 
> [Severity: Medium]
> Does this unconditional name matching affect non-pointer integer fields
> that happen to share these generic names? 
> 
> If a tracepoint has an integer field named "action" or "function" (which
> can occur in subsystems like btrfs, power, or xfs), wouldn't this assign
> SCA_KSYM to it regardless of its actual type? 
> 
> This might cause perf trace to treat simple integers as kernel addresses,
> failing to resolve a symbol and formatting them incorrectly.

Agreed. Matching field names unconditionally without checking field flags
or size caused non-pointer integer/enum fields like int action or u32
function to be misassigned SCA_KSYM.

In v2, I'll update syscall_arg_fmt__init_array() to restrict name-based
SCA_KSYM assignment so that it only applies if the field is actually a
pointer (TEP_FIELD_IS_POINTER) or a 64-bit address (field->size ==
sizeof(u64)), alongside type signature matching.


> 
> >  		} else if ((field->flags & TEP_FIELD_IS_POINTER) || strstr(field->name, "addr") ||
> >  			   field_has_hex_fmt(field, len))
> >  			arg->scnprintf = SCA_PTR;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2


Kind regards,
-- 
Aaron Tomlin