On Mon, 14 Sep 2026 14:58:44 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s)
> to consider:
> - [High] Missing RCU synchronization in the error path of wprobe trigger
> registration leads to a Use-After-Free of hardware breakpoint perf_event
> structures.
> - [High] Hardware breakpoint addresses are not updated for offline CPUs,
> leading to stale addresses when CPUs come online.
> - [Medium] The 'count' parameter is decremented on trigger evaluation misses,
> causing the trigger to exhaust its count without actually modifying the
> watchpoint.
> - [Medium] The wprobe event permanently leaks as enabled when the trigger is
> unregistered because `EVENT_FILE_FL_SOFT_DISABLED_BIT` is manually cleared.
> - [Low] A field named 'unlimited' is silently ignored by `clear_wprobe` if no
> count keyword is provided.
> --
>
> commit 146ca411781d7203b266aa3745d30641c87b97d2
> Author: Masami Hiramatsu (Google) <[email protected]>
>
> tracing/wprobe: Add set_wprobe and clear_wprobe event triggers
>
> Add set_wprobe and clear_wprobe event triggers to dynamically attach
> and detach hardware breakpoint address monitoring based on event field
> contents.
>
> Link:
> https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao...@gmail.com/
>
> > diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> > index 9c6100b63e8d9..55a201504bb0b 100644
> > --- a/kernel/trace/trace_wprobe.c
> > +++ b/kernel/trace/trace_wprobe.c
>
> [ ... ]
>
> > @@ -231,6 +303,12 @@ static struct trace_wprobe *alloc_trace_wprobe(const
> > char *group,
> >
> > +static void wprobe_work_func(struct work_struct *work)
> > +{
> > + struct trace_wprobe *tw = container_of(work, struct trace_wprobe, work);
> > +
> > + on_each_cpu(wprobe_smp_update_func, tw, true);
>
> [Severity: High]
> Could this leave hardware breakpoints with stale addresses on offline CPUs?
>
> on_each_cpu() only broadcasts to currently online CPUs, so offline CPUs
> won't execute trace_wprobe_update_local(). When an offline CPU comes
> online, won't the perf event core reinstall the hardware breakpoint using
> the old, stale address since it wasn't updated here?
OK, introduce cpu hotplug callback to handle it.
>
> [ ... ]
>
> > @@ -773,3 +866,538 @@ static __init int init_wprobe_trace(void)
>
> [ ... ]
>
> > +static void wprobe_trigger(struct event_trigger_data *data,
> > + struct trace_buffer *buffer, void *rec,
> > + struct ring_buffer_event *event)
> > +{
> > + struct wprobe_trigger_data *wprobe_data = data->private_data;
> > + struct trace_wprobe *tw = wprobe_data->tw;
> > + unsigned long addr = 0, flags;
> > + bool changed = false;
>
> [ ... ]
>
> > + raw_spin_lock_irqsave(&tw->lock, flags);
> > +
> > + if (!wprobe_data->clear) {
> > + if (!trace_wprobe_is_valid_addr(addr, tw->len)) {
> > + atomic_inc(&tw->missed);
> > + goto out;
> > + }
> > + if (tw->addr == WPROBE_DEFAULT_CLEAR_ADDRESS) {
> > + /* Skip if a previous work is still propagating the
> > address */
> > + if (atomic_read(&tw->work_pending)) {
> > + atomic_inc(&tw->missed);
> > + goto out;
> > + }
> > + WRITE_ONCE(tw->addr, addr);
> > + changed = true;
> > + clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT,
> > &wprobe_data->file->flags);
>
> [Severity: Medium]
> Does manually clearing EVENT_FILE_FL_SOFT_DISABLED_BIT cause the event to leak
> as enabled?
>
> When the trigger is unregistered, wprobe_trigger_free() calls
> trace_event_enable_disable(file, 0, 1). Because the soft-disabled bit was
> cleared here, won't the core event logic skip the unregister step, leaving
> the wprobe tracepoint permanently enabled in the kernel?
Good catch!
Hmm, we should set EVENT_FILE_FL_SOFT_DISABLED_BIT when removing
a wprobe_trigger.
>
> [ ... ]
>
> > +static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,
> > + struct trace_event_file *file,
> > + char *glob, char *cmd,
> > + char *param_and_filter)
> > +{
>
> [ ... ]
>
> > + } else if (param && (isalpha(param[0]) || param[0] == '_')) {
> > + if (strncmp(param, "count=", 6) != 0 &&
> > + strcmp(param, "unlimited") != 0) {
>
> [Severity: Low]
> Is this intended to skip fields named "unlimited"?
>
> If a user configures a clear_wprobe trigger on an event that happens to have
> a field named "unlimited", this parser matches the keyword and skips parsing
> it as a field, which defaults to an unconditional clear action.
Hmm, OK. Let's check the event has "unlimited" field or not.
>
> [ ... ]
>
> > +static int wprobe_register_trigger(char *glob,
> > + struct event_trigger_data *data,
> > + struct trace_event_file *file)
> > +{
> > + int ret = 0;
> > +
> > + lockdep_assert_held(&event_mutex);
> > +
> > + /* The same wprobe is not accept on the same file (event) */
> > + if (wprobe_trigger_find_same(data, file))
> > + return -EEXIST;
> > +
> > + if (data->cmd_ops->init) {
> > + ret = data->cmd_ops->init(data);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + list_add_rcu(&data->list, &file->triggers);
> > +
> > + update_cond_flag(file);
> > + ret = trace_event_trigger_enable_disable(file, 1);
> > + if (ret < 0) {
> > + list_del_rcu(&data->list);
> > + update_cond_flag(file);
> > + if (data->cmd_ops->free)
> > + data->cmd_ops->free(data);
> > + }
>
> [Severity: High]
> Does this error path need RCU synchronization?
>
> If trace_event_trigger_enable_disable() fails, list_del_rcu() is called
> without a subsequent synchronize_rcu(). The cmd_ops->free callback
> (wprobe_trigger_free) will then synchronously disable the wprobe, which
> frees the tw->bp_event array.
>
> Could a concurrent tracepoint reader still observe the trigger data,
> execute wprobe_trigger(), and dereference the freed tw->bp_event via
> this_cpu_ptr() inside trace_wprobe_update_local()?
Yes. Let me add tracepoint_synchronize_unregister() here for
waiting RCU.
>
> [ ... ]
>
> > +static struct event_command trigger_wprobe_set_cmd = {
> > + .name = SET_WPROBE_STR,
> > + .trigger_type = ETT_EVENT_WPROBE,
> > + /* This triggers after when the event is recorded. */
> > + .flags = EVENT_CMD_FL_NEEDS_REC,
> > + .parse = wprobe_trigger_cmd_parse,
> > + .reg = wprobe_register_trigger,
> > + .unreg = wprobe_unregister_trigger,
> > + .set_filter = set_trigger_filter,
> > + .trigger = wprobe_trigger,
> > + .count_func = event_trigger_count,
>
> [Severity: Medium]
> Will this unconditionally exhaust the trigger's count parameter even on
> misses?
>
> Because .count_func is set to event_trigger_count, the count is decremented
> before wprobe_trigger() runs. If the trigger fails its implicit checks inside
> wprobe_trigger() (such as checking if the field matches the watched address),
> the count has already been consumed.
Hmm, OK. Let me add wprobe_count_func() to check the condition.
Thank you,
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/178939017565.94750.9431053336761330458.stgit@devnote2?part=10
--
Masami Hiramatsu (Google) <[email protected]>
