On Fri, 18 Jul 2025 13:55:49 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> On Fri, 18 Jul 2025 20:34:40 +0900
> "Masami Hiramatsu (Google)" <mhira...@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhira...@kernel.org>
> >
> > Allocate temporary string buffers for parsing eprobe-events
> > from heap instead of stack.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhira...@kernel.org>
> > ---
> > kernel/trace/trace_eprobe.c | 24 ++++++++++++++++++++----
> > 1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 1e18a8619b40..75d8208cd859 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -9,6 +9,7 @@
> > * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov
> > tz.stoya...@gmail.com>
> > *
> > */
> > +#include <linux/cleanup.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/ftrace.h>
> > @@ -871,10 +872,10 @@ static int __trace_eprobe_create(int argc, const char
> > *argv[])
> > const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
> > const char *sys_event = NULL, *sys_name = NULL;
> > struct trace_event_call *event_call;
> > + char *buf1 __free(kfree) = NULL;
> > + char *buf2 __free(kfree) = NULL;
> > + char *gbuf __free(kfree) = NULL;
> > struct trace_eprobe *ep = NULL;
> > - char buf1[MAX_EVENT_NAME_LEN];
> > - char buf2[MAX_EVENT_NAME_LEN];
> > - char gbuf[MAX_EVENT_NAME_LEN];
> > int ret = 0, filter_idx = 0;
> > int i, filter_cnt;
> >
> > @@ -885,6 +886,11 @@ static int __trace_eprobe_create(int argc, const char
> > *argv[])
> >
> > event = strchr(&argv[0][1], ':');
> > if (event) {
> > + gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > + if (!gbuf) {
> > + ret = -ENOMEM;
> > + goto parse_error;
> > + }
> > event++;
> > ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > event - argv[0]);
> > @@ -894,6 +900,12 @@ static int __trace_eprobe_create(int argc, const char
> > *argv[])
> >
> > trace_probe_log_set_index(1);
> > sys_event = argv[1];
> > +
> > + buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > + if (!buf2) {
> > + ret = -ENOMEM;
> > + goto parse_error;
> > + }
> > ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> > if (ret || !sys_event || !sys_name) {
> > trace_probe_log_err(0, NO_EVENT_INFO);
> > @@ -901,7 +913,11 @@ static int __trace_eprobe_create(int argc, const char
> > *argv[])
> > }
> >
> > if (!event) {
> > - strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
> > + buf1 = kstrdup(sys_event, GFP_KERNEL);
> > + if (!buf1) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
>
> I kinda hate all these updating of "ret" before jumping to error.
Agreed.
>
> > event = buf1;
> > }
> >
>
> What about this:
Looks good to me. Note that eventually I will use cleanup.h to
remove gotos from this function as same as other probes too.
Anyway, in this series I will use the below code.
Thank you!
>
> -- Steve
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 916555f0de81..48cc1079a1dd 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -9,6 +9,7 @@
> * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoya...@gmail.com>
> *
> */
> +#include <linux/cleanup.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/ftrace.h>
> @@ -869,10 +870,10 @@ static int __trace_eprobe_create(int argc, const char
> *argv[])
> const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
> const char *sys_event = NULL, *sys_name = NULL;
> struct trace_event_call *event_call;
> + char *buf1 __free(kfree) = NULL;
> + char *buf2 __free(kfree) = NULL;
> + char *gbuf __free(kfree) = NULL;
> struct trace_eprobe *ep = NULL;
> - char buf1[MAX_EVENT_NAME_LEN];
> - char buf2[MAX_EVENT_NAME_LEN];
> - char gbuf[MAX_EVENT_NAME_LEN];
> int ret = 0, filter_idx = 0;
> int i, filter_cnt;
>
> @@ -883,6 +884,9 @@ static int __trace_eprobe_create(int argc, const char
> *argv[])
>
> event = strchr(&argv[0][1], ':');
> if (event) {
> + gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> + if (!gbuf)
> + goto mem_error;
> event++;
> ret = traceprobe_parse_event_name(&event, &group, gbuf,
> event - argv[0]);
> @@ -892,6 +896,10 @@ static int __trace_eprobe_create(int argc, const char
> *argv[])
>
> trace_probe_log_set_index(1);
> sys_event = argv[1];
> +
> + buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> + if (!buf2)
> + goto mem_error;
> ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> if (ret || !sys_event || !sys_name) {
> trace_probe_log_err(0, NO_EVENT_INFO);
> @@ -899,7 +907,9 @@ static int __trace_eprobe_create(int argc, const char
> *argv[])
> }
>
> if (!event) {
> - strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
> + buf1 = kstrdup(sys_event, GFP_KERNEL);
> + if (!buf1)
> + goto mem_error;
> event = buf1;
> }
>
> @@ -972,6 +982,9 @@ static int __trace_eprobe_create(int argc, const char
> *argv[])
> trace_probe_log_clear();
> return ret;
>
> +mem_error:
> + ret = -ENOMEM;
> + goto error;
> parse_error:
> ret = -EINVAL;
> error:
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
