Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The patch leaves an identical Use-After-Free and memory leak if trigger registration fails after `.init()` succeeds. - [Medium] Ignored return value from `save_named_trigger()` causes silent failure and breaks histogram sharing. -- commit 540a72d9b730e99f5331add357da7f87a7bcb997 Author: Donggeun Yoo <[email protected]> tracing: hist: take the reference before publishing the named trigger This commit delays the publication of a named trigger to the global list until after its initialization succeeds. This prevents a failed initialization from leaving a dangling pointer in the named_triggers list and fixes a use-after-free bug. Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/ > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 963e0d6b61fdf..c6c04926bdf0c 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -6383,17 +6383,18 @@ static int event_hist_trigger_named_init(struct > event_trigger_data *data) > { > int ret; > > - data->ref++; > - > - 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; > } > > - return ret; > + data->ref++; > + > + save_named_trigger(data->named_data->name, data); [Severity: Medium] This is a pre-existing issue, but does ignoring the return value of save_named_trigger() break histogram sharing? save_named_trigger() uses kstrdup() to allocate the name string, which can fail and return -ENOMEM. If this happens, the trigger is not added to the named_triggers list, but the function continues as if it succeeded. Could this lead to a state where the trigger exists but cannot be found by functions like pause_named_trigger()? Does this silently break histogram sharing semantics (causing subsequent triggers with the same name to create independent maps instead of sharing them) and group pause/unpause logic? This same return value is also ignored in event_hist_trigger_init(): kernel/trace/trace_events_hist.c:event_hist_trigger_init() { ... if (!data->ref && hist_data->attrs->name) save_named_trigger(hist_data->attrs->name, data); ... } > + > + return 0; [Severity: High] This isn't a bug introduced by this patch, but does this fix leave another identical use-after-free and memory leak reachable from tracefs? If trigger registration fails in hist_register_trigger() after this .init() function succeeds, the error propagates up without undoing it: kernel/trace/trace_events_hist.c:hist_register_trigger() { ... ret = tracing_set_clock(file->tr, hist_data->attrs->clock); if (ret) { hist_err(tr, HIST_ERR_SET_CLOCK_FAIL, errpos(clock)); goto out; } ... } The caller event_hist_trigger_parse() then catches this error and blindly frees the trigger data: kernel/trace/trace_events_hist.c:event_hist_trigger_parse() { ... out_free: remove_hist_vars(hist_data); trigger_data_free(trigger_data); ... } Because trigger_data_free() does not invoke cmd_ops->free(), does this leave the newly freed trigger linked in the global named_triggers list and leak data->cmd_ops, leading to the exact same KASAN slab-use-after-free this patch was intended to fix? > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
