Messages in this thread | | | From | Jiri Olsa <> | Date | Wed, 17 Aug 2022 11:16:02 +0200 | Subject | Re: [PATCH] tracing/perf: Fix double put of trace event when init fails |
| |
On Tue, Aug 16, 2022 at 07:28:17PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > 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/ > > Reported-by: Krister Johansen <kjlx@templeofstupid.com> > Reviewed-by: Krister Johansen <kjlx@templeofstupid.com> > Tested-by: Krister Johansen <kjlx@templeofstupid.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
LGTM
Acked-by: Jiri Olsa <jolsa@kernel.org>
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 >
| |