| 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
| ||
|
Message-Id: <20250106190947.12f64f10d32290aad7ab0cea@kernel.org>
Date: Mon, 6 Jan 2025 19:09:47 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Steven Rostedt <rostedt@...dmis.org>, Peter Zijlstra
<peterz@...radead.org>, Anil S Keshavamurthy
<anil.s.keshavamurthy@...el.com>, "David S . Miller" <davem@...emloft.net>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Oleg Nesterov
<oleg@...hat.com>, Tzvetomir Stoyanov <tz.stoyanov@...il.com>, Naveen N Rao
<naveen@...nel.org>, Josh Poimboeuf <jpoimboe@...nel.org>, Jason Baron
<jbaron@...mai.com>, Ard Biesheuvel <ardb@...nel.org>,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] tracing: Use __free() for kprobe events to
cleanup
On Sun, 5 Jan 2025 21:48:29 +0900
"Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
>
> Use __free() in trace_kprobe.c to cleanup code.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.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@...nel.org>
Powered by blists - more mailing lists