Re: [PATCH v2] perf/ftrace: Fix WARNING in __unregister_ftrace_function

[Google]

On Fri, 22 May 2026 16:39:41 -0400
Rik van Riel <riel@xxxxxxxxxxx> wrote:

> On Wed, 2026-05-20 at 16:41 -0400, Steven Rostedt wrote:
> > 
> > Rik,
> > 
> > Can you reply to Sashiko here?
> > 
> > The below comes from Sashiko:
> > https://sashiko.dev/?list=org.kernel.vger.linux-trace-kernel#/patchset/20260513161916.04151502%40fangorn
> > 
> > Thanks,
> > 
> > -- Steve
> > 
> > 
> > > commit f90ac36f39b79a19c015522bb1caf7ef903caf02
> > > Author: Rik van Riel <riel@xxxxxxxxxxx>
> > > perf/ftrace: Fix WARNING in __unregister_ftrace_function
> > > This commit attempts to fix a WARN_ON triggered during
> > > perf_event_alloc() error
> > > cleanup by checking FTRACE_OPS_FL_ENABLED before unregistering the
> > > ftrace
> > > function. The warning occurs because the ftrace_ops registration
> > > failed or was
> > > torn down by perf_try_init_event() earlier, but the event->destroy
> > > callback is
> > > executed again during the error path.
> > > > diff --git a/kernel/trace/trace_event_perf.c
> > > > b/kernel/trace/trace_event_perf.c
> > > > index 61e3a2620fa3c..8d64f17135e8f 100644
> > > > --- a/kernel/trace/trace_event_perf.c
> > > > +++ b/kernel/trace/trace_event_perf.c
> > > > @@ -495,7 +495,11 @@ static int
> > > > perf_ftrace_function_register(struct perf_event *event)
> > > >  static int perf_ftrace_function_unregister(struct perf_event
> > > > *event)
> > > >  {
> > > >  	struct ftrace_ops *ops = &event->ftrace_ops;
> > > > -	int ret = unregister_ftrace_function(ops);
> > > > +	int ret = 0;
> > > > +
> > > > +	if (ops->flags & FTRACE_OPS_FL_ENABLED)
> > > > +		ret = unregister_ftrace_function(ops);
> > > > +
> > > >  	ftrace_free_filter(ops);
> > 
> > > Does calling ftrace_free_filter() unconditionally here mask a
> > > double-teardown
> > > regression while leaving the underlying double-free active?
> 
> I don't see how calling ftrace_free_filter() twice would
> call issues, given that it sets the ->*_hash values to
> EMPTY_HASH:
> 
> void ftrace_free_filter(struct ftrace_ops *ops)
> {
>         ftrace_ops_init(ops);
>         if (WARN_ON(ops->flags & FTRACE_OPS_FL_ENABLED))
>                 return;
>         free_ftrace_hash(ops->func_hash->filter_hash);
>         free_ftrace_hash(ops->func_hash->notrace_hash);
>         ops->func_hash->filter_hash = EMPTY_HASH;
>         ops->func_hash->notrace_hash = EMPTY_HASH;
> }
> 
> void free_ftrace_hash(struct ftrace_hash *hash)
> {
>         if (!hash || hash == EMPTY_HASH)
>                 return;
> ..
> 

Yeah, confirmed.

> 
> > > In perf_try_init_event(), if a PMU event_init() succeeds but a
> > > subsequent
> > > capability check fails, it explicitly calls event->destroy(event)
> > > to roll back:
> > > kernel/events/core.c:perf_try_init_event() {
> > >     ...
> > > 		if (ret && event->destroy)
> > > 			event->destroy(event);
> > >     ...
> > > }
> 
> The error handling there all seems to "goto err_destroy"
> 
> err_destroy:
>         if (event->destroy) {
>                 event->destroy(event);
>                 event->destroy = NULL;
>         }
> 
> 
> > > However, it does not set event->destroy to NULL.
> 
> ... but it does?
> 
> I am not sure what code Sashiko is looking at,
> but it does not look like the code I just pulled.

Indeed.

> 
> Is there a different tree I should be looking at
> than upstream Linus?

You can see the baseline info if you expand the collapsed triangle.
Anyway, it said:

linux-trace/HEAD (70575e77839f4c5337ce2653b39b86bb365a870e)

So that is linux-trace/master.

commit 70575e77839f4c5337ce2653b39b86bb365a870e (linux-trace/master)
Merge: 7bc6e90d7aa4 a43ae8057cc1
Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date:   Fri Sep 30 09:41:34 2022 -0700

    Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost


Hmm, this is too old. And linux-trace/master is not used anymore.

Reported to Sashiko.

https://github.com/sashiko-dev/sashiko/issues/218

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>