On Mon, 24 Feb 2025 13:10:49 -0500
Steven Rostedt <rost...@goodmis.org> wrote:
> On Mon, 24 Feb 2025 16:22:39 +0100
> Tomas Glozar <tglo...@redhat.com> wrote:
>
> Thanks for the report.
>
> > I'm not familiar with the trigger implementation. Do you have any
> > ideas on why the hist_unregister_trigger fails and/or a fix?
>
> One day I would love to rewrite the trigger code. It's really convoluted
> and not easy to follow. I'm constantly trying to remember the workflow of
> it.
>
> Anyway, I'll dig into this.
I think I figured it out.
event_trigger_write() {
trigger_process_regex() {
event_hist_trigger_parse() {
data = event_trigger_alloc(..);
event_trigger_register(.., data) {
cmd_ops->reg(.., data, ..) [hist_register_trigger()] {
data->ops->init() [event_hist_trigger_init()] {
save_named_trigger(name, data) {
list_add(&data->named_list, &named_triggers);
}
}
}
}
ret = create_actions(); (return -EINVAL)
if (ret)
goto out_unreg;
[..]
ret = hist_trigger_enable(data, ...) {
list_add_tail_rcu(&data->list, &file->triggers); <<<---- SKIPPED!!!
(this is important!)
[..]
out_unreg:
event_hist_unregister(.., data) {
cmd_ops->unreg(.., data, ..) [hist_unregister_trigger()] {
list_for_each_entry(iter, &file->triggers, list) {
if (!hist_trigger_match(data, iter, named_data, false)) <- never
matches
continue;
[..]
test = iter;
}
if (test && test->ops->free) <<<-- test is NULL
test->ops->free(test) [event_hist_trigger_free()] {
[..]
if (data->name)
del_named_trigger(data) {
list_del(&data->named_list); <<<<-- NEVER gets removed!
}
}
}
}
[..]
kfree(data); <<<-- frees item but it is still on list
The next time you register a item, it accesses this freed value.
So I moved the code around such that if event_trigger_register() succeeds,
the next thing called is hist_trigger_enable() which adds it to the list.
Probably should be port of event_hist_register() instead, but that could be
part of a clean up.
Looks like a bunch of actions is supposed to be called if
get_named_trigger_data() returns false. But that doesn't need to be called
after event_trigger_register(), so moving that up shouldn't be an issue. At
least I can't find one.
Can you try this patch?
-- Steve
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 261163b00137..c32adc372808 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6724,27 +6724,28 @@ static int event_hist_trigger_parse(struct
event_command *cmd_ops,
if (existing_hist_update_only(glob, trigger_data, file))
goto out_free;
- ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
- if (ret < 0)
- goto out_free;
+ if (!get_named_trigger_data(trigger_data)) {
- if (get_named_trigger_data(trigger_data))
- goto enable;
+ ret = create_actions(hist_data);
+ if (ret)
+ goto out_free;
- ret = create_actions(hist_data);
- if (ret)
- goto out_unreg;
+ if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
+ ret = save_hist_vars(hist_data);
+ if (ret)
+ goto out_free;
+ }
- if (has_hist_vars(hist_data) || hist_data->n_var_refs) {
- ret = save_hist_vars(hist_data);
+ ret = tracing_map_init(hist_data->map);
if (ret)
- goto out_unreg;
+ goto out_free;
}
- ret = tracing_map_init(hist_data->map);
- if (ret)
- goto out_unreg;
-enable:
+ ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
+ if (ret < 0)
+ goto out_free;
+
+
ret = hist_trigger_enable(trigger_data, file);
if (ret)
goto out_unreg;
