Re: [PATCH 1/1] tracing: fix a WARN from trace_event_dyn_put_ref

Steven Rostedt
2022-08-16T16:25:58Z

[ 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;
> }
>