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

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

[Rik van Riel]

On Wed, 13 May 2026 12:33:44 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Instead of duplicating code, what about doing:

That is much nicer. Thank you!

---8<---

From 9de86227b917c49315b7b67aac3a83afae8d792d Mon Sep 17 00:00:00 2001
From: Rik van Riel <riel@meta.com>
Date: Sat, 25 Apr 2026 03:33:54 -0700
Subject: [PATCH] perf/ftrace: Fix WARNING in __unregister_ftrace_function

perf_ftrace_function_unregister() unconditionally calls
unregister_ftrace_function() without checking whether the ftrace_ops
was ever successfully registered. This triggers a WARN_ON in
__unregister_ftrace_function() when the ops doesn't have
FTRACE_OPS_FL_ENABLED set.

This can happen during perf_event_alloc() error cleanup when
perf_trace_destroy() is called via __free_event() on an event whose
ftrace_ops registration failed or was already torn down by
perf_try_init_event()'s err_destroy path.

The call path is:
  perf_event_alloc() error cleanup
    -> __free_event()
      -> event->destroy() [tp_perf_event_destroy]
        -> perf_trace_destroy()
          -> perf_trace_event_close()
            -> TRACE_REG_PERF_CLOSE
              -> perf_ftrace_function_unregister()
                -> unregister_ftrace_function()
                  -> __unregister_ftrace_function()
                    -> WARN_ON(!(ops->flags & FTRACE_OPS_FL_ENABLED))

Fix this by checking FTRACE_OPS_FL_ENABLED before attempting to
unregister. If the ops is not enabled, just free the filter and
return success.

Assisted-by: Claude:claude-opus-4.7 syzkaller
Signed-off-by: Rik van Riel <riel@surriel.com>
---
 kernel/trace/trace_event_perf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index a6bb7577e8c5..58e1b427b576 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -497,7 +497,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);
 	return ret;
 }
-- 
2.52.0

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

[Steven Rostedt]

On Wed, 13 May 2026 13:24:45 -0400
Rik van Riel <riel@surriel.com> wrote:

> From 9de86227b917c49315b7b67aac3a83afae8d792d Mon Sep 17 00:00:00 2001
> From: Rik van Riel <riel@meta.com>
> Date: Sat, 25 Apr 2026 03:33:54 -0700
> Subject: [PATCH] perf/ftrace: Fix WARNING in __unregister_ftrace_function
> 

Can you resend this as a normal patch so that it can be picked up by patchwork.

Otherwise it will be ignored.

Thanks,

-- Steve

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

[Rik van Riel]

perf_ftrace_function_unregister() unconditionally calls
unregister_ftrace_function() without checking whether the ftrace_ops
was ever successfully registered. This triggers a WARN_ON in
__unregister_ftrace_function() when the ops doesn't have
FTRACE_OPS_FL_ENABLED set.

This can happen during perf_event_alloc() error cleanup when
perf_trace_destroy() is called via __free_event() on an event whose
ftrace_ops registration failed or was already torn down by
perf_try_init_event()'s err_destroy path.

The call path is:
  perf_event_alloc() error cleanup
    -> __free_event()
      -> event->destroy() [tp_perf_event_destroy]
        -> perf_trace_destroy()
          -> perf_trace_event_close()
            -> TRACE_REG_PERF_CLOSE
              -> perf_ftrace_function_unregister()
                -> unregister_ftrace_function()
                  -> __unregister_ftrace_function()
                    -> WARN_ON(!(ops->flags & FTRACE_OPS_FL_ENABLED))

Fix this by checking FTRACE_OPS_FL_ENABLED before attempting to
unregister. If the ops is not enabled, just free the filter and
return success.

Assisted-by: Claude:claude-opus-4.7 syzkaller
Signed-off-by: Rik van Riel <riel@surriel.com>
---
 kernel/trace/trace_event_perf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index a6bb7577e8c5..58e1b427b576 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -497,7 +497,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);
 	return ret;
 }
-- 
2.52.0

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

[Masami Hiramatsu]

On Wed, 13 May 2026 16:19:16 -0400
Rik van Riel <riel@surriel.com> wrote:

> perf_ftrace_function_unregister() unconditionally calls
> unregister_ftrace_function() without checking whether the ftrace_ops
> was ever successfully registered. This triggers a WARN_ON in
> __unregister_ftrace_function() when the ops doesn't have
> FTRACE_OPS_FL_ENABLED set.
> 
> This can happen during perf_event_alloc() error cleanup when
> perf_trace_destroy() is called via __free_event() on an event whose
> ftrace_ops registration failed or was already torn down by
> perf_try_init_event()'s err_destroy path.
> 
> The call path is:
>   perf_event_alloc() error cleanup
>     -> __free_event()
>       -> event->destroy() [tp_perf_event_destroy]
>         -> perf_trace_destroy()
>           -> perf_trace_event_close()
>             -> TRACE_REG_PERF_CLOSE
>               -> perf_ftrace_function_unregister()
>                 -> unregister_ftrace_function()
>                   -> __unregister_ftrace_function()
>                     -> WARN_ON(!(ops->flags & FTRACE_OPS_FL_ENABLED))
> 
> Fix this by checking FTRACE_OPS_FL_ENABLED before attempting to
> unregister. If the ops is not enabled, just free the filter and
> return success.
> 
> Assisted-by: Claude:claude-opus-4.7 syzkaller
> Signed-off-by: Rik van Riel <riel@surriel.com>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Fixes: ced39002f5ea ("ftrace, perf: Add support to use function tracepoint in perf")

Thanks,

> ---
>  kernel/trace/trace_event_perf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index a6bb7577e8c5..58e1b427b576 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -497,7 +497,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);
>  	return ret;
>  }
> -- 
> 2.52.0
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Steven Rostedt]

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?
> 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);
>     ...
> }
> However, it does not set event->destroy to NULL.
> When the event is later freed in error paths, the stale event->destroy callback
> executes a second time.
> If ftrace_free_filter() is called twice, won't it result in a direct
> double-free of ops->func_hash->filter_hash, since ftrace_free_filter() frees
> the hash but does not nullify the pointers?
> Additionally, for regular tracepoint events, wouldn't the second teardown
> execute perf_trace_event_unreg() again?
> This could double-decrement total_ref_count, potentially freeing the shared
> perf_trace_buf while other events are active, and trigger an unbalanced
> module_put() leading to module refcount underflows.
> Should the root cause be addressed by setting event->destroy = NULL in
> perf_try_init_event() immediately after invoking it?

> >  	return ret;
> >  }

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

[Rik van Riel]

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.

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

[Masami Hiramatsu]

On Fri, 22 May 2026 16:39:41 -0400
Rik van Riel <riel@surriel.com> 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@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;
> ..
> 

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@linux-foundation.org>
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@kernel.org>