Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context from heap

Steven Rostedt
2025-07-18T16:58:29Z

On Fri, 18 Jul 2025 20:34:08 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 854e5668f5ee..7bc4c84464e4 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -10,6 +10,7 @@
> * Author: Srikar Dronamraju
> */
>
> +#include <linux/cleanup.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> #include <linux/smp.h>

Nit, but let's keep the "upside-down x-mas tree" format:

#include <linux/seq_file.h>
#include <linux/cleanup.h>
#include <linux/slab.h>
#include <linux/smp.h>

> @@ -438,6 +439,14 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
> * this MUST be called for clean up the context and return a resource.
> */
> void traceprobe_finish_parse(struct traceprobe_parse_context *ctx);
> +static inline void traceprobe_free_parse_ctx(struct traceprobe_parse_context *ctx)
> +{
> + traceprobe_finish_parse(ctx);
> + kfree(ctx);
> +}
> +
> +DEFINE_FREE(traceprobe_parse_context, struct traceprobe_parse_context *,
> + if (!IS_ERR_OR_NULL(_T)) traceprobe_free_parse_ctx(_T))

ctx will either be allocated or NULL, I think the above could be:

if (_T) traceprobe_free_parse_ctx(_T))

>
> extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
> int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index f95a2c3d5b1b..1fd479718d03 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -537,6 +537,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
> */
> static int __trace_uprobe_create(int argc, const char **argv)
> {
> + struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
> struct trace_uprobe *tu;
> const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> char *arg, *filename, *rctr, *rctr_end, *tmp;
> @@ -693,15 +694,17 @@ static int __trace_uprobe_create(int argc, const char **argv)
> tu->path = path;
> tu->filename = filename;
>
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + ctx->flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER;
> +
> /* parse arguments */
> for (i = 0; i < argc; i++) {
> - struct traceprobe_parse_context ctx = {
> - .flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER,
> - };
> -
> trace_probe_log_set_index(i + 2);
> - ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], &ctx);
> - traceprobe_finish_parse(&ctx);
> + ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], ctx);

Doesn't this change the semantics a bit?

Before this change, traceprobe_finish_parse(&ctx) is called for every
iteration of the loop. Now we only do it when it exits the function.

-- Steve

> if (ret)
> goto error;
> }