Re: [PATCH v12 08/11] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers

On Fri, 07 Aug 2026 15:54:39 +0000
[email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Mixing scope-based cleanup (`__free()`) with explicit resource management causes double-frees and memory leaks.
> - [High] Hardware breakpoint fallback address spans adjacent kernel variables, causing NMI storms.
> - [High] Double initialization of the event trigger reference count causes unconditional memory leaks.
> - [Medium] Trigger `count` is double-decremented, breaking the `:count=1` feature.
> - [Medium] The `clear_wprobe` trigger fails to parse its documented optional FIELD parameter.
> --
> 
> --- Patch [8]: [PATCH v12 08/11] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers ---
> commit cb59d7e61a8c78c308ca9ae32e1de403900f7a05
> Author: Masami Hiramatsu (Google) <[email protected]>
> Subject: tracing/wprobe: Add set_wprobe and clear_wprobe event triggers
> 
> This commit adds 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
> > --- a/kernel/trace/trace_wprobe.c
> > +++ b/kernel/trace/trace_wprobe.c
> 
> [ ... ]
> 
> > +static int wprobe_trigger_global_enabled;
> > +
> > +#define SET_WPROBE_STR		"set_wprobe"
> > +#define CLEAR_WPROBE_STR	"clear_wprobe"
> > +#define WPROBE_DEFAULT_CLEAR_ADDRESS ((unsigned long)&wprobe_trigger_global_enabled)
> 
> [Severity: High]
> Will this 4-byte fallback variable safely accommodate 8-byte hardware
> breakpoints? 
> 
> When a wprobe is temporarily disabled, its hardware breakpoint is pointed here
> at WPROBE_DEFAULT_CLEAR_ADDRESS, but it retains its configured length (e.g., 8
> bytes for a 64-bit pointer). 
> 
> Could this cause the breakpoint to monitor the 4-byte variable plus 4 adjacent
> bytes in the .bss section, potentially triggering NMI storms if a frequently
> accessed kernel variable happens to reside in those adjacent bytes?

Ah, indeed. This should be fixed.

> 
> [ ... ]
> 
> > +static void wprobe_trigger(struct event_trigger_data *data,
> > +			   struct trace_buffer *buffer,  void *rec,
> > +			   struct ring_buffer_event *event)
> > +{
> [ ... ]
> > +	/* count < 0 means endless, 0 means trigger count exhausted */
> > +	if (!data->count)
> > +		goto out;
> [ ... ]
> > +	if (changed) {
> > +		if (data->count > 0)
> > +			data->count--;
> 
> [ ... ]
> 
> > +static struct event_command trigger_wprobe_set_cmd = {
> > +	.name			= SET_WPROBE_STR,
> [ ... ]
> > +	.trigger		= wprobe_trigger,
> > +	.count_func		= event_trigger_count,
> 
> [Severity: Medium]
> Does this manual decrement in wprobe_trigger() conflict with the core
> event_trigger_count decrement function? 
> 
> The trigger commands are assigned .count_func = event_trigger_count, which
> unconditionally executes before the main .trigger callback and pre-decrements
> the count. 
> 
> If a user specifies :count=1, the core framework reduces it to 0. 
> wprobe_trigger() will then see a count of 0 and abort immediately. Does this
> render a single-count trigger completely non-functional?

Ah, good catch! OK, let's use the standard one at this point.

> 
> [ ... ]
> 
> > +static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,
> > +				    struct trace_event_file *file,
> > +				    char *glob, char *cmd,
> > +				    char *param_and_filter)
> > +{
> > +	/*
> > +	 * set_wprobe:EVENT:FIELD[+OFFS]
> > +	 * clear_wprobe:EVENT[:FIELD[+OFFS]]
> > +	 */
> > +	struct wprobe_trigger_data *wprobe_data __free(free_wprobe_trigger_data) = NULL;
> > +	struct event_trigger_data *trigger_data __free(kfree) = NULL;
> 
> [ ... ]
> 
> > +	/* clear_wprobe does not need field. */
> > +	if (!clear) {
> > +		char *offs;
> 
> [Severity: Medium]
> Does skipping the field parsing here break the documented optional FIELD
> parameter for clear_wprobe? 
> 
> The documentation claims clear_wprobe:WPEVENT[:FIELD] conditionally clears a
> watchpoint, but if clear is true, the parsing code skips extracting the field
> string. 
> 
> It looks like the unparsed string is then passed to event_trigger_parse_num(),
> which expects a numeric string and fails with -EINVAL.

Yeah, I think we can just drop FIELD support for clear_wprobe.

> 
> [ ... ]
> 
> > +	ret = event_trigger_parse_num(param, trigger_data);
> > +	if (ret) {
> > +		wprobe_trigger_log_err(file, glob, param - glob, BAD_IMM);
> > +		return ret;
> > +	}
> 
> [Severity: High]
> Can this early error path leak memory? 
> 
> The trigger_data uses __free(kfree), which performs a shallow free. If
> event_trigger_parse_num() fails and returns early, does this permanently leak
> internal allocations like data->name?

Oops, let me fix it.

> 
> [ ... ]
> 
> > +	event_trigger_init(trigger_data);
> > +
> > +	ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> 
> [Severity: High]
> Will this result in a double initialization of the reference count? 
> 
> We call event_trigger_init(trigger_data) explicitly here, which increments the
> reference count. Then event_trigger_register() calls the .reg callback
> wprobe_register_trigger(), which executes data->cmd_ops->init(data) and
> increments the reference count a second time. 
> 
> When the trigger is later unregistered, wprobe_trigger_free() decrements the
> count by only 1, leaving a permanent reference and memory leak.

OK, this refcounter updates seems to related to the below error path.

> 
> > +	if (ret) {
> > +		event_trigger_reset_filter(cmd_ops, trigger_data);
> > +		trace_event_enable_disable(wprobe_file, 0, 1);
> > +		trace_event_put_ref(wprobe_file->event_call);
> > +		tracepoint_synchronize_unregister();
> > +		event_trigger_free(trigger_data);
> > +		return ret;
> > +	}
> 
> [Severity: High]
> Can this late error path cause a double-free? 
> 
> Both trigger_data and wprobe_data are declared with scope-based cleanup
> (__free). If event_trigger_register() fails, the code manually calls
> event_trigger_free(trigger_data), freeing both objects. 
> 
> When the function returns, the __free macros will execute and attempt to free
> them a second time.

Let me fix both cases.

Thank you!

-- 
Masami Hiramatsu (Google) <[email protected]>