Re: [PATCH v12 04/11] tracing/wprobe: Add wprobe (watchpoint probe) trace event support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware breakpoints are not synchronized with module unloads, leading to dangling breakpoints on reallocated memory.
--

--- Patch [4]: [PATCH v12 04/11] tracing/wprobe: Add wprobe (watchpoint probe) trace event support ---
commit 6f99af1ed7b9527ff7af700fbd99ed2a3d0632d9
Author: Masami Hiramatsu (Google) <[email protected]>

tracing/wprobe: Add wprobe (watchpoint probe) trace event support

This commit adds wprobe, a hardware-breakpoint-based dynamic trace event.
It allows users to monitor memory accesses by setting hardware breakpoints
on specified addresses or symbols and logging the access context.

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
> new file mode 100644
> index 0000000000000..df592d9280a47
> --- /dev/null
> +++ b/kernel/trace/trace_wprobe.c
[ ... ]
> +static int __register_trace_wprobe(struct trace_wprobe *tw)
> +{
> +	struct perf_event_attr attr;
> +	int i, ret;
[ ... ]
> +	hw_breakpoint_init(&attr);
> +	attr.bp_addr = tw->addr;
> +	attr.bp_len = tw->len;
> +	attr.bp_type = tw->type;
> +
> +	tw->bp_event = register_wide_hw_breakpoint(&attr, wprobe_perf_handler, tw);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
When a watchpoint probe is created for a module symbol, does this leave a
dangling hardware breakpoint if the module is later unloaded?

Since the wprobe subsystem doesn't seem to register a module notifier
(unlike kprobes) to clean up when modules are unloaded, the breakpoint stays
active on the raw virtual address. If that memory is freed and subsequently
reallocated for a different module or vmalloc allocation, could this lead
to spurious trace events and unintended performance overhead on the reused
memory?

[ ... ]
> +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);

[Severity: Medium]
Is it possible to track the module lifecycle here when resolving the symbol?
Currently, kallsyms_lookup_name() simply translates the module symbol to an
address, but without pinning the module or reacting to unload events, the
hardware breakpoint loses its relationship to the original symbol once the
module disappears.

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