| Message ID | 20250318112737.4174-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tracing: Fix error handling in event_trigger_parse | expand |
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) > >
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) > > > > > >
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
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; }
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(-)