From: "Steven Rostedt (Google)" <[email protected]>
If in perf_trace_event_init(), the perf_trace_event_open() fails, then it
will call perf_trace_event_unreg() which will not only unregister the perf
trace event, but will also call the put() function of the tp_event.
The problem here is that the trace_event_try_get_ref() is called by the
caller of perf_trace_event_init() and if perf_trace_event_init() returns a
failure, it will then call trace_event_put(). But since the
perf_trace_event_unreg() already called the trace_event_put() function, it
triggers a WARN_ON().
WARNING: CPU: 1 PID: 30309 at kernel/trace/trace_dynevent.c:46 trace_event_dyn_put_ref+0x15/0x20
If perf_trace_event_reg() does not call the trace_event_try_get_ref() then
the perf_trace_event_unreg() should not be calling trace_event_put(). This
breaks symmetry and causes bugs like these.
Pull out the trace_event_put() from perf_trace_event_unreg() and call it
in the locations that perf_trace_event_unreg() is called. This not only
fixes this bug, but also brings back the proper symmetry of the reg/unreg
vs get/put logic.
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: Krister Johansen <[email protected]>
Reviewed-by: Krister Johansen <[email protected]>
Tested-by: Krister Johansen <[email protected]>
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
kernel/trace/trace_event_perf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index a114549720d6..61e3a2620fa3 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -157,7 +157,7 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
int i;
if (--tp_event->perf_refcount > 0)
- goto out;
+ return;
tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
@@ -176,8 +176,6 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
perf_trace_buf[i] = NULL;
}
}
-out:
- trace_event_put_ref(tp_event);
}
static int perf_trace_event_open(struct perf_event *p_event)
@@ -241,6 +239,7 @@ void perf_trace_destroy(struct perf_event *p_event)
mutex_lock(&event_mutex);
perf_trace_event_close(p_event);
perf_trace_event_unreg(p_event);
+ trace_event_put_ref(p_event->tp_event);
mutex_unlock(&event_mutex);
}
@@ -292,6 +291,7 @@ void perf_kprobe_destroy(struct perf_event *p_event)
mutex_lock(&event_mutex);
perf_trace_event_close(p_event);
perf_trace_event_unreg(p_event);
+ trace_event_put_ref(p_event->tp_event);
mutex_unlock(&event_mutex);
destroy_local_trace_kprobe(p_event->tp_event);
@@ -347,6 +347,7 @@ void perf_uprobe_destroy(struct perf_event *p_event)
mutex_lock(&event_mutex);
perf_trace_event_close(p_event);
perf_trace_event_unreg(p_event);
+ trace_event_put_ref(p_event->tp_event);
mutex_unlock(&event_mutex);
destroy_local_trace_uprobe(p_event->tp_event);
}
--
2.35.1
On Tue, Aug 16, 2022 at 07:28:17PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> If in perf_trace_event_init(), the perf_trace_event_open() fails, then it
> will call perf_trace_event_unreg() which will not only unregister the perf
> trace event, but will also call the put() function of the tp_event.
>
> The problem here is that the trace_event_try_get_ref() is called by the
> caller of perf_trace_event_init() and if perf_trace_event_init() returns a
> failure, it will then call trace_event_put(). But since the
> perf_trace_event_unreg() already called the trace_event_put() function, it
> triggers a WARN_ON().
>
> WARNING: CPU: 1 PID: 30309 at kernel/trace/trace_dynevent.c:46 trace_event_dyn_put_ref+0x15/0x20
>
> If perf_trace_event_reg() does not call the trace_event_try_get_ref() then
> the perf_trace_event_unreg() should not be calling trace_event_put(). This
> breaks symmetry and causes bugs like these.
>
> Pull out the trace_event_put() from perf_trace_event_unreg() and call it
> in the locations that perf_trace_event_unreg() is called. This not only
> fixes this bug, but also brings back the proper symmetry of the reg/unreg
> vs get/put logic.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> Reported-by: Krister Johansen <[email protected]>
> Reviewed-by: Krister Johansen <[email protected]>
> Tested-by: Krister Johansen <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
Thanks again, Steven. Is this one that you would consider tagging for a
backport to stable at the appropriate time? I believe this one showed up
in 5.15, if it's any help.
-K
> ---
> kernel/trace/trace_event_perf.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index a114549720d6..61e3a2620fa3 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -157,7 +157,7 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
> int i;
>
> if (--tp_event->perf_refcount > 0)
> - goto out;
> + return;
>
> tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
>
> @@ -176,8 +176,6 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
> perf_trace_buf[i] = NULL;
> }
> }
> -out:
> - trace_event_put_ref(tp_event);
> }
>
> static int perf_trace_event_open(struct perf_event *p_event)
> @@ -241,6 +239,7 @@ void perf_trace_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
> }
>
> @@ -292,6 +291,7 @@ void perf_kprobe_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
>
> destroy_local_trace_kprobe(p_event->tp_event);
> @@ -347,6 +347,7 @@ void perf_uprobe_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
> destroy_local_trace_uprobe(p_event->tp_event);
> }
> --
> 2.35.1
>
On Tue, Aug 16, 2022 at 07:28:17PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> If in perf_trace_event_init(), the perf_trace_event_open() fails, then it
> will call perf_trace_event_unreg() which will not only unregister the perf
> trace event, but will also call the put() function of the tp_event.
>
> The problem here is that the trace_event_try_get_ref() is called by the
> caller of perf_trace_event_init() and if perf_trace_event_init() returns a
> failure, it will then call trace_event_put(). But since the
> perf_trace_event_unreg() already called the trace_event_put() function, it
> triggers a WARN_ON().
>
> WARNING: CPU: 1 PID: 30309 at kernel/trace/trace_dynevent.c:46 trace_event_dyn_put_ref+0x15/0x20
>
> If perf_trace_event_reg() does not call the trace_event_try_get_ref() then
> the perf_trace_event_unreg() should not be calling trace_event_put(). This
> breaks symmetry and causes bugs like these.
>
> Pull out the trace_event_put() from perf_trace_event_unreg() and call it
> in the locations that perf_trace_event_unreg() is called. This not only
> fixes this bug, but also brings back the proper symmetry of the reg/unreg
> vs get/put logic.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> Reported-by: Krister Johansen <[email protected]>
> Reviewed-by: Krister Johansen <[email protected]>
> Tested-by: Krister Johansen <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
LGTM
Acked-by: Jiri Olsa <[email protected]>
jirka
> ---
> kernel/trace/trace_event_perf.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index a114549720d6..61e3a2620fa3 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -157,7 +157,7 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
> int i;
>
> if (--tp_event->perf_refcount > 0)
> - goto out;
> + return;
>
> tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
>
> @@ -176,8 +176,6 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
> perf_trace_buf[i] = NULL;
> }
> }
> -out:
> - trace_event_put_ref(tp_event);
> }
>
> static int perf_trace_event_open(struct perf_event *p_event)
> @@ -241,6 +239,7 @@ void perf_trace_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
> }
>
> @@ -292,6 +291,7 @@ void perf_kprobe_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
>
> destroy_local_trace_kprobe(p_event->tp_event);
> @@ -347,6 +347,7 @@ void perf_uprobe_destroy(struct perf_event *p_event)
> mutex_lock(&event_mutex);
> perf_trace_event_close(p_event);
> perf_trace_event_unreg(p_event);
> + trace_event_put_ref(p_event->tp_event);
> mutex_unlock(&event_mutex);
> destroy_local_trace_uprobe(p_event->tp_event);
> }
> --
> 2.35.1
>
On Wed, 17 Aug 2022 11:16:02 +0200
Jiri Olsa <[email protected]> wrote:
> LGTM
>
> Acked-by: Jiri Olsa <[email protected]>
Thanks Jiri!
-- Steve
On Tue, 16 Aug 2022 21:46:56 -0700
Krister Johansen <[email protected]> wrote:
> Thanks again, Steven. Is this one that you would consider tagging for a
> backport to stable at the appropriate time? I believe this one showed up
> in 5.15, if it's any help.
So the warning started with this commit:
1d18538e6a092 ("tracing: Have dynamic events have a ref counter")
Which switched the module_put() to the trace_event_put(). I guess the
difference is that module_put() has
ret = atomic_dec_if_positive(&module->refcnt);
Where it could be called more than once after reaching zero and not warn
about it. But the trace_event_put() will warn if you call it after it
reaches zero.
I'll add a stable tag.
-- Steve