Re: [PATCH v13 09/12] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Sat, 22 Aug 2026 09:42:17 +0000
sashiko-bot@kernel.org 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) <mhiramat@kernel.org>
> 
> 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.wangjinchao600@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(&param, ":");
> 
> [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) <mhiramat@kernel.org>