* [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe
@ 2023-07-03 4:28 Tzvetomir Stoyanov (VMware)
2023-07-03 8:41 ` Masami Hiramatsu
2023-07-06 16:00 ` Steven Rostedt
0 siblings, 2 replies; 3+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2023-07-03 4:28 UTC (permalink / raw)
To: rostedt; +Cc: mhiramat, dan.carpenter, linux-trace-kernel, linux-kernel
The enable_trace_eprobe() function enables all event probes, attached
to given trace probe. If an error occurs in enabling one of the event
probes, all others should be roll backed. There is a bug in that roll
back logic - instead of all event probes, only the failed one is
disabled.
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
Changes since v1: https://lore.kernel.org/all/20230628121811.338655-1-tz.stoyanov@gmail.com/
- Added one-time warning, suggested by Steven Rostedt.
Changed since v2: https://lore.kernel.org/lkml/20230630121627.833560-1-tz.stoyanov@gmail.com/
- Added counter of successfully enabled eprobes, suggested by Steven Rostedt.
kernel/trace/trace_eprobe.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index cb0077ba2b49..a0a704ba27db 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -644,6 +644,7 @@ static int enable_trace_eprobe(struct trace_event_call *call,
struct trace_eprobe *ep;
bool enabled;
int ret = 0;
+ int cnt = 0;
tp = trace_probe_primary_from_call(call);
if (WARN_ON_ONCE(!tp))
@@ -667,12 +668,25 @@ static int enable_trace_eprobe(struct trace_event_call *call,
if (ret)
break;
enabled = true;
+ cnt++;
}
if (ret) {
/* Failed to enable one of them. Roll back all */
- if (enabled)
- disable_eprobe(ep, file->tr);
+ if (enabled) {
+ /*
+ * It's a bug if one failed for something other than memory
+ * not being available but another eprobe succeeded.
+ */
+ WARN_ON_ONCE(ret != -ENOMEM);
+
+ list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
+ ep = container_of(pos, struct trace_eprobe, tp);
+ disable_eprobe(ep, file->tr);
+ if (!--cnt)
+ break;
+ }
+ }
if (file)
trace_probe_remove_file(tp, file);
else
--
2.41.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe
2023-07-03 4:28 [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe Tzvetomir Stoyanov (VMware)
@ 2023-07-03 8:41 ` Masami Hiramatsu
2023-07-06 16:00 ` Steven Rostedt
1 sibling, 0 replies; 3+ messages in thread
From: Masami Hiramatsu @ 2023-07-03 8:41 UTC (permalink / raw)
To: Tzvetomir Stoyanov (VMware)
Cc: rostedt, mhiramat, dan.carpenter, linux-trace-kernel,
linux-kernel
On Mon, 3 Jul 2023 07:28:53 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> The enable_trace_eprobe() function enables all event probes, attached
> to given trace probe. If an error occurs in enabling one of the event
> probes, all others should be roll backed. There is a bug in that roll
> back logic - instead of all event probes, only the failed one is
> disabled.
This looks good to me.
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you,
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
> Changes since v1: https://lore.kernel.org/all/20230628121811.338655-1-tz.stoyanov@gmail.com/
> - Added one-time warning, suggested by Steven Rostedt.
> Changed since v2: https://lore.kernel.org/lkml/20230630121627.833560-1-tz.stoyanov@gmail.com/
> - Added counter of successfully enabled eprobes, suggested by Steven Rostedt.
>
> kernel/trace/trace_eprobe.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index cb0077ba2b49..a0a704ba27db 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -644,6 +644,7 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> struct trace_eprobe *ep;
> bool enabled;
> int ret = 0;
> + int cnt = 0;
>
> tp = trace_probe_primary_from_call(call);
> if (WARN_ON_ONCE(!tp))
> @@ -667,12 +668,25 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> if (ret)
> break;
> enabled = true;
> + cnt++;
> }
>
> if (ret) {
> /* Failed to enable one of them. Roll back all */
> - if (enabled)
> - disable_eprobe(ep, file->tr);
> + if (enabled) {
> + /*
> + * It's a bug if one failed for something other than memory
> + * not being available but another eprobe succeeded.
> + */
> + WARN_ON_ONCE(ret != -ENOMEM);
> +
> + list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> + ep = container_of(pos, struct trace_eprobe, tp);
> + disable_eprobe(ep, file->tr);
> + if (!--cnt)
> + break;
> + }
> + }
> if (file)
> trace_probe_remove_file(tp, file);
> else
> --
> 2.41.0
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe
2023-07-03 4:28 [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe Tzvetomir Stoyanov (VMware)
2023-07-03 8:41 ` Masami Hiramatsu
@ 2023-07-06 16:00 ` Steven Rostedt
1 sibling, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2023-07-06 16:00 UTC (permalink / raw)
To: Tzvetomir Stoyanov (VMware)
Cc: mhiramat, dan.carpenter, linux-trace-kernel, linux-kernel
On Mon, 3 Jul 2023 07:28:53 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> The enable_trace_eprobe() function enables all event probes, attached
> to given trace probe. If an error occurs in enabling one of the event
> probes, all others should be roll backed. There is a bug in that roll
> back logic - instead of all event probes, only the failed one is
> disabled.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Thanks Tzvetomir!
-- Steve
> ---
> Changes since v1: https://lore.kernel.org/all/20230628121811.338655-1-tz.stoyanov@gmail.com/
> - Added one-time warning, suggested by Steven Rostedt.
> Changed since v2: https://lore.kernel.org/lkml/20230630121627.833560-1-tz.stoyanov@gmail.com/
> - Added counter of successfully enabled eprobes, suggested by Steven Rostedt.
>
> kernel/trace/trace_eprobe.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index cb0077ba2b49..a0a704ba27db 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -644,6 +644,7 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> struct trace_eprobe *ep;
> bool enabled;
> int ret = 0;
> + int cnt = 0;
>
> tp = trace_probe_primary_from_call(call);
> if (WARN_ON_ONCE(!tp))
> @@ -667,12 +668,25 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> if (ret)
> break;
> enabled = true;
> + cnt++;
> }
>
> if (ret) {
> /* Failed to enable one of them. Roll back all */
> - if (enabled)
> - disable_eprobe(ep, file->tr);
> + if (enabled) {
> + /*
> + * It's a bug if one failed for something other than memory
> + * not being available but another eprobe succeeded.
> + */
> + WARN_ON_ONCE(ret != -ENOMEM);
> +
> + list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> + ep = container_of(pos, struct trace_eprobe, tp);
> + disable_eprobe(ep, file->tr);
> + if (!--cnt)
> + break;
> + }
> + }
> if (file)
> trace_probe_remove_file(tp, file);
> else
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-06 16:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-03 4:28 [PATCH v3] kernel/trace: Fix cleanup logic of enable_trace_eprobe Tzvetomir Stoyanov (VMware)
2023-07-03 8:41 ` Masami Hiramatsu
2023-07-06 16:00 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).