Messages in this thread | | | Date | Tue, 16 Aug 2022 12:25:59 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 1/1] tracing: fix a WARN from trace_event_dyn_put_ref |
| |
[ Added the authors of this code to the Cc ]
On Fri, 12 Aug 2022 17:02:20 -0700 Krister Johansen <kjlx@templeofstupid.com> wrote:
> The code in perf_trace_init takes a reference on a trace_event_call that is > looked up as part of the function call. If perf_trace_event_int fails, > however, perf_trace_event_unreg can decrement that refcount from underneath > perf_trace_init. This means that in some failure cases, perf_trace_init > can trigger the WARN in trace_dynevent.c which attempts to guard against > zero reference counts going negative. > > The author can reproduce this problem readily by running perf record in a > loop against a series of uprobes with no other users. Killing the record > process before it can finish its setup is enough to trigger this warn > within a few seconds. > > This patch leaves the behavior in perf_trace_event_unreg unchanged, but > moves most of the code in that function to perf_trace_event_cleanup. The > unreg function retains the ability to drop the refcount on the tp_event, > but cleanup does not. This modification is based upon the observation that > all of the other callers of perf_trace_event_init don't bother with > manipulating a reference count on the tp_events that they create. For > those callers, the trace_event_put_ref was already a no-op. > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > Reviewed-by: David Reaver <me@davidreaver.com> > Fixes: 1d18538e6a092 "tracing: Have dynamic events have a ref counter" > CC: stable@vger.kernel.org # 5.15, 5.18, 5.19 > --- > kernel/trace/trace_event_perf.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c > index a114549720d6..7762bfd268cd 100644 > --- a/kernel/trace/trace_event_perf.c > +++ b/kernel/trace/trace_event_perf.c > @@ -151,13 +151,13 @@ static int perf_trace_event_reg(struct trace_event_call *tp_event, > return ret; > } > > -static void perf_trace_event_unreg(struct perf_event *p_event) > +static void perf_trace_event_cleanup(struct perf_event *p_event) > { > struct trace_event_call *tp_event = p_event->tp_event; > int i; > > if (--tp_event->perf_refcount > 0) > - goto out; > + return; > > tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL); > > @@ -176,7 +176,13 @@ static void perf_trace_event_unreg(struct perf_event *p_event) > perf_trace_buf[i] = NULL; > } > } > -out: > +} > + > +static void perf_trace_event_unreg(struct perf_event *p_event) > +{ > + struct trace_event_call *tp_event = p_event->tp_event; > + > + perf_trace_event_cleanup(p_event); > trace_event_put_ref(tp_event); > } > > @@ -207,7 +213,7 @@ static int perf_trace_event_init(struct trace_event_call *tp_event, > > ret = perf_trace_event_open(p_event); > if (ret) { > - perf_trace_event_unreg(p_event); > + perf_trace_event_cleanup(p_event);
The only problem I have with this patch is the loss of symmetry. Where perf_trace_event_reg() returns successful, so unreg() should be the function you call on failure.
Since perf_trace_event_reg() is only called from perf_trace_event_init() let's move the perf_trace_event_open() into the perf_trace_event_reg() and have the unreg still do the clean up on failure.
This way we keep the symmetry between *_reg() and *_unreg().
And then the init function will not have to do any clean up, and can just end with:
return perf_trace_event_reg(tp_event, p_event);
Thanks,
-- Steve
> return ret; > } >
| |