On Mon, 1 Jun 2026 12:21:26 -0400 Steven Rostedt <[email protected]> wrote:
> On Sun, 31 May 2026 10:14:58 +0900 > Masami Hiramatsu (Google) <[email protected]> wrote: > > > > > Does this prematurely release the BTF struct reference? > > > > If TPARG_FL_TYPECAST is unset here and ctx->struct_btf is put, won't > > > > later steps in traceprobe_parse_probe_arg_body() (like > > > > find_fetch_type_from_btf_type()) fail to properly infer struct field > > > > sizes? > > > > When ctx_btf(ctx) is called later without TPARG_FL_TYPECAST set, it > > > > will evaluate to ctx->btf (which is NULL for eprobes). > > > > Could this potentially lead to silent defaults, such as 64-bit reads for > > > > smaller fields, or fail to inject pointer dereferences for string > > > > fields, > > > > while also leaving ctx->last_type pointing to a prematurely released BTF > > > > object? > > > > > > Does this mean we need to set ctx->last_type to NULL here too? > > > > No, since the member we refer can be different from unsigned long. > > When we don't have ":type" suffix, we use BTF type information to > > decide appropriate type. > > > > > > > > Because everything above is pretty much the expected behavior. The put is > > > *not* premature. The last_struct and struct_btf are both set to NULL. I > > > guess the only thing missing is to reset last_type as well. > > > > No, as I explained, the last_type is used to determine the member type > > when user does not specify the ":type" suffix. > > > > So, what we need to do is deferring the btf_put(struct_btf) as below: > > (no build test yet.) > > OK, but I don't think we want the struct_btf to exist beyond a single > arg like the btf descriptor does. How about this (on top of this change), > where it clears the struct_btf at the end of > traceprobe_parse_probe_arg_body()? > > Also, I see the flag as being redundant and use the existence of > struct_btf to denote that it's parsing a typedef struct. Ah, indeed. OK, let me check v8 patch. Thanks! > > -- Steve > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > index 9246e9c3d066..56b7dc406ca1 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -397,8 +397,7 @@ static bool btf_type_is_char_array(struct btf *btf, const > struct btf_type *type) > > static struct btf *ctx_btf(struct traceprobe_parse_context *ctx) > { > - return ctx->flags & TPARG_FL_TYPECAST ? > - ctx->struct_btf : ctx->btf; > + return ctx->struct_btf ? : ctx->btf; > } > > static int check_prepare_btf_string_fetch(char *typename, > @@ -531,6 +530,15 @@ static int query_btf_context(struct > traceprobe_parse_context *ctx) > return 0; > } > > +static void clear_struct_btf(struct traceprobe_parse_context *ctx) > +{ > + if (ctx->struct_btf) { > + btf_put(ctx->struct_btf); > + ctx->struct_btf = NULL; > + ctx->last_struct = NULL; > + } > +} > + > static void clear_btf_context(struct traceprobe_parse_context *ctx) > { > if (ctx->btf) { > @@ -579,7 +587,7 @@ static int parse_btf_field(char *fieldname, const struct > btf_type *type, > struct fetch_insn *code = *pcode; > const struct btf_member *field; > u32 bitoffs, anon_offs; > - bool is_struct = ctx->flags & TPARG_FL_TYPECAST; > + bool is_struct = ctx->struct_btf != NULL; > struct btf *btf = ctx_btf(ctx); > char *next; > int is_ptr; > @@ -690,7 +698,7 @@ static int parse_btf_arg(char *varname, > ret = parse_trace_event(varname, code, ctx); > if (ret < 0) > return ret; > - if (WARN_ON_ONCE(!(ctx->flags & TPARG_FL_TYPECAST))) > + if (WARN_ON_ONCE(ctx->struct_btf == NULL)) > return -EINVAL; > type = ctx->last_struct; > goto found_type; > @@ -804,21 +812,19 @@ static int parse_btf_bitfield(struct fetch_insn **pcode, > > static int query_btf_struct(const char *sname, struct > traceprobe_parse_context *ctx) > { > + struct btf *btf = NULL; > 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); > - if (id < 0) > - return id; > + /* Could be a for a structure in a different module */ > + if (ctx->struct_btf) { > + btf_put(ctx->struct_btf); > + ctx->struct_btf = NULL; > } > > + id = bpf_find_btf_id(sname, BTF_KIND_STRUCT, &btf); > + if (id < 0) > + return id; > + ctx->struct_btf = btf; > ctx->last_struct = btf_type_by_id(ctx->struct_btf, id); > return 0; > } > @@ -848,25 +854,23 @@ static int handle_typecast(char *arg, struct fetch_insn > **pcode, > > if (ret < 0) { > trace_probe_log_err(ctx->offset + 1, NO_PTR_STRCT); > - ret = -EINVAL; > - goto out_put; > + return -EINVAL; > } > > - ctx->flags |= TPARG_FL_TYPECAST; > tmp++; > > ctx->offset += tmp - arg; > ret = parse_btf_arg(tmp, pcode, end, ctx); > - ctx->flags &= ~TPARG_FL_TYPECAST; > - ctx->last_struct = NULL; > -out_put: > - btf_put(ctx->struct_btf); > - ctx->struct_btf = NULL; > return ret; > } > > #else /* !CONFIG_PROBE_EVENTS_BTF_ARGS */ > > +static void clear_struct_btf(struct traceprobe_parse_context *ctx) > +{ > + ctx->struct_btf = NULL; > +} > + > static void clear_btf_context(struct traceprobe_parse_context *ctx) > { > ctx->btf = NULL; > @@ -1673,6 +1677,9 @@ static int traceprobe_parse_probe_arg_body(const char > *argv, ssize_t *size, > } > kfree(tmp); > > + /* struct_btf should not be passed to other arguments */ > + clear_struct_btf(ctx); > + > return ret; > } > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > index 952e3d7582b8..83565f1634db 100644 > --- a/kernel/trace/trace_probe.h > +++ b/kernel/trace/trace_probe.h > @@ -394,7 +394,6 @@ static inline int traceprobe_get_entry_data_size(struct > trace_probe *tp) > * TPARG_FL_KERNEL and TPARG_FL_USER are also mutually exclusive. > * TPARG_FL_FPROBE and TPARG_FL_TPOINT are optional but it should be with > * TPARG_FL_KERNEL. > - * TPARG_FL_TYPECAST is set if an argument was typecast to a structure. > */ > #define TPARG_FL_RETURN BIT(0) > #define TPARG_FL_KERNEL BIT(1) > @@ -403,7 +402,6 @@ static inline int traceprobe_get_entry_data_size(struct > trace_probe *tp) > #define TPARG_FL_USER BIT(4) > #define TPARG_FL_FPROBE BIT(5) > #define TPARG_FL_TPOINT BIT(6) > -#define TPARG_FL_TYPECAST BIT(7) > #define TPARG_FL_LOC_MASK GENMASK(4, 0) > > static inline bool tparg_is_function_entry(unsigned int flags) -- Masami Hiramatsu (Google) <[email protected]>
