On Sat, 22 Aug 2026 09:42:17 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [Medium] Leaked `trigger_data` and `wprobe_data` when trigger registration
> fails due to unbalanced reference counts.
> - [Medium] Hardware breakpoints are inadvertently left monitoring the initial
> wprobe address (unparked) instead of the parked dummy address upon enablement.
> - [Low] The `clear_wprobe` trigger cannot be removed by echoing its exact
> printed output if a trigger count is specified.
> --
>
> --- Patch [9]: [PATCH v13 09/12] tracing/wprobe: Add set_wprobe and
> clear_wprobe event triggers ---
> commit e794ff7412c73348b335e9a50aceecbbd82d9a0c
> 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 a8543766a245b..acb15fd018e80 100644
> > --- a/kernel/trace/trace_wprobe.c
> > +++ b/kernel/trace/trace_wprobe.c
>
> [ ... ]
>
> > @@ -756,3 +827,501 @@ static __init int init_wprobe_trace(void)
>
> [ ... ]
>
> > +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] == '_')) {
> > + char *offs;
> > +
> > + field_str = strsep(¶m, ":");
>
> [Severity: Low]
> Does this code prevent removing the clear_wprobe trigger if it has a count
> but no field?
Ah, indeed. The trigger shows ":count=N" or ":unlimited" for counter
instead of digit value.
>
> If the user sets a count on a clear_wprobe trigger, wprobe_trigger_print()
> outputs the trigger as ending with ":count=N". When attempting to remove it
> by writing the exact string prepended with '!', this parser checks if the
> parameter starts with a letter.
>
> Since "count=N" starts with a letter ('c'), it treats it as an event field
> name, and fails with -ENOENT in wprobe_trigger_field_parse(), preventing the
> trigger from being unregistered.
OK, let me fix it.
>
> [ ... ]
>
> > + /* Soft-enable (register) wprobe event on WPROBE_DEFAULT_CLEAR_ADDRESS
> > */
> > + if (!trace_event_try_get_ref(wprobe_file->event_call)) {
> > + ret = -ENODEV;
> > + goto out_free_trigger;
> > + }
> > +
> > + ret = trace_event_enable_disable(wprobe_file, 1, 1);
> > + if (ret < 0)
> > + goto out_put;
> > +
> > + if (!clear)
> > + WRITE_ONCE(tw->addr, WPROBE_DEFAULT_CLEAR_ADDRESS);
>
> [Severity: Medium]
> Should we update the target address before soft-enabling the event?
Ah, indeed. We also need to roll it back if soft-enable fails.
>
> The documentation suggests initializing a new wprobe at address 0 and keeping
> it disabled. When we call trace_event_enable_disable(wprobe_file, 1, 1) here,
> it installs hardware breakpoints monitoring the current address (e.g., 0).
>
> The parked dummy address WPROBE_DEFAULT_CLEAR_ADDRESS is only assigned to
> tw->addr afterwards, which means the hardware breakpoints are left actively
> monitoring address 0 and could generate spurious debug exceptions.
>
> [ ... ]
>
> > +out_disable:
> > + trace_event_enable_disable(wprobe_file, 0, 1);
> > +out_put:
> > + trace_event_put_ref(wprobe_file->event_call);
> > +out_free_trigger:
> > + event_trigger_reset_filter(cmd_ops, trigger_data);
> > + event_trigger_free(trigger_data);
> > + return ret;
>
> [Severity: Medium]
> Can this leak trigger_data and wprobe_data on the error path?
>
> When wprobe_trigger_cmd_parse() executes, it increments the trigger_data
> refcount to 1 via event_trigger_init().
>
> It then calls event_trigger_register() which calls wprobe_register_trigger()
> and increments the refcount to 2 via data->cmd_ops->init(data).
>
> If trace_event_trigger_enable_disable() fails inside
> wprobe_register_trigger(),
> it returns an error without balancing the reference count:
>
> kernel/trace/trace_wprobe.c:wprobe_register_trigger() {
> ...
> ret = trace_event_trigger_enable_disable(file, 1);
> if (ret < 0) {
> list_del_rcu(&data->list);
> update_cond_flag(file);
> }
> return ret;
> }
>
> The error path here then calls event_trigger_free(trigger_data), which only
> decrements the refcount from 2 to 1 and skips freeing the objects.
Ah, OK. It should call the data->cmd_ops->free() in failure path.
Thanks,
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=9
--
Masami Hiramatsu (Google) <[email protected]>
