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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mapping highly generic field names like 'action', 'cb', and 'caller' globally to `SCA_KSYM` short-circuits type-based logic, leaking perf userspace memory addresses instead of strings and breaking integers/enums.
- [Medium] Removal of `trace__field_is_ip()` and omission of `caller_ip` from the new lookup table causes kernel instruction pointers to be formatted as unreadable decimal integers.
--

--- Patch [2]: [PATCH perf-tools-next v3 2/4] perf trace: Auto-assign kernel symbol beautifier to function pointer fields ---
commit d3017d506b83550526e2692582e2544d0b006d0f
Author: Aaron Tomlin <[email protected]>

perf trace: Auto-assign kernel symbol beautifier to function pointer fields

This patch enhances perf trace to automatically format kernel function
pointer fields using the SCA_KSYM beautifier. It detects these fields
by type signature or by matching common function pointer and callback
field names in a lookup table.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 0030489465032..bb8f4fd9ba244 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2096,6 +2096,17 @@ 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 = "call_site",	.scnprintf = SCA_KSYM, },
> +	{ .name = "callback",	.scnprintf = SCA_KSYM, },
> +	{ .name = "caller",	.scnprintf = SCA_KSYM, },
> +	{ .name = "callsite",	.scnprintf = SCA_KSYM, },
> +	{ .name = "cb",		.scnprintf = SCA_KSYM, },

[ ... ]

> @@ -2230,6 +2220,24 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
>  			if (fmt) {
>  				arg->scnprintf = fmt->scnprintf;
>  				arg->strtoul   = fmt->strtoul;
> +			} else if ((field->flags & TEP_FIELD_IS_POINTER) || strstr(field->name, "addr") ||
> +				   field_has_hex_fmt(field, len)) {
> +				arg->scnprintf = SCA_PTR;
> +			} else if (strcmp(field->type, "pid_t") == 0) {
> +				arg->scnprintf = SCA_PID;
> +			} else if (strcmp(field->type, "umode_t") == 0) {
> +				arg->scnprintf = SCA_MODE_T;
> +			} else if ((field->flags & TEP_FIELD_IS_ARRAY) && strstr(field->type, "char")) {
> +				arg->scnprintf = SCA_CHAR_ARRAY;
> +				arg->nr_entries = field->arraylen;

[Severity: High]
Does checking syscall_arg_fmt__find_by_name() earlier in this branch,
before the type-based checks like string arrays or BTF enums, short-circuit
those checks for generic field names?

If a field is named "caller", "action", or "cb" but is actually a dynamic
string or integer, it seems this early name match will blindly assign
SCA_KSYM.

Would this cause strings to be printed as raw kernel addresses by passing a
userspace perf pointer to SCA_KSYM, permanently obfuscating the trace data?

[ ... ]

> @@ -3297,12 +3305,6 @@ static unsigned char bitmap_byte(const unsigned long *mask, int byte_idx)
>  	return b_val;
>  }
>  
> -static bool trace__field_is_ip(const char *name)
> -{
> -	return !strcmp(name, "__probe_ip") ||
> -	       !strcmp(name, "caller_ip") ||
> -	       !strcmp(name, "call_site");
> -}

[Severity: Medium]
Since trace__field_is_ip() is removed here, should "caller_ip" have been
added to the new syscall_arg_fmts__by_name lookup table?

Because "caller_ip" is typed as unsigned long (not a pointer), lacks "addr"
in its name, and its TP_printk format strings do not match the strict
=0x/=%p checks in field_has_hex_fmt(), it evades all heuristics.

Will this cause kernel instruction pointers like "caller_ip" (used heavily
in XFS and OCFS2 tracepoints) to be formatted as unreadable base-10 decimal
integers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2