út 25. 2. 2025 v 18:53 odesílatel Steven Rostedt <[email protected]> napsal: > > From: Steven Rostedt <[email protected]> > > The following commands causes a crash: > > ~# cd /sys/kernel/tracing/events/rcu/rcu_callback > ~# echo 'hist:name=bad:keys=common_pid:onmax(bogus).save(common_pid)' > > trigger > bash: echo: write error: Invalid argument > ~# echo 'hist:name=bad:keys=common_pid' > trigger > > Because the following occurs: > > 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 a hist with name is registered, it causes an u-a-f bug and > the kernel can crash. > > Move the code around such that if event_trigger_register() succeeds, the > next thing called is hist_trigger_enable() which adds it to the list. > > A bunch of actions is called if get_named_trigger_data() returns false. > But that doesn't need to be called after event_trigger_register(), so it > can be moved up, allowing event_trigger_register() to be called just > before hist_trigger_enable() keeping them together and allowing the > file->triggers to be properly populated. > > Cc: [email protected] > Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist > triggers") > Reported-by: Tomas Glozar <[email protected]> > Closes: > https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=oe_uyh8ked9_szc4i++4h72mjlic4_...@mail.gmail.com/ > Signed-off-by: Steven Rostedt (Google) <[email protected]> > --- > kernel/trace/trace_events_hist.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > 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; > -- > 2.47.2 >
Applied this on 6.14.0-rc4 and the problem is gone. Tested-by: Tomas Glozar <[email protected]> Tomas
