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

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

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>