Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Calling bpf_find_btf_id() during trace formatting introduces a spinlock acquisition into atomic/NMI contexts, leading to deadlocks and kernel panics during oops reporting. - [High] bpf_find_btf_id() performs global name-based BTF lookups per trace event, causing severe O(N) performance degradation, lock contention, and incorrect typing for module/static functions. - [Medium] Functions returning empty structs (size 0) cause an integer underflow in the bitmask calculation, triggering undefined behavior in the GENMASK macro. - [Medium] btf_trim_retval incorrectly zero-extends signed narrow types (e.g., s8, s16), breaking the formatting of negative values and error codes. --
commit b33d384f150276d1e47f02dbd04205841c0951e8 Author: pengdonglin <[email protected]> fgraph: Enhance funcgraph-retval with BTF-based type-aware output The current funcgraph-retval implementation suffers from two accuracy issues: void-returning functions print return values, and narrower types can contain undefined data in high bits. This patch leverages BTF to obtain the exact return type of each traced function, eliminating clutter for void functions and properly truncating/formatting return values. > diff --git a/kernel/trace/trace_btf.c b/kernel/trace/trace_btf.c > index 00172f301f25a..38825f5662351 100644 > --- a/kernel/trace/trace_btf.c > +++ b/kernel/trace/trace_btf.c [ ... ] > @@ -120,3 +121,70 @@ const struct btf_member *btf_find_struct_member(struct > btf *btf, > return member; > } > > +#ifdef CONFIG_DEBUG_INFO_BTF > +void btf_trim_retval(unsigned long func, unsigned long *retval, bool > *print_retval, > + int *fmt) > +{ [ ... ] > + switch (kind) { [ ... ] > + case BTF_KIND_STRUCT: > + case BTF_KIND_UNION: > + case BTF_KIND_ENUM: > + case BTF_KIND_ENUM64: > + if (kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION) > + *fmt = RETVAL_FMT_HEX; > + else > + *fmt = RETVAL_FMT_DEC; > + > + if (t->size > sizeof(unsigned long)) { > + *fmt |= RETVAL_FMT_TRUNC; > + } else { > + msb = BITS_PER_BYTE * t->size - 1; > + *retval &= GENMASK(msb, 0); > + } [Severity: Medium] Can this calculation underflow msb? If the traced function returns an empty struct (e.g., struct {} in GNU C), t->size is 0. The calculation BITS_PER_BYTE * 0 - 1 will underflow to 4294967295. Passing this huge value to GENMASK(msb, 0) invokes C undefined behavior due to out-of-bounds bitwise shifts. > + break; > + case BTF_KIND_INT: > + v = *(u32 *)(t + 1); > + if (BTF_INT_ENCODING(v) == BTF_INT_BOOL) { > + *fmt = RETVAL_FMT_BOOL; > + msb = 0; > + } else { > + if (BTF_INT_ENCODING(v) == BTF_INT_SIGNED) > + *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 = BTF_INT_BITS(v) - 1; > + } > + } > + *retval &= GENMASK(msb, 0); [Severity: Medium] Will this unconditional masking destroy the sign bit extension for signed narrow integers like s8 or s16? The masking clears the upper sign-extension bits. Since the legacy IS_ERR_VALUE() check in print_graph_retval() only inspects bit 31, which will be 0 for these narrow types, negative values (like error codes) will be incorrectly printed as positive integers. > + break; > + default: > + *fmt = RETVAL_FMT_HEX; > + break; > + } > + > + btf_put(btf); > +} > +#endif > diff --git a/kernel/trace/trace_functions_graph.c > b/kernel/trace/trace_functions_graph.c > index ff7cb1a76b953..89a64982df2cc 100644 > --- a/kernel/trace/trace_functions_graph.c > +++ b/kernel/trace/trace_functions_graph.c [ ... ] > @@ -888,17 +889,35 @@ 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; > > - if (!IS_ERR_VALUE(err_code)) > - err_code = 0; > + btf_trim_retval((unsigned long)func, &retval, &print_retval, > &fmt); [Severity: High] Is it safe to call bpf_find_btf_id() from this trace formatting path, considering it might run in atomic/NMI contexts? This trace formatting function can run in NMI context via ftrace_dump_one(), which executes with local IRQs disabled: kernel/trace/trace.c:ftrace_dump_one() ... local_irq_save(flags); ... btf_trim_retval() eventually calls bpf_find_btf_id(), which unconditionally acquires btf_idr_lock using spin_lock_bh(). Taking a bottom-half lock in hard IRQ/NMI context triggers WARN_ON_ONCE(irqs_disabled()) and causes deadlocks if the lock is already held. [Severity: High] Could this global name-based lookup cause severe O(N) performance degradation during trace reads? Trace formatting executes for every function return event. btf_trim_retval() uses lookup_symbol_name() to convert the IP to a string, and then globally searches BTF by name. For module functions, this requires acquiring the global btf_idr_lock and iterating over all modules per event, completely stalling trace formatting and contending with BPF operations. Additionally, it causes incorrect typing for static functions with common names, as it will just return the first match in vmlinux. > + if (print_retval) { > + if (opt_flags & TRACE_GRAPH_PRINT_RETVAL_HEX) > + retval_fmt = RETVAL_FMT_HEX; > + > + if (retval && retval_fmt != RETVAL_FMT_HEX) { > + /* 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 (!IS_ERR_VALUE(err_code)) > + err_code = 0; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
