On Tue, 19 May 2026 14:17:26 -0400 Steven Rostedt <[email protected]> wrote:
> On Tue, 19 May 2026 17:48:47 +0000 > [email protected] wrote: > > > Thank you for your contribution! Sashiko AI review found 5 potential > > issue(s) to consider: > > - [High] Using `(STRUCT)` typecasts for non-eprobes leaves the base fetch > > instruction uninitialized (`FETCH_OP_NOP`), silently breaking the probe. > > - [High] Prematurely clearing `TPARG_FL_TYPECAST` causes cross-BTF type > > confusion and breaks string fetches. > > - [Medium] Erroneous caching of `struct_btf` restricts a single trace probe > > to using typecasts from only one BTF object, preventing cross-module > > lookups. > > - [Medium] Cross-BTF string table access in `parse_btf_arg()` causes > > invalid string comparisons. > > - [Low] `clear_btf_context()` fails to clear `ctx->struct_btf` after > > dropping its reference. > > -- > > > > commit de4b6287a7069fde24b0e0f679a545a06fb9ae6c > > Author: Steven Rostedt <[email protected]> > > > > tracing/eprobes: Allow use of BTF names to dereference pointers > > > > Add syntax to the parsing of eprobes to be able to typecast a trace event > > field that is a pointer to a structure. > > > > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > > > index e1c73065dae51..2bc9f4ca8aa7e 100644 > > > --- a/kernel/trace/trace_probe.c > > > +++ b/kernel/trace/trace_probe.c > > > @@ -464,6 +470,27 @@ static const char *fetch_type_from_btf_type(struct > > > btf *btf, > > > return NULL; > > > } > > > > > > +static int query_btf_struct(const char *sname, struct > > > traceprobe_parse_context *ctx) > > > +{ > > > + int id; > > > + > > > + if (!ctx->struct_btf) { > > > + struct btf *btf; > > > + > > > + id = bpf_find_btf_id(sname, BTF_KIND_STRUCT, &btf); > > > + if (id < 0) > > > + return id; > > > + ctx->struct_btf = btf; > > > + } else { > > > + id = btf_find_by_name_kind(ctx->struct_btf, sname, > > > BTF_KIND_STRUCT); > > > > [Severity: Medium] > > Does caching struct_btf here restrict a single trace probe event to using > > typecasts from only one BTF object? > > > > If a probe attempts to typecast structures from two different kernel > > modules, wouldn't the lookup for the second module's struct fail because > > it only searches the cached BTF object of the first module (or vmlinux)? > > Hmm, good point. I guess I should clear the struct_btf at the end of > the '(' switch case. Yes, it should be reset before parsing next argument. > > > > > > + if (id < 0) > > > + return id; > > > + } > > > + > > > + ctx->last_struct = btf_type_by_id(ctx->struct_btf, id); > > > + return 0; > > > +} > > > > [ ... ] > > > > > @@ -515,6 +542,10 @@ static void clear_btf_context(struct > > > traceprobe_parse_context *ctx) > > > ctx->params = NULL; > > > ctx->nr_params = 0; > > > } > > > + if (ctx->struct_btf) { > > > + btf_put(ctx->struct_btf); > > > + ctx->last_struct = NULL; > > > > [Severity: Low] > > Should ctx->struct_btf be explicitly set to NULL after btf_put() drops > > the reference? > > I'm thinking of dropping it in the '(' switch case. Can you consider making the '(' switch case part as a helper function because it depends on CONFIG_DEBUG_INFO_BTF? Thanks, -- Masami Hiramatsu (Google) <[email protected]>
