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 <[email protected]> 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 <[email protected]>
> > ---
> > 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)
> >
> >
>
>
