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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Overly broad name-matching heuristics in `syscall_arg_fmt__init_array()` unconditionally intercept non-function pointer fields, corrupting trace output.
- [Medium] Handling of the `work` field fails to symbolize pointers as intended while corrupting unrelated non-pointer integer fields.
- [Low] Redundant entries added to the `syscall_arg_fmts__by_name` lookup table are completely unreachable dead code.
--

--- 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.

>  	{ .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?

[ ... ]

> @@ -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.

>  		} 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