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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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)
> > 
> > 
> 
>