1. Patchew
  2. linux
  3. [PATCH v2 0/6] tracing/kprobes: Cleanup with guard and __free
  4. View patch

[PATCH v2 5/6] tracing: Use __free() for kprobe events to cleanup

Masami Hiramatsu (Google) posted 6 patches 1 year, 1 month ago
[PATCH v2 5/6] tracing: Use __free() for kprobe events to cleanup
Posted by Masami Hiramatsu (Google) 1 year, 1 month ago 
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Use __free() in trace_kprobe.c to cleanup code.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Instead of using no_free_ptr(), just assign NULL to the registered pointer.
---
 kernel/trace/trace_kprobe.c |   57 ++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 4c3e316454a0..e1ae65f57bf9 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt)	"trace_kprobe: " fmt
 
 #include <linux/bpf-cgroup.h>
+#include <linux/cleanup.h>
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
@@ -257,6 +258,8 @@ static void free_trace_kprobe(struct trace_kprobe *tk)
 	}
 }
 
+DEFINE_FREE(trace_kprobe, struct trace_kprobe *, free_trace_kprobe(_T))
+
 /*
  * Allocate new trace_probe and initialize it (including kprobes).
  */
@@ -268,7 +271,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 					     int maxactive,
 					     int nargs, bool is_return)
 {
-	struct trace_kprobe *tk;
+	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
 	int ret = -ENOMEM;
 
 	tk = kzalloc(struct_size(tk, tp.args, nargs), GFP_KERNEL);
@@ -277,12 +280,12 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 
 	tk->nhit = alloc_percpu(unsigned long);
 	if (!tk->nhit)
-		goto error;
+		return ERR_PTR(ret);
 
 	if (symbol) {
 		tk->symbol = kstrdup(symbol, GFP_KERNEL);
 		if (!tk->symbol)
-			goto error;
+			return ERR_PTR(ret);
 		tk->rp.kp.symbol_name = tk->symbol;
 		tk->rp.kp.offset = offs;
 	} else
@@ -299,13 +302,10 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 
 	ret = trace_probe_init(&tk->tp, event, group, false, nargs);
 	if (ret < 0)
-		goto error;
+		return ERR_PTR(ret);
 
 	dyn_event_init(&tk->devent, &trace_kprobe_ops);
-	return tk;
-error:
-	free_trace_kprobe(tk);
-	return ERR_PTR(ret);
+	return_ptr(tk);
 }
 
 static struct trace_kprobe *find_trace_kprobe(const char *event,
@@ -861,11 +861,12 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	 * Type of args:
 	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
 	 */
-	struct trace_kprobe *tk = NULL;
+	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
 	int i, len, new_argc = 0, ret = 0;
 	bool is_return = false;
-	char *symbol = NULL, *tmp = NULL;
-	const char **new_argv = NULL;
+	char *symbol __free(kfree) = NULL;
+	char *tmp = NULL;
+	const char **new_argv __free(kfree) = NULL;
 	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
 	enum probe_print_type ptype;
 	int maxactive = 0;
@@ -874,7 +875,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	char buf[MAX_EVENT_NAME_LEN];
 	char gbuf[MAX_EVENT_NAME_LEN];
 	char abuf[MAX_BTF_ARGS_LEN];
-	char *dbuf = NULL;
+	char *dbuf __free(kfree) = NULL;
 	struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL };
 
 	switch (argv[0][0]) {
@@ -931,7 +932,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		/* Check whether uprobe event specified */
 		if (strchr(argv[1], '/') && strchr(argv[1], ':')) {
 			ret = -ECANCELED;
-			goto error;
+			goto out;
 		}
 		/* a symbol specified */
 		symbol = kstrdup(argv[1], GFP_KERNEL);
@@ -1035,7 +1036,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		ctx.offset = 0;
 		ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], &ctx);
 		if (ret)
-			goto error;	/* This can be -ENOMEM */
+			goto out;	/* This can be -ENOMEM */
 	}
 	/* entry handler for kretprobe */
 	if (is_return && tk->tp.entry_arg) {
@@ -1046,7 +1047,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	ptype = is_return ? PROBE_PRINT_RETURN : PROBE_PRINT_NORMAL;
 	ret = traceprobe_set_print_fmt(&tk->tp, ptype);
 	if (ret < 0)
-		goto error;
+		goto out;
 
 	ret = register_trace_kprobe(tk);
 	if (ret) {
@@ -1057,21 +1058,20 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 			trace_probe_log_err(0, BAD_PROBE_ADDR);
 		else if (ret != -ENOMEM && ret != -EEXIST)
 			trace_probe_log_err(0, FAIL_REG_PROBE);
-		goto error;
-	}
+	} else
+		/*
+		 * Here, 'tk' has been registered to the list successfully,
+		 * so we don't need to free it.
+		 */
+		tk = NULL;
 
 out:
 	traceprobe_finish_parse(&ctx);
 	trace_probe_log_clear();
-	kfree(new_argv);
-	kfree(symbol);
-	kfree(dbuf);
 	return ret;
 
 parse_error:
 	ret = -EINVAL;
-error:
-	free_trace_kprobe(tk);
 	goto out;
 }
 
@@ -1893,7 +1893,7 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 			  bool is_return)
 {
 	enum probe_print_type ptype;
-	struct trace_kprobe *tk;
+	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
 	int ret;
 	char *event;
 
@@ -1924,19 +1924,14 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 
 	ptype = trace_kprobe_is_return(tk) ?
 		PROBE_PRINT_RETURN : PROBE_PRINT_NORMAL;
-	if (traceprobe_set_print_fmt(&tk->tp, ptype) < 0) {
-		ret = -ENOMEM;
-		goto error;
-	}
+	if (traceprobe_set_print_fmt(&tk->tp, ptype) < 0)
+		return ERR_PTR(-ENOMEM);
 
 	ret = __register_trace_kprobe(tk);
 	if (ret < 0)
-		goto error;
+		return ERR_PTR(ret);
 
 	return trace_probe_event_call(&tk->tp);
-error:
-	free_trace_kprobe(tk);
-	return ERR_PTR(ret);
 }
 
 void destroy_local_trace_kprobe(struct trace_event_call *event_call)
Re: [PATCH v2 5/6] tracing: Use __free() for kprobe events to cleanup
Posted by Masami Hiramatsu (Google) 1 year, 1 month ago 
On Sun,  5 Jan 2025 21:48:29 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Use __free() in trace_kprobe.c to cleanup code.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v2:
>   - Instead of using no_free_ptr(), just assign NULL to the registered pointer.
> ---
>  kernel/trace/trace_kprobe.c |   57 ++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 4c3e316454a0..e1ae65f57bf9 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -8,6 +8,7 @@
>  #define pr_fmt(fmt)	"trace_kprobe: " fmt
>  
>  #include <linux/bpf-cgroup.h>
> +#include <linux/cleanup.h>
>  #include <linux/security.h>
>  #include <linux/module.h>
>  #include <linux/uaccess.h>
> @@ -257,6 +258,8 @@ static void free_trace_kprobe(struct trace_kprobe *tk)
>  	}
>  }
>  
> +DEFINE_FREE(trace_kprobe, struct trace_kprobe *, free_trace_kprobe(_T))

Oops, this also need to check !IS_ERR_OR_NULL(_T) since free_trace_kprobe()
only checks the pointer != NULL.

Thanks,

> +
>  /*
>   * Allocate new trace_probe and initialize it (including kprobes).
>   */
> @@ -268,7 +271,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
>  					     int maxactive,
>  					     int nargs, bool is_return)
>  {
> -	struct trace_kprobe *tk;
> +	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
>  	int ret = -ENOMEM;
>  
>  	tk = kzalloc(struct_size(tk, tp.args, nargs), GFP_KERNEL);
> @@ -277,12 +280,12 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
>  
>  	tk->nhit = alloc_percpu(unsigned long);
>  	if (!tk->nhit)
> -		goto error;
> +		return ERR_PTR(ret);
>  
>  	if (symbol) {
>  		tk->symbol = kstrdup(symbol, GFP_KERNEL);
>  		if (!tk->symbol)
> -			goto error;
> +			return ERR_PTR(ret);
>  		tk->rp.kp.symbol_name = tk->symbol;
>  		tk->rp.kp.offset = offs;
>  	} else
> @@ -299,13 +302,10 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
>  
>  	ret = trace_probe_init(&tk->tp, event, group, false, nargs);
>  	if (ret < 0)
> -		goto error;
> +		return ERR_PTR(ret);
>  
>  	dyn_event_init(&tk->devent, &trace_kprobe_ops);
> -	return tk;
> -error:
> -	free_trace_kprobe(tk);
> -	return ERR_PTR(ret);
> +	return_ptr(tk);
>  }
>  
>  static struct trace_kprobe *find_trace_kprobe(const char *event,
> @@ -861,11 +861,12 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	 * Type of args:
>  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
>  	 */
> -	struct trace_kprobe *tk = NULL;
> +	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
>  	int i, len, new_argc = 0, ret = 0;
>  	bool is_return = false;
> -	char *symbol = NULL, *tmp = NULL;
> -	const char **new_argv = NULL;
> +	char *symbol __free(kfree) = NULL;
> +	char *tmp = NULL;
> +	const char **new_argv __free(kfree) = NULL;
>  	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
>  	enum probe_print_type ptype;
>  	int maxactive = 0;
> @@ -874,7 +875,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	char buf[MAX_EVENT_NAME_LEN];
>  	char gbuf[MAX_EVENT_NAME_LEN];
>  	char abuf[MAX_BTF_ARGS_LEN];
> -	char *dbuf = NULL;
> +	char *dbuf __free(kfree) = NULL;
>  	struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL };
>  
>  	switch (argv[0][0]) {
> @@ -931,7 +932,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  		/* Check whether uprobe event specified */
>  		if (strchr(argv[1], '/') && strchr(argv[1], ':')) {
>  			ret = -ECANCELED;
> -			goto error;
> +			goto out;
>  		}
>  		/* a symbol specified */
>  		symbol = kstrdup(argv[1], GFP_KERNEL);
> @@ -1035,7 +1036,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  		ctx.offset = 0;
>  		ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], &ctx);
>  		if (ret)
> -			goto error;	/* This can be -ENOMEM */
> +			goto out;	/* This can be -ENOMEM */
>  	}
>  	/* entry handler for kretprobe */
>  	if (is_return && tk->tp.entry_arg) {
> @@ -1046,7 +1047,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	ptype = is_return ? PROBE_PRINT_RETURN : PROBE_PRINT_NORMAL;
>  	ret = traceprobe_set_print_fmt(&tk->tp, ptype);
>  	if (ret < 0)
> -		goto error;
> +		goto out;
>  
>  	ret = register_trace_kprobe(tk);
>  	if (ret) {
> @@ -1057,21 +1058,20 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  			trace_probe_log_err(0, BAD_PROBE_ADDR);
>  		else if (ret != -ENOMEM && ret != -EEXIST)
>  			trace_probe_log_err(0, FAIL_REG_PROBE);
> -		goto error;
> -	}
> +	} else
> +		/*
> +		 * Here, 'tk' has been registered to the list successfully,
> +		 * so we don't need to free it.
> +		 */
> +		tk = NULL;
>  
>  out:
>  	traceprobe_finish_parse(&ctx);
>  	trace_probe_log_clear();
> -	kfree(new_argv);
> -	kfree(symbol);
> -	kfree(dbuf);
>  	return ret;
>  
>  parse_error:
>  	ret = -EINVAL;
> -error:
> -	free_trace_kprobe(tk);
>  	goto out;
>  }
>  
> @@ -1893,7 +1893,7 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
>  			  bool is_return)
>  {
>  	enum probe_print_type ptype;
> -	struct trace_kprobe *tk;
> +	struct trace_kprobe *tk __free(trace_kprobe) = NULL;
>  	int ret;
>  	char *event;
>  
> @@ -1924,19 +1924,14 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
>  
>  	ptype = trace_kprobe_is_return(tk) ?
>  		PROBE_PRINT_RETURN : PROBE_PRINT_NORMAL;
> -	if (traceprobe_set_print_fmt(&tk->tp, ptype) < 0) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> +	if (traceprobe_set_print_fmt(&tk->tp, ptype) < 0)
> +		return ERR_PTR(-ENOMEM);
>  
>  	ret = __register_trace_kprobe(tk);
>  	if (ret < 0)
> -		goto error;
> +		return ERR_PTR(ret);
>  
>  	return trace_probe_event_call(&tk->tp);
> -error:
> -	free_trace_kprobe(tk);
> -	return ERR_PTR(ret);
>  }
>  
>  void destroy_local_trace_kprobe(struct trace_event_call *event_call)
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.