| Message ID | 0d7970e8702b7d15293a844cb59ddd352c298f26.1661651934.git.kjlx@templeofstupid.com |
|---|---|
| State | New |
| Headers | show |
| Series | trace events WARN | expand |
On 28.08.22 04:02, Krister Johansen wrote: BugLink: https://bugs.launchpad.net/bugs/1987232 > commit 7249921d94ff64f67b733eca0b68853a62032b3d upstream. > > 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/cover.1660347763.git.kjlx@templeofstupid.com/ > Link: https://lkml.kernel.org/r/20220816192817.43d5e17f@gandalf.local.home > > Cc: stable@vger.kernel.org > Fixes: 1d18538e6a092 ("tracing: Have dynamic events have a ref counter") > Reported-by: Krister Johansen <kjlx@templeofstupid.com> > Reviewed-by: Krister Johansen <kjlx@templeofstupid.com> > Tested-by: Krister Johansen <kjlx@templeofstupid.com> > Acked-by: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> (cherry picked from commit 0d7970e8702b7d15293a844cb59ddd352c298f26 linux-5.15.y) > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Hi Krister, since a new cycle just stated and we are still about 10 releases of upstream stable behind I would guess this would probably still be a rather long time until it hits the normal way. So picking it up before sounds reasonable. Things need a little addition for process but that is not a big problem. -Stefan > 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 fba8cb77a73a..083f648e3265 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); > }
On Mon, Aug 29, 2022 at 09:35:05AM +0200, Stefan Bader wrote: > On 28.08.22 04:02, Krister Johansen wrote: > > BugLink: https://bugs.launchpad.net/bugs/1987232 > > > commit 7249921d94ff64f67b733eca0b68853a62032b3d upstream. > > > > 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/cover.1660347763.git.kjlx@templeofstupid.com/ > > Link: https://lkml.kernel.org/r/20220816192817.43d5e17f@gandalf.local.home > > > > Cc: stable@vger.kernel.org > > Fixes: 1d18538e6a092 ("tracing: Have dynamic events have a ref counter") > > Reported-by: Krister Johansen <kjlx@templeofstupid.com> > > Reviewed-by: Krister Johansen <kjlx@templeofstupid.com> > > Tested-by: Krister Johansen <kjlx@templeofstupid.com> > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > (cherry picked from commit 0d7970e8702b7d15293a844cb59ddd352c298f26 linux-5.15.y) > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > --- > > Hi Krister, > > since a new cycle just stated and we are still about 10 releases of upstream > stable behind I would guess this would probably still be a rather long time > until it hits the normal way. So picking it up before sounds reasonable. > Things need a little addition for process but that is not a big problem. Hi Stefan, I appreciate your willingness to pick this up ahead of 5.15.63 making it into the SRU cycle. Thanks again! -K
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c index fba8cb77a73a..083f648e3265 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); }
commit 7249921d94ff64f67b733eca0b68853a62032b3d upstream. 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/cover.1660347763.git.kjlx@templeofstupid.com/ Link: https://lkml.kernel.org/r/20220816192817.43d5e17f@gandalf.local.home Cc: stable@vger.kernel.org Fixes: 1d18538e6a092 ("tracing: Have dynamic events have a ref counter") Reported-by: Krister Johansen <kjlx@templeofstupid.com> Reviewed-by: Krister Johansen <kjlx@templeofstupid.com> Tested-by: Krister Johansen <kjlx@templeofstupid.com> Acked-by: Jiri Olsa <jolsa@kernel.org> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> --- kernel/trace/trace_event_perf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)