linux-kernel - Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250718125820.0d0ae198@batman.local.home>
Date: Fri, 18 Jul 2025 12:58:20 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context
 from heap

On Fri, 18 Jul 2025 20:34:08 +0900
"Masami Hiramatsu (Google)" <mhiramat@...nel.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;
>  	}