Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Use-After-Free list corruption if `event_hist_trigger_init` fails during named trigger initialization. - [High] Use-After-Free list corruption because a newly created named trigger remains on the global `named_triggers` list if `hist_trigger_enable()` fails. -- commit 1e5ea6c77dc8e3a10275a88758d5ef5a8d54be44 Author: Donggeun Yoo <[email protected]> tracing: hist: set the trace clock before registering the trigger hist_register_trigger() puts the trigger on the global named_triggers list in cmd_ops->init(), and only then sets the trace clock. The clock string is not checked anywhere before that call, so a named trigger using common_timestamp with an unknown clock fails after it has already become findable. > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 963e0d6b61fdf..6c628415468a6 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -6643,12 +6643,6 @@ static int hist_register_trigger(char *glob, > data->cmd_ops = cmd_ops; > } > > - if (data->cmd_ops->init) { > - ret = data->cmd_ops->init(data); > - if (ret < 0) > - goto out; > - } > - [ ... ] > @@ -6661,6 +6655,15 @@ static int hist_register_trigger(char *glob, > tracing_set_filter_buffering(file->tr, true); > } > > + if (data->cmd_ops->init) { > + ret = data->cmd_ops->init(data); > + if (ret < 0) { > + if (hist_data->enable_timestamps) > + tracing_set_filter_buffering(file->tr, false); > + goto out; > + } > + } > + > if (named_data) { > remove_hist_vars(hist_data); > destroy_hist_data(hist_data); While reviewing this patch, I noticed a couple of pre-existing issues related to how named triggers are handled during initialization and error paths in this same file. kernel/trace/trace_events_hist.c:event_hist_trigger_named_init() { ... save_named_trigger(data->named_data->name, data); ret = event_hist_trigger_init(data->named_data); if (ret < 0) { kfree(data->cmd_ops); data->cmd_ops = &trigger_hist_cmd; } return ret; } [Severity: High] This isn't a bug introduced by this patch, but does this error path leave the new trigger on the global named_triggers list? If event_hist_trigger_init() fails, the trigger is never removed from the list before the function returns the error. The caller then propagates this error, eventually calling trigger_data_free() which frees the structure. Can this lead to a Use-After-Free list corruption when the global named_triggers list is accessed later? Also, further down in the file: kernel/trace/trace_events_hist.c:event_hist_trigger_parse() { ... 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; ... out_unreg: event_trigger_unregister(cmd_ops, file, glob+1, trigger_data); out_free: remove_hist_vars(hist_data); trigger_data_free(trigger_data); ... } [Severity: High] This is a pre-existing issue, but does this error path also result in Use-After-Free list corruption? If hist_trigger_enable() fails, it drops the trigger from the local file list but then we jump to out_unreg. Because the trigger is no longer in file->triggers, event_trigger_unregister() won't find it and skips calling cmd_ops->free() (which would normally call del_named_trigger()). The code then falls through to trigger_data_free(). Does this manually free the memory without ever calling del_named_trigger(), leaving a freed node on the global named_triggers list? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
