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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `syscall_arg_fmt__cache_btf_type()` causes severe performance degradation due to un-cached O(N) BTF lookups for standard C types.
- [Medium] Pointers to enums are incorrectly evaluated as enum values, leading to silent truncation and invalid enum string formatting.
- [Medium] Function pointer symbolization silently fails for inline function pointers and pointers to function prototype typedefs.
--

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

This commit introduces btf_is_func_ptr() to inspect BTF type hierarchies
to determine if a parameter is a function prototype. It generalizes BTF
type caching to handle various types via syscall_arg_fmt__cache_btf_type(),
allowing for automatic symbolization of kernel function pointers in
perf trace.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 85abae09d328b..cb3cb877a22e9 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -979,21 +979,61 @@ 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)
>  {
> +	bool has_ptr = false;
> +
> +	while (type) {
> +		if (btf_is_ptr(type)) {
> +			has_ptr = true;
> +			type = btf__type_by_id(btf, type->type);
> +		} else if (btf_is_typedef(type) || btf_is_mod(type)) {
> +			type = btf__type_by_id(btf, type->type);
> +		} else {
> +			break;
> +		}
> +	}
> +	return has_ptr && type && btf_is_func_proto(type);
> +}

[Severity: Medium]
Will this reliably detect function pointers if the pointer modifier was
stripped during parsing?

If a parameter is an inline function pointer (e.g., void (*cb)(int)) or a
pointer to a function prototype typedef (e.g., foo_t *cb), the string
extraction in syscall_arg_fmt__cache_btf_type() strips the pointer and
punctuation.

For foo_t, it resolves to a typedef of a prototype, so btf_is_func_ptr()
evaluates has_ptr = false since the pointer aspect was stripped. Symbolization
only seems to work for typedefs that intrinsically encapsulate the pointer.

> +
> +static void syscall_arg_fmt__cache_btf_type(struct syscall_arg_fmt *arg_fmt,
> +					    struct btf *btf, const char *type)
> +{
> +	char name[128];
> +	const char *pos;
> +	size_t len = 0;
>  	int id;
>  
> -	type = strstr(type, "enum ");
>  	if (type == NULL)
>  		return;
>  
> -	type += 5; // skip "enum " to get the enumeration name
> +	if ((pos = strstr(type, "enum ")) != NULL)
> +		pos += 5;
> +	else if ((pos = strstr(type, "struct ")) != NULL)
> +		pos += 7;
> +	else if ((pos = strstr(type, "union ")) != NULL)
> +		pos += 6;
> +	else
> +		pos = type;
> +
> +	while (isspace(*pos))
> +		pos++;
> +
> +	while ((isalnum(pos[len]) || pos[len] == '_') && len < sizeof(name) - 1) {
> +		name[len] = pos[len];
> +		len++;
> +	}

[Severity: Medium]
Does extracting only alphanumeric characters incorrectly strip pointer
modifiers and function pointer structures?

If a traced syscall takes an argument of type enum foo *, this logic extracts
only the alphanumeric words, stripping the pointer modifier. This causes the
parameter to resolve to the base BTF_KIND_ENUM type rather than a pointer.

> +	name[len] = '\0';
>  
> -	id = btf__find_by_name(btf, type);
> +	if (len == 0)
> +		return;
> +
> +	id = btf__find_by_name(btf, name);
>  	if (id < 0)
>  		return;
>  
>  	arg_fmt->type = btf__type_by_id(btf, id);
> +	arg_fmt->type_id = id;
>  }

[Severity: High]
Does this logic lead to repeated, expensive searches if a type is not found?

If btf__find_by_name() fails to find a type (which it does for standard C types
like unsigned long or void *), arg_fmt->type remains NULL.

trace__btf_scnprintf() checks if arg_fmt->type is NULL and repeatedly calls
syscall_arg_fmt__cache_btf_type():

trace__btf_scnprintf() {
    ...
    if (arg_fmt->type == NULL) {
        syscall_arg_fmt__cache_btf_type(arg_fmt, trace->btf, type);
    }
    ...
}

This results in an un-cached linear search over all kernel BTF types for every
single trace event when a type is not found, potentially causing a severe
performance regression.

[ ... ]

> @@ -1108,7 +1147,7 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
>  }
>  
>  static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg, char *bf,
> -				   size_t size, int val, char *type)
> +				   size_t size, unsigned long val, char *type)
>  {
>  	struct syscall_arg_fmt *arg_fmt = arg->fmt;
>  
> @@ -1116,8 +1155,7 @@ static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg,
>  		return 0;
>  
>  	if (arg_fmt->type == NULL) {
> -		// Check if this is an enum and if we have the BTF type for it.
> -		syscall_arg_fmt__cache_btf_enum(arg_fmt, trace->btf, type);
> +		syscall_arg_fmt__cache_btf_type(arg_fmt, trace->btf, type);
>  	}
>  
>  	// Did we manage to find a BTF type for the syscall/tracepoint argument?
> @@ -1128,13 +1166,17 @@ 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);

[Severity: Medium]
Can this truncate a pointer address to 32 bits?

Since val is an unsigned long (64 bits), passing it to btf_enum_scnprintf()
which takes an int truncates the value.

If an argument of type enum foo * had its pointer modifier stripped by
syscall_arg_fmt__cache_btf_type(), this will silently truncate the pointer
address to 32 bits. If the truncated value happens to match an enum member, it
incorrectly displays the enum string instead of the pointer address.

>  	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)) {
> +		arg->val = val;
> +		return syscall_arg__scnprintf_ksym(bf, size, arg);
> +	}
>  
>  	return 0;
>  }

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