Re: [PATCH perf-tools-next 3/3] perf trace: Enhance BTF type formatting to symbolise kernel function pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] `trace__btf_scnprintf()` calls `syscall_arg__scnprintf_ksym()` without `arg->val` being updated, causing kernel function pointers to unconditionally print as "NULL" or display incorrect symbols.
- [Medium] `syscall_arg_fmt__cache_btf_type()` attempts to look up BTF types using raw string matching, which systematically fails for pointer declarations (e.g., `struct task_struct *`).
- [Medium] Replacing `strstr()` with `strstarts()` in `syscall_arg_fmt__cache_btf_type()` breaks BTF type resolution for type strings containing modifiers (e.g., `"const enum"`).
- [Medium] `btf_is_func_ptr()` improperly handles pointers targeting typedefs or modifiers.
--

--- Patch [3]: [PATCH perf-tools-next 3/3] perf trace: Enhance BTF type formatting to symbolise kernel function pointers ---
commit 53acf94abe39f745266994a52d3b39a3f7fa20c2
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Enhance BTF type formatting to symbolise kernel function pointers
    
When BTF metadata is loaded from vmlinux, this commit introduces
btf_is_func_ptr() to identify function prototypes. It generalizes BTF type
caching to handle structs, unions, enums, and function pointers. It also
adds routing to syscall_arg__scnprintf_ksym() to symbolize kernel function
pointers.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 825b16581581b..94dc720a2b3ac 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -974,21 +974,43 @@ static size_t syscall_arg__scnprintf_getrandom_flags(char *bf, size_t size,
>  #define SCA_GETRANDOM_FLAGS syscall_arg__scnprintf_getrandom_flags
>  
>  #ifdef HAVE_LIBBPF_SUPPORT
> -static void syscall_arg_fmt__cache_btf_enum(struct syscall_arg_fmt *arg_fmt, struct btf *btf, char *type)
> +static bool btf_is_func_ptr(const struct btf *btf, const struct btf_type *type)
> +{
> +	while (type) {
> +		if (btf_is_ptr(type)) {
> +			type = btf__type_by_id(btf, type->type);
> +			return type && btf_is_func_proto(type);

[Severity: Medium]
Will this return prematurely without unwrapping typedefs or modifiers on the
pointer's target type? 

If a function pointer is defined via a typedef (for example, typedef void
vq_callback_t(struct virtqueue *); vq_callback_t *cb;), does this unwrap
the pointer kind and immediately return false because it doesn't evaluate
subsequent modifiers on the function prototype?

> +		}
> +		if (btf_is_typedef(type) || btf_is_mod(type)) {
> +			type = btf__type_by_id(btf, type->type);
> +		} else {
> +			break;
> +		}
> +	}
> +	return false;
> +}
> +
> +static void syscall_arg_fmt__cache_btf_type(struct syscall_arg_fmt *arg_fmt,
> +					    struct btf *btf, char *type)
>  {
>  	int id;
>  
> -	type = strstr(type, "enum ");
>  	if (type == NULL)
>  		return;
>  
> -	type += 5; // skip "enum " to get the enumeration name
> +	if (strstarts(type, "enum "))
> +		type += 5;

[Severity: Medium]
Does replacing strstr() with strstarts() break BTF type resolution for type
strings containing modifiers? 

If a tracepoint argument is defined as const enum, strstarts() evaluates
from the beginning and returns false, leaving the full string intact and
causing the subsequent lookup to fail.

> +	else if (strstarts(type, "struct "))
> +		type += 7;
> +	else if (strstarts(type, "union "))
> +		type += 6;
>  
>  	id = btf__find_by_name(btf, type);

[Severity: Medium]
Will this raw string matching systematically fail for pointer declarations
like struct task_struct *? 

The code above strips the prefix but leaves the trailing asterisk intact.
Since pointers are anonymous in BTF and structs are named without asterisks,
does this guarantee btf__find_by_name() will fail to find the type?

>  	if (id < 0)
>  		return;
>  
>  	arg_fmt->type = btf__type_by_id(btf, id);
> +	arg_fmt->type_id = id;
>  }

[ ... ]

> @@ -1123,6 +1143,8 @@ static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg,
>  		return btf_enum_scnprintf(arg_fmt->type, trace->btf, bf, size, val);
>  	else if (btf_is_struct(arg_fmt->type) || btf_is_union(arg_fmt->type))
>  		return btf_struct_scnprintf(arg_fmt->type, trace->btf, bf, size, arg);
> +	else if (btf_is_func_ptr(trace->btf, arg_fmt->type))
> +		return syscall_arg__scnprintf_ksym(bf, size, arg);
>  
>  	return 0;
>  }

[ ... ]

> @@ -2566,7 +2588,8 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
>  
>  			default_scnprintf = sc->arg_fmt[arg.idx].scnprintf;
>  
> -			if (trace->force_btf || default_scnprintf == NULL || default_scnprintf == SCA_PTR) {
> +			if (trace->force_btf || default_scnprintf == NULL ||
> +			    default_scnprintf == SCA_PTR || default_scnprintf == SCA_KSYM) {
>  				btf_printed = trace__btf_scnprintf(trace, &arg, bf + printed,
>  								   size - printed, val, field->type);

[Severity: Medium]
When calling trace__btf_scnprintf() here, the syscall argument val is passed
as a separate parameter, but arg.val is not updated to match val.

In trace__btf_scnprintf(), the code delegates to syscall_arg__scnprintf_ksym()
which expects arg->val to contain the valid address. Will this result in an
uninitialized or stale value being used from arg->val, causing the kernel
function pointer to print as NULL or display incorrect symbols?

>  				if (btf_printed) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815233651.527936-1-atomlin@atomlin.com?part=3