From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Use __free() in trace_probe to cleanup some gotos.
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
kernel/trace/trace_probe.c | 52 +++++++++++++++-----------------------------
1 file changed, 18 insertions(+), 34 deletions(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 16a5e368e7b7..bf6a7b81ae95 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -1409,7 +1409,7 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
struct traceprobe_parse_context *ctx)
{
struct fetch_insn *code, *tmp = NULL;
- char *type, *arg;
+ char *type, *arg __free(kfree) = NULL;
int ret, len;
len = strlen(argv);
@@ -1426,22 +1426,16 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
return -ENOMEM;
parg->comm = kstrdup(arg, GFP_KERNEL);
- if (!parg->comm) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!parg->comm)
+ return -ENOMEM;
type = parse_probe_arg_type(arg, parg, ctx);
- if (IS_ERR(type)) {
- ret = PTR_ERR(type);
- goto out;
- }
+ if (IS_ERR(type))
+ return PTR_ERR(type);
code = tmp = kcalloc(FETCH_INSN_MAX, sizeof(*code), GFP_KERNEL);
- if (!code) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!code)
+ return -ENOMEM;
code[FETCH_INSN_MAX - 1].op = FETCH_OP_END;
ctx->last_type = NULL;
@@ -1497,8 +1491,6 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
kfree(code->data);
}
kfree(tmp);
-out:
- kfree(arg);
return ret;
}
@@ -1668,7 +1660,7 @@ const char **traceprobe_expand_meta_args(int argc, const char *argv[],
{
const struct btf_param *params = NULL;
int i, j, n, used, ret, args_idx = -1;
- const char **new_argv = NULL;
+ const char **new_argv __free(kfree) = NULL;
ret = argv_has_var_arg(argc, argv, &args_idx, ctx);
if (ret < 0)
@@ -1707,7 +1699,7 @@ const char **traceprobe_expand_meta_args(int argc, const char *argv[],
ret = sprint_nth_btf_arg(n, "", buf + used,
bufsize - used, ctx);
if (ret < 0)
- goto error;
+ return ERR_PTR(ret);
new_argv[j++] = buf + used;
used += ret + 1;
@@ -1721,25 +1713,20 @@ const char **traceprobe_expand_meta_args(int argc, const char *argv[],
n = simple_strtoul(argv[i] + 4, &type, 10);
if (type && !(*type == ':' || *type == '\0')) {
trace_probe_log_err(0, BAD_VAR);
- ret = -ENOENT;
- goto error;
+ return ERR_PTR(-ENOENT);
}
/* Note: $argN starts from $arg1 */
ret = sprint_nth_btf_arg(n - 1, type, buf + used,
bufsize - used, ctx);
if (ret < 0)
- goto error;
+ return ERR_PTR(ret);
new_argv[j++] = buf + used;
used += ret + 1;
} else
new_argv[j++] = argv[i];
}
- return new_argv;
-
-error:
- kfree(new_argv);
- return ERR_PTR(ret);
+ return_ptr(new_argv);
}
/* @buf: *buf must be equal to NULL. Caller must to free *buf */
@@ -1747,14 +1734,14 @@ int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
{
int i, used, ret;
const int bufsize = MAX_DENTRY_ARGS_LEN;
- char *tmpbuf = NULL;
+ char *tmpbuf __free(kfree) = NULL;
if (*buf)
return -EINVAL;
used = 0;
for (i = 0; i < argc; i++) {
- char *tmp;
+ char *tmp __free(kfree) = NULL;
char *equal;
size_t arg_len;
@@ -1769,7 +1756,7 @@ int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
tmp = kstrdup(argv[i], GFP_KERNEL);
if (!tmp)
- goto nomem;
+ return -ENOMEM;
equal = strchr(tmp, '=');
if (equal)
@@ -1790,18 +1777,15 @@ int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
offsetof(struct file, f_path.dentry),
equal ? equal + 1 : tmp);
- kfree(tmp);
+ kfree(no_free_ptr(tmp));
if (ret >= bufsize - used)
- goto nomem;
+ return -ENOMEM;
argv[i] = tmpbuf + used;
used += ret + 1;
}
- *buf = tmpbuf;
+ *buf = no_free_ptr(tmpbuf);
return 0;
-nomem:
- kfree(tmpbuf);
- return -ENOMEM;
}
void traceprobe_finish_parse(struct traceprobe_parse_context *ctx)On Tue, 7 Jan 2025 20:50:25 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > @@ -1790,18 +1777,15 @@ int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf) > offsetof(struct file, f_path.dentry), > equal ? equal + 1 : tmp); > > - kfree(tmp); > + kfree(no_free_ptr(tmp)); I don't get this? You are telling the compiler not to free tmp, because you decided to free it yourself? Why not just remove the kfree() here altogether? -- Steve > if (ret >= bufsize - used) > - goto nomem; > + return -ENOMEM; > argv[i] = tmpbuf + used; > used += ret + 1; > } > > - *buf = tmpbuf; > + *buf = no_free_ptr(tmpbuf); > return 0; > -nomem: > - kfree(tmpbuf); > - return -ENOMEM; > }
On Tue, 7 Jan 2025 10:36:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 7 Jan 2025 20:50:25 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > @@ -1790,18 +1777,15 @@ int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
> > offsetof(struct file, f_path.dentry),
> > equal ? equal + 1 : tmp);
> >
> > - kfree(tmp);
> > + kfree(no_free_ptr(tmp));
>
> I don't get this? You are telling the compiler not to free tmp, because you
> decided to free it yourself? Why not just remove the kfree() here altogether?
In the for-loop block, the __free() work only when we exit the loop, not
each iteration. In each iteration, kstrdup() is assigned to the 'tmp',
so we need to kfree() each time.
Hmm, maybe this is a sign that I should not use __free() for the 'tmp',
or I should call kfree(tmp) right before kstrdup(), like below.
for (i = 0; i < argc; i++) {
char *tmp __free(kfree) = NULL;
...
kfree(tmp);
tmp = kstrdup(argv[i], GFP_KERNEL);
}
Does this make sense?
>
> -- Steve
>
>
> > if (ret >= bufsize - used)
> > - goto nomem;
> > + return -ENOMEM;
> > argv[i] = tmpbuf + used;
> > used += ret + 1;
> > }
> >
> > - *buf = tmpbuf;
> > + *buf = no_free_ptr(tmpbuf);
> > return 0;
> > -nomem:
> > - kfree(tmpbuf);
> > - return -ENOMEM;
> > }
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Wed, 8 Jan 2025 09:38:43 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > I don't get this? You are telling the compiler not to free tmp, because you
> > decided to free it yourself? Why not just remove the kfree() here altogether?
>
> In the for-loop block, the __free() work only when we exit the loop, not
> each iteration. In each iteration, kstrdup() is assigned to the 'tmp',
> so we need to kfree() each time.
Really? It doesn't trigger for each iteration? That's rather unintuitive. :-/
And sounds buggy, as wouldn't that then cause a memory leak?
I would say not to use __free() for tmp at all. Because now it's just
getting confusing.
-- Steve
>
> Hmm, maybe this is a sign that I should not use __free() for the 'tmp',
> or I should call kfree(tmp) right before kstrdup(), like below.
>
> for (i = 0; i < argc; i++) {
> char *tmp __free(kfree) = NULL;
> ...
> kfree(tmp);
> tmp = kstrdup(argv[i], GFP_KERNEL);
> }
>
> Does this make sense?
On Tue, 7 Jan 2025 20:34:32 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 8 Jan 2025 09:38:43 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
> > > I don't get this? You are telling the compiler not to free tmp, because you
> > > decided to free it yourself? Why not just remove the kfree() here altogether?
> >
> > In the for-loop block, the __free() work only when we exit the loop, not
> > each iteration. In each iteration, kstrdup() is assigned to the 'tmp',
> > so we need to kfree() each time.
>
> Really? It doesn't trigger for each iteration? That's rather unintuitive. :-/
> And sounds buggy, as wouldn't that then cause a memory leak?
Ahh, sorry, it was my misunderstood. I made a test code and confirmed that
kfree() is called in each iteration. Previously I checked but I confused the result.
----------
#include <stdio.h>
void count_func(int *p)
{
printf("Scope out: %d\n", *p);
}
int main(void)
{
for (int i = 0; i < 10; i++) {
int j __attribute((cleanup(count_func))) = 0;
j++;
}
return 0;
}
----------
$ ./loop_cleanup
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Let me fix that.
Thanks,
>
> I would say not to use __free() for tmp at all. Because now it's just
> getting confusing.
>
> -- Steve
>
>
> >
> > Hmm, maybe this is a sign that I should not use __free() for the 'tmp',
> > or I should call kfree(tmp) right before kstrdup(), like below.
> >
> > for (i = 0; i < argc; i++) {
> > char *tmp __free(kfree) = NULL;
> > ...
> > kfree(tmp);
> > tmp = kstrdup(argv[i], GFP_KERNEL);
> > }
> >
> > Does this make sense?
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
© 2016 - 2026 Red Hat, Inc.