Re: [PATCH v4] tracing: Fix use-after-free freeing trigger private data
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2026-07-24 13:42:10
Also in:
lkml, sashiko-reviews
[ Replying to Sashiko ]
quoted hunk ↗ jump to hunk
--- Patch [1]: [PATCH v4] tracing: Fix use-after-free freeing trigger private data ---commit cd29772ccd2634c1b696094501e9c7fd0bcd1c25 Author: David Carlier [off-list ref]quoted
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 82ce492ab2681..58d28cd1afa3b 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c@@ -6349,6 +6349,7 @@ static void event_hist_trigger_free(struct event_trigger_data *data) trigger_data_free(data); + tracepoint_synchronize_unregister(); remove_hist_vars(hist_data); unregister_field_var_hists(hist_data);This isn't a bug introduced by this patch, but since we are fixing the teardown paths here, do the error paths in event_hist_trigger_parse() and event_enable_trigger_parse() need similar synchronization? For example, in event_hist_trigger_parse(): out_free: remove_hist_vars(hist_data); trigger_data_free(trigger_data); destroy_hist_data(hist_data); If trigger registration fails after the trigger was briefly published to file->triggers, could a concurrent tracepoint that observed the trigger dereference the synchronously freed hist_data because we didn't wait for an RCU grace period?
Where was it published to file->triggers?
I see it happening as the last operation that can fail. If that fails, then
we hit the error path where it is not registered. Is there someplace it can
become visible and then fail that I'm not seeing?
From what I see, the last command before "success" is:
ret = hist_trigger_enable(trigger_data, file);
And that does:
static int hist_trigger_enable(struct event_trigger_data *data,
struct trace_event_file *file)
{
int ret = 0;
list_add_tail_rcu(&data->list, &file->triggers);
update_cond_flag(file);
if (trace_event_trigger_enable_disable(file, 1) < 0) {
list_del_rcu(&data->list);
update_cond_flag(file);
ret--;
}
return ret;
}
Now it adds to the file trigger and then tries to enable that event. If it
fails to enable it, it removes it from the list. But if the event is not
enabled, it can't be racing with this.
[ ... ]quoted
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c index 655db2e825134..46e60b70a4bbc 100644 --- a/kernel/trace/trace_events_trigger.c +++ b/kernel/trace/trace_events_trigger.c@@ -1728,9 +1740,9 @@ void event_enable_trigger_free(struct event_trigger_data *data) if (!data->ref) { /* Remove the SOFT_MODE flag */ trace_event_enable_disable(enable_data->file, 0, 1); + data->private_data_free = enable_trigger_private_data_free; trace_event_put_ref(enable_data->file->event_call); trigger_data_free(data); - kfree(enable_data); } }This is a pre-existing issue, but by deferring the kfree of enable_data while keeping trace_event_put_ref() synchronous, could we still face a Use-After-Free issue during module unload? If trace_event_put_ref(enable_data->file->event_call) allows the target module to be unloaded and its trace event files to be freed immediately, could a concurrent tracepoint RCU reader dereference the freed enable_data->file in event_enable_trigger() when attempting to clear EVENT_FILE_FL_SOFT_DISABLED_BIT?
Hmm, this does look like it can be an issue. I'll write up a patch on top of this one. -- Steve