[ Replying to Sashiko ]
> --- Patch [1]: [PATCH v4] tracing: Fix use-after-free freeing trigger private > data --- > commit cd29772ccd2634c1b696094501e9c7fd0bcd1c25 > Author: David Carlier <[email protected]> > > > 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. > > [ ... ] > > > 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
