Re: [PATCH v6 3/4] fgraph: Enhance funcgraph-retval with BTF-based type-aware output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] trace: incorrect BTF formatting for static functions due to name collision
- [Medium] trace: O(Modules) overhead per missing BTF function return
- [Medium] trace: error pointers are unconditionally printed as raw hex
--

commit adb7ea59c69663f88eec1f3f789cb7dab120bb0f
Author: pengdonglin <pengdonglin@xiaomi.com>

    fgraph: Enhance funcgraph-retval with BTF-based type-aware output
    
    The current funcgraph-retval implementation suffers from two accuracy
    issues:
    
    1. Void-returning functions still print a return value, creating
       misleading noise in the trace output.
    
    2. For functions returning narrower types (e.g., char, short), the
       displayed value can be incorrect because high bits of the register
       may contain undefined data.

> diff --git a/kernel/trace/trace_btf.c b/kernel/trace/trace_btf.c
> index ae562192b619f..bcf65fec7bb14 100644
> --- a/kernel/trace/trace_btf.c
> +++ b/kernel/trace/trace_btf.c
> @@ -123,3 +124,85 @@ const struct btf_member *btf_find_struct_member(struct btf *btf,
>  	return member;
>  }
>  
> +void btf_trim_retval(unsigned long func, unsigned long *retval, bool *print_retval,
> +			int *fmt, bool hex)
> +{
> +	const struct btf_type *t;
> +	char name[KSYM_NAME_LEN];
> +	struct btf *btf;
> +	u32 v, msb;
> +	bool signed_type;
> +	int kind;
> +
> +	if (lookup_symbol_name(func, name))
> +		return;
> +
> +	t = btf_find_func_proto(name, &btf);

[Severity: High]
Is it possible that converting the instruction pointer to a string name here
causes incorrect BTF formatting for static functions due to name collisions?

When tracing a static function, `btf_find_func_proto` falls back to scanning
the vmlinux BTF and returning the first function with that name. Since the
kernel has thousands of name collisions for static functions, couldn't this
silently corrupt trace precision by applying the wrong return type formatting
(e.g., truncating an int or silencing a return value by treating it as void)?

[ ... ]

> +	case BTF_KIND_INT:
> +		v = *(u32 *)(t + 1);
> +		signed_type = false;
> +		if (BTF_INT_ENCODING(v) == BTF_INT_BOOL) {
> +			*fmt = RETVAL_FMT_BOOL;
> +			msb = 0;
> +		} else {
> +			signed_type = BTF_INT_ENCODING(v) & BTF_INT_SIGNED;
> +			if (signed_type)
> +				*fmt = RETVAL_FMT_DEC;
> +			else
> +				*fmt = RETVAL_FMT_HEX;
> +
> +			if (t->size > sizeof(unsigned long)) {
> +				*fmt |= RETVAL_FMT_TRUNC;
> +				msb = BITS_PER_LONG - 1;
> +			} else {
> +				msb = min_t(u32, BTF_INT_BITS(v) - 1,
> +					    BITS_PER_LONG - 1);
> +			}
> +		}
> +		*retval &= GENMASK(msb, 0);
> +		if (signed_type && !hex)
> +			*retval = sign_extend64(*retval, msb);
> +		break;
> +	default:
> +		*fmt = RETVAL_FMT_HEX;
> +		break;

[Severity: Medium]
Will this default fallback for pointer types bypass the error code heuristic?

When `btf_trim_retval` identifies a pointer return type (`BTF_KIND_PTR`), it
sets `RETVAL_FMT_HEX` and later flags it with `RETVAL_FMT_BTF`.

In `print_graph_retval`:

    if (retval && retval_fmt != RETVAL_FMT_HEX &&
        !(fmt & RETVAL_FMT_BTF)) {
            /* Check if the return value matches the negative format */

The condition `!(fmt & RETVAL_FMT_BTF)` evaluates to false, causing the legacy
`IS_ERR_VALUE()` error decoding heuristic to be skipped. Could this cause any
function returning an error pointer to be unconditionally printed as an
unreadable hexadecimal (e.g., `0xfffffffffffffff4` instead of `-12`)?

> +	}
> +
> +	if (*print_retval)
> +		*fmt |= RETVAL_FMT_BTF;
> +	btf_put(btf);
> +}

> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index ff7cb1a76b953..8645a33f0388a 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -875,9 +876,10 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
>  {
>  	unsigned long err_code = 0;
>  	unsigned long retval = 0;
> +	bool hex_format;
>  	bool print_retaddr = false;
>  	bool print_retval = false;
> -	bool hex_format = !!(opt_flags & TRACE_GRAPH_PRINT_RETVAL_HEX);
> +	int retval_fmt = 0;
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_RETVAL
>  	retval = graph_ret->retval;
> @@ -888,17 +890,38 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
>  	print_retaddr = !!(opt_flags & TRACE_GRAPH_PRINT_RETADDR);
>  #endif
>  
> -	if (print_retval && retval && !hex_format) {
> -		/* Check if the return value matches the negative format */
> -		if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
> -			(((u64)retval) >> 32) == 0) {
> -			err_code = sign_extend64(retval, 31);
> -		} else {
> -			err_code = retval;
> +	if (print_retval) {
> +		int fmt = RETVAL_FMT_HEX;
> +
> +		hex_format = !!(opt_flags & TRACE_GRAPH_PRINT_RETVAL_HEX);
> +		btf_trim_retval((unsigned long)func, &retval, &print_retval, &fmt,
> +				hex_format);

[Severity: Medium]
Could this call introduce an O(Modules) overhead per missing BTF function
return?

When formatting a trace event for a function missing from vmlinux BTF (e.g.
asm routines), `btf_find_func_proto` falls back to `bpf_find_btf_id`, which
iterates over all loaded modules:

kernel/bpf/btf.c:bpf_find_btf_id() {
    idr_for_each_entry(&btf_idr, btf, id) {
        if (!btf_is_module(btf))
            continue;
        /* linear search could be slow hence unlock/lock
         * the IDR to avoiding holding it for too long
         */
        btf_get(btf);
        spin_unlock_bh(&btf_idr_lock);
        ret = btf_find_by_name_kind(btf, name, kind);
        ...
}

Might this result in a severe performance degradation during trace output
formatting, as any user reading the trace file can trigger this string search
and lock/unlock per module?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916133218.1282590-1-dolinux.peng@gmail.com?part=3