Re: [PATCH v7 01/10] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint

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

On Wed, 15 Jul 2026 02:01:39 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Missing error rollback in enable_trace_wprobe() leaves dangling event file links and active hardware breakpoints, leading to Use-After-Free and resource leaks.
> - [High] Format string bugs in trace_wprobe_show break the dynamic_events save and restore functionality.
> - [Medium] wprobes silently drop all perf events because the hardware breakpoint handler lacks perf ring buffer delivery logic.
> - [Medium] wprobe_trace_handler silently drops events if the hardware breakpoint address was updated, contradicting the stated design.
> - [Medium] Unconditional memory leak of the dynamically allocated `symbol` string in `__trace_wprobe_create()`.
> - [Medium] Undocumented `$value` variable is implemented, contradicting the commit message claims.

Thanks for the comments.

> --
> 
> --- Patch [1]: [PATCH v7 01/10] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint ---
> commit f8a7aeb231575f15f9227ab411134bde7408e480
> Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
>     
> This patch adds a new dynamic event for hardware breakpoints, called a
> wprobe-event. It allows users to trace memory access at specified addresses
> or symbols, supporting both ftrace and perf interfaces.
> 
> Link: https://lore.kernel.org/all/175859021100.374439.8723137923620348816.stgit@devnote2/
> 
> > User also can use $addr to fetch the accessed address. But no other
> > variables are supported. To record updated value, use '+0($addr)'.
> 
> [Severity: Medium]
> Is this documentation accurate?
> 
> Looking at parse_probe_vars(), it explicitly parses and handles a $value
> variable:
> 
> parse_probe_vars() {
>     ...
>     if (!strcmp(arg, "value")) {
>         code->op = FETCH_OP_BADDR;
>         code++;
>         code->op = FETCH_OP_DEREF;
> ...
> 
> Should the commit message be updated to reflect that $value is actually
> supported?

Good catch! Yeah $value was added, so we don't need to use +0($addr).
The $addr is used for ensuring watching address. That will be useful when
wprobe trigger is introduced.

> 
> > diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> > new file mode 100644
> > index 0000000000000..b52f3eac719f9
> > --- /dev/null
> > +++ b/kernel/trace/trace_wprobe.c
> 
> [ ... ]
> 
> > +static void wprobe_trace_handler(struct trace_wprobe *tw,
> > +				 unsigned long addr,
> > +				 struct pt_regs *regs,
> > +				 struct trace_event_file *trace_file)
> > +{
> 
> [ ... ]
> 
> > +	if (trace_trigger_soft_disabled(trace_file))
> > +		return;
> > +
> > +	if (tw->addr != addr)
> > +		return;
> 
> [Severity: Medium]
> Does this strict address comparison inadvertently drop valid events if the
> hardware breakpoint address was updated dynamically?

Yes, to avoid confusion, if the watchpoint is changed on another CPU,
it is better to be skipped.

> 
> The v7 changelog mentions using the dynamic address because the original
> address can be updated from another CPU, but this check causes the handler
> to exit immediately if the updated address differs from the statically
> recorded tw->addr.

Yeah, this is something for wprobe trigger, which will dynamically update
the tw->addr. So maybe we can move this in wprobe event trigger patch.

> 
> [ ... ]
> 
> > +static void wprobe_perf_handler(struct perf_event *bp,
> > +			      struct perf_sample_data *data,
> > +			      struct pt_regs *regs)
> > +{
> > +	struct trace_wprobe *tw = bp->overflow_handler_context;
> > +	struct event_file_link *link;
> > +	unsigned long addr = bp->attr.bp_addr;
> > +
> > +	trace_probe_for_each_link_rcu(link, &tw->tp)
> > +		wprobe_trace_handler(tw, addr, regs, link->file);
> > +}
> 
> [Severity: Medium]
> Does this handler omit the necessary logic to generate and submit perf
> samples?

Ah, OK. This should not be used from perf. I think this should be
explicitly prohibited at this moment.


> 
> [ ... ]
> 
> > +static int enable_trace_wprobe(struct trace_event_call *call,
> > +			       struct trace_event_file *file)
> > +{
> 
> [ ... ]
> 
> > +	if (file) {
> > +		ret = trace_probe_add_file(tp, file);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
> > +	}
> > +
> > +	if (!enabled) {
> > +		list_for_each_entry(tw, trace_probe_probe_list(tp), tp.list) {
> > +			ret = __register_trace_wprobe(tw);
> > +			if (ret < 0) {
> > +				/* TODO: rollback */
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [Severity: High]
> Could this missing rollback logic lead to a use-after-free or memory
> corruption if a later hardware breakpoint registration fails?

OK, to avoid memory corruption add a rollback logic here.

> [ ... ]
> 
> > +static int parse_address_spec(const char *spec, unsigned long *addr, int *type,
> > +			      int *len, char **symbol)
> > +{
> 
> [ ... ]
> 
> > +	if (kstrtoul(at + 1, 0, &_addr) != 0) {
> > +		char *off_str = strpbrk(at + 1, "+-");
> > +		int offset = 0;
> > +
> > +		if (off_str) {
> > +			if (kstrtoint(off_str, 0, &offset) != 0) {
> > +				trace_probe_log_err(off_str - _spec, BAD_PROBE_ADDR);
> > +				return -EINVAL;
> > +			}
> > +			*off_str = '\0';
> > +		}
> > +		_addr = kallsyms_lookup_name(at + 1);
> > +		if (!_addr) {
> > +			trace_probe_log_err(at + 1 - _spec, BAD_ACCESS_ADDR);
> > +			return -ENOENT;
> > +		}
> > +		_addr += offset;
> > +		*symbol = kstrdup(at + 1, GFP_KERNEL);
> > +		if (!*symbol)
> > +			return -ENOMEM;
> > +	}
> 
> [Severity: High]
> Could the null terminator injection here break dynamic_events save and
> restore functionality for events with symbol offsets?
> 
> When the event is created with a symbol offset (like symbol+8), *off_str
> is overwritten with a null byte. This truncates the string before it gets
> duplicated into *symbol.
> 
> Later, trace_wprobe_show() outputs the truncated base symbol without the
> offset, meaning the restored event will monitor the wrong memory address.

Since the symbol is used for making the event name, it should not have
offset.  However, in trace_wprobe_show(), the symbol is directly used
to show the target address. Thus, instead of changing
parse_address_spec(), we need to update trace_wprobe_show() to use
kallsyms_lookup_name() to find the symbol address and calculate the
offset from tw->addr and symbol address, and print out the "symbol+offset".

> 
> [ ... ]
> 
> > +static int __trace_wprobe_create(int argc, const char *argv[])
> > +{
> 
> [ ... ]
> 
> > +	char *symbol = NULL;
> > +	unsigned long addr;
> > +	int len, type, i;
> > +	int ret = 0;
> > +
> 
> [ ... ]
> 
> > +	trace_probe_log_set_index(1);
> > +	ret = parse_address_spec(argv[1], &addr, &type, &len, &symbol);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!event)
> > +		event = symbol ? symbol : "wprobe";
> > +
> > +	argc -= 2; argv += 2;
> > +	tw = alloc_trace_wprobe(group, event, symbol, addr, len, type, argc);
> > +	if (IS_ERR(tw))
> > +		return PTR_ERR(tw);
> 
> [Severity: Medium]
> Does this leak the dynamically allocated symbol string?
> 
> The symbol variable is populated with an allocated string from
> parse_address_spec(). When passed to alloc_trace_wprobe(), that function
> duplicates the string again:
> 
> alloc_trace_wprobe() {
>     ...
>     if (symbol) {
>         tw->symbol = kstrdup(symbol, GFP_KERNEL);
>     ...
> }
> 
> However, __trace_wprobe_create() never frees the original symbol string on
> either the success or error paths.

Yes, let me use __free() macro to free it.

> 
> [ ... ]
> 
> > +static int trace_wprobe_show(struct seq_file *m, struct dyn_event *ev)
> > +{
> > +	struct trace_wprobe *tw = to_trace_wprobe(ev);
> > +	int i;
> > +
> > +	seq_printf(m, "w:%s/%s", trace_probe_group_name(&tw->tp),
> > +		   trace_probe_name(&tw->tp));
> > +
> > +	char type_char;
> > +
> > +	if (tw->type == HW_BREAKPOINT_R)
> > +		type_char = 'r';
> > +	else if (tw->type == HW_BREAKPOINT_W)
> > +		type_char = 'w';
> > +	else
> > +		type_char = 'x'; /* Should be rw */
> 
> [Severity: High]
> Will outputting 'x' for HW_BREAKPOINT_RW cause parse_address_spec() to
> reject the configuration upon restore?
> 
> If type_char is set to 'x', the formatted string saved to dynamic_events
> will be invalid. When the system attempts to restore the event across
> reboots, parse_address_spec() checks strictly for 'r', 'w', or 'rw', and
> will fail with -EINVAL for 'x'.
> 
> Should this be adjusted to output a string like "rw" instead?

Ah, good catch. Let me use rw instead.

Thank you,

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