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

[Rik van Riel <riel@surriel.com>]

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@surriel.com>
> > 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;
..


> > 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.

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


-- 
All Rights Reversed.