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 <[email protected]> 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/[email protected]?part=3
