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

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?

[ ... ]

> +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?

[ ... ]

> +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.

[ ... ]

> +	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?

[ ... ]

> +	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.

> +	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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178611679753.237811.10190793347982785419.stgit@devnote2?part=8