[PATCH] tracing: Fix error handling in event_trigger_parse

[PATCH] tracing: Fix error handling in event_trigger_parse

[Miaoqian Lin]

According to event_trigger_alloc() doc, event_trigger_free() should be
used to free an event_trigger_data object. This fixes a mismatch introduced
when kzalloc was replaced with event_trigger_alloc without updating
the corresponding deallocation calls.

Fixes: e1f187d09e11 ("tracing: Have existing event_command.parse() implementations use helpers")
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
 kernel/trace/trace_events_trigger.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index d45448947094..8389314b8c2d 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -995,7 +995,7 @@ event_trigger_parse(struct event_command *cmd_ops,
 
 	if (remove) {
 		event_trigger_unregister(cmd_ops, file, glob+1, trigger_data);
-		kfree(trigger_data);
+		event_trigger_free(trigger_data);
 		ret = 0;
 		goto out;
 	}
@@ -1022,7 +1022,7 @@ event_trigger_parse(struct event_command *cmd_ops,
 
  out_free:
 	event_trigger_reset_filter(cmd_ops, trigger_data);
-	kfree(trigger_data);
+	event_trigger_free(trigger_data);
 	goto out;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] tracing: Fix error handling in event_trigger_parse

[Masami Hiramatsu]

On Tue, 18 Mar 2025 19:27:37 +0800
Miaoqian Lin <linmq006@gmail.com> wrote:

> According to event_trigger_alloc() doc, event_trigger_free() should be
> used to free an event_trigger_data object. This fixes a mismatch introduced
> when kzalloc was replaced with event_trigger_alloc without updating
> the corresponding deallocation calls.
> 

Hmm, it seems more complicated problems are there. e.g. in `remove = true`
case, since the trigger_data is not initialized (no event_trigger_init()),
the `trigger_data->ref` is 0. Thus, ;

static void
event_trigger_free(struct event_trigger_data *data)
{
	if (WARN_ON_ONCE(data->ref <= 0))
		return;

	data->ref--;
	if (!data->ref)
		trigger_data_free(data);
}

this will never call `trigger_data_free(data)`. 

But latter part(after out_free) seems correct.

Tom, could you check it?

Thank you,

> Fixes: e1f187d09e11 ("tracing: Have existing event_command.parse() implementations use helpers")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
>  kernel/trace/trace_events_trigger.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index d45448947094..8389314b8c2d 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -995,7 +995,7 @@ event_trigger_parse(struct event_command *cmd_ops,
>  
>  	if (remove) {
>  		event_trigger_unregister(cmd_ops, file, glob+1, trigger_data);
> -		kfree(trigger_data);
> +		event_trigger_free(trigger_data);
>  		ret = 0;
>  		goto out;
>  	}
> @@ -1022,7 +1022,7 @@ event_trigger_parse(struct event_command *cmd_ops,
>  
>   out_free:
>  	event_trigger_reset_filter(cmd_ops, trigger_data);
> -	kfree(trigger_data);
> +	event_trigger_free(trigger_data);
>  	goto out;
>  }
>  
> -- 
> 2.39.5 (Apple Git-154)
> 
> 


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

Re: [PATCH] tracing: Fix error handling in event_trigger_parse

[Tom Zanussi]

Hi Masami,

On Wed, 2025-03-19 at 09:06 +0900, Masami Hiramatsu wrote:
> On Tue, 18 Mar 2025 19:27:37 +0800
> Miaoqian Lin <linmq006@gmail.com> wrote:
> 
> > According to event_trigger_alloc() doc, event_trigger_free() should be
> > used to free an event_trigger_data object. This fixes a mismatch introduced
> > when kzalloc was replaced with event_trigger_alloc without updating
> > the corresponding deallocation calls.
> > 
> 
> Hmm, it seems more complicated problems are there. e.g. in `remove = true`
> case, since the trigger_data is not initialized (no event_trigger_init()),
> the `trigger_data->ref` is 0. Thus, ;
> 
> static void
> event_trigger_free(struct event_trigger_data *data)
> {
> 	if (WARN_ON_ONCE(data->ref <= 0))
> 		return;
> 
> 	data->ref--;
> 	if (!data->ref)
> 		trigger_data_free(data);
> }
> 
> this will never call `trigger_data_free(data)`. 
> 
> But latter part(after out_free) seems correct.
> 
> Tom, could you check it?
> 

In both these cases, the code calls kfree() directly in order to avoid
the WARN_ON_ONCE(data->ref) check.

In the first case (remove), trigger_data is only being used as a test
object and will never have data->ref incremented.

The second case is the failure case, which is also dealing with a
trigger_data object that hasn't been successfully registered and
therefore has a 0 data->ref.

So perhaps the event_trigger_alloc doc should be changed to something
like:

"Use event_trigger_free() to free a successfully registered
event_trigger_data object."

Thanks,

Tom

> Thank you,
> 
> > Fixes: e1f187d09e11 ("tracing: Have existing event_command.parse() implementations use helpers")
> > Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> > ---
> >  kernel/trace/trace_events_trigger.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> > index d45448947094..8389314b8c2d 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -995,7 +995,7 @@ event_trigger_parse(struct event_command *cmd_ops,
> >  
> >  	if (remove) {
> >  		event_trigger_unregister(cmd_ops, file, glob+1, trigger_data);
> > -		kfree(trigger_data);
> > +		event_trigger_free(trigger_data);
> >  		ret = 0;
> >  		goto out;
> >  	}
> > @@ -1022,7 +1022,7 @@ event_trigger_parse(struct event_command *cmd_ops,
> >  
> >   out_free:
> >  	event_trigger_reset_filter(cmd_ops, trigger_data);
> > -	kfree(trigger_data);
> > +	event_trigger_free(trigger_data);
> >  	goto out;
> >  }
> >  
> > -- 
> > 2.39.5 (Apple Git-154)
> > 
> > 
> 
> 

Re: [PATCH] tracing: Fix error handling in event_trigger_parse

[Steven Rostedt]

On Wed, 19 Mar 2025 14:03:03 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> In both these cases, the code calls kfree() directly in order to avoid
> the WARN_ON_ONCE(data->ref) check.
> 
> In the first case (remove), trigger_data is only being used as a test
> object and will never have data->ref incremented.
> 
> The second case is the failure case, which is also dealing with a
> trigger_data object that hasn't been successfully registered and
> therefore has a 0 data->ref.
> 
> So perhaps the event_trigger_alloc doc should be changed to something
> like:
> 
> "Use event_trigger_free() to free a successfully registered
> event_trigger_data object."

Honestly, I think event_trigger_alloc() should set the data->ref to 1,
and remove the event_trigger_init() from those that use
event_trigger_alloc(). Then it's a lot easier to map
event_trigger_free() to event_trigger_alloc() and the users don't need
to keep track of the internals of event_triggers.

Then we don't need to have special cases of error conditions after
event_trigger_alloc(), we can simply use this patch.

So, this patch should stay as is, but another patch is needed before
this to make event_trigger_alloc() set data->ref to 1, and remove the
event_trigger_init() from the callers of event_trigger_alloc().

-- Steve

Re: [PATCH] tracing: Fix error handling in event_trigger_parse

[Steven Rostedt]

On Wed, 19 Mar 2025 14:03:03 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> In both these cases, the code calls kfree() directly in order to avoid
> the WARN_ON_ONCE(data->ref) check.
> 
> In the first case (remove), trigger_data is only being used as a test
> object and will never have data->ref incremented.
> 
> The second case is the failure case, which is also dealing with a
> trigger_data object that hasn't been successfully registered and
> therefore has a 0 data->ref.
> 
> So perhaps the event_trigger_alloc doc should be changed to something
> like:
> 
> "Use event_trigger_free() to free a successfully registered
> event_trigger_data object."

I was trying to get this patch in, and realized that the code is all messed
up.

event_trigger_alloc() creates a event_trigger_data which needs to be freed
by trigger_data_free() NOT event_trigger_free()!

I'm renaming event_tigger_alloc() to trigger_data_alloc(), and changing
this patch to just call trigger_data_free() on error.

One day, if I get time, I need to rewrite the event trigger code, as it's
really confusing to deal with! Trying to follow the function pointers that
get called via init, reg, unreg, etc between struct event_command and
struct event_trigger_ops is just a nightmare!

-- Steve