| Message ID | 20250225125356.29236cd1@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing: Fix bad hist from corrupting named_triggers list | expand |
út 25. 2. 2025 v 18:53 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal: > > From: Steven Rostedt <rostedt@goodmis.org> > > 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: stable@vger.kernel.org > Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers") > Reported-by: Tomas Glozar <tglozar@redhat.com> > Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/ > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > 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 <tglozar@redhat.com> Tomas
Hi Steve, On Tue, 2025-02-25 at 12:53 -0500, Steven Rostedt wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > 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: stable@vger.kernel.org > Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers") > Reported-by: Tomas Glozar <tglozar@redhat.com> > Closes: https://lore.kernel.org/all/CAP4=nvTsxjckSBTz=Oe_UYh8keD9_sZC4i++4h72mJLic4_W4A@mail.gmail.com/ > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Looks like a good fix, and is cleaner without the goto as well. Small typo below... Reviewed-by: Tom Zanussi <zanussi@kernel.org> > --- > 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; > + > + Extra space added here. > ret = hist_trigger_enable(trigger_data, file); > if (ret) > goto out_unreg; Thanks, Tom
On Thu, 27 Feb 2025 13:53:27 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > > -enable: > > + ret = event_trigger_register(cmd_ops, file, glob, trigger_data); > > + if (ret < 0) > > + goto out_free; > > + > > + > > Extra space added here. Bah, I'll remove that when adding your tested by. I've already ran it through my entire test suite, but I'm going to break my rule and just build and boot to test removing that line, and not run the entire test suite. Now, if there's another bug that shows up, it will get run through that. Thanks, -- 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;