Re: [PATCH perf-tools-next v4 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:
- [Medium] Using the host's `sizeof(void *)` to validate tracepoint field sizes breaks cross-architecture analysis.
- [Medium] The type-based heuristic for function pointers assigns `SCA_KSYM` without array safety guards, which can lead to misinterpretation of data and leaking of perf's internal heap addresses.
--

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

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?

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