Re: [PATCH v8 1/9] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing traceprobe_update_arg() call leaves @SYM fetch arguments unresolved, causing reads from address 0.
- [Medium] Sibling probe support is broken by a hardcoded uniqueness check, rendering the new rollback logic as dead code and limiting functionality.
- [Medium] The implementation fails to dynamically generate a unique event name for anonymous address-based watchpoints, contradicting the documentation.
- [Medium] Missing group and event name parsing allows invalid characters and breaks tracefs directory creation.
--

--- Patch [1]: [PATCH v8 1/9] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint ---
commit c7cef5689391cf83a545269b44666666b82afb31
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tracing: wprobe: Add watchpoint probe event based on hardware breakpoint

This commit adds a new wprobe-event for hardware breakpoints, allowing
users to trace memory accesses at specific addresses. It supports fetching
the accessed address and value, and can be configured via the dynamic_events
interface.

diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
new file mode 100644
--- /dev/null
+++ b/kernel/trace/trace_wprobe.c
[ ... ]
> static int register_trace_wprobe_event(struct trace_wprobe *tw)
> {
> 	struct trace_wprobe *old_tb;
> 	int ret;
> 
> 	guard(mutex)(&event_mutex);
> 
> 	old_tb = find_trace_wprobe(trace_probe_name(&tw->tp),
> 				   trace_probe_group_name(&tw->tp));
> 	if (old_tb)
> 		return -EBUSY;

[Severity: Medium]
Does this uniqueness check inadvertently break sibling probe support?

The commit message mentions rollback logic for already-enabled sibling
probes, but returning -EBUSY here unconditionally rejects adding a wprobe
to an existing event name. 

Shouldn't this call trace_probe_append() for existing events similar to how
kprobes and fprobes handle them, instead of returning -EBUSY?

[ ... ]
> static int __trace_wprobe_create(int argc, const char *argv[])
> {
[ ... ]
> 	if (argv[0][1] != '\0') {
> 		if (argv[0][1] != ':') {
> 			trace_probe_log_set_index(0);
> 			trace_probe_log_err(1, BAD_MAXACT_TYPE);
> 			/* Invalid format */
> 			return -EINVAL;
> 		}
> 		event = &argv[0][2];
> 	}

[Severity: Medium]
Is this missing a call to traceprobe_parse_event_name()?

By assigning the remainder of the command directly to the event name, it
bypasses separating the group from the event and checking against
is_good_name().

If a user provides a group like w:mygroup/myevent, could this cause tracefs
directory creation to fail or create a malformed structure?

> 
> 	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";

[Severity: Medium]
If multiple anonymous address-based watchpoints are created without a symbol
(for example, w@0x12345678 and w@0xcafebabe), won't they all be assigned
the hardcoded wprobe name?

Does this need to dynamically generate a unique name based on the address
(perhaps similar to what kprobes does with snprintf) to avoid naming
collisions?

> 
> 	argc -= 2; argv += 2;
> 	tw = alloc_trace_wprobe(group, event, symbol, addr, len, type, argc);
> 	if (IS_ERR(tw))
> 		return PTR_ERR(tw);
> 
> 	ctx = kzalloc_obj(*ctx);
> 	if (!ctx)
> 		return -ENOMEM;
> 
> 	ctx->flags = TPARG_FL_KERNEL | TPARG_FL_WPROBE;
> 
> 	/* parse arguments */
> 	for (i = 0; i < argc; i++) {
> 		trace_probe_log_set_index(i + 2);
> 		ctx->offset = 0;
> 		ret = traceprobe_parse_probe_arg(&tw->tp, i, argv[i], ctx);
> 		if (ret)
> 			return ret;	/* This can be -ENOMEM */
> 	}

[Severity: High]
Does this argument parsing loop need to call traceprobe_update_arg() to
resolve symbol references?

If a user registers a wprobe event with a symbol fetch argument like
@my_symbol, the parser generates a FETCH_NOP_SYMBOL instruction. Without
calling traceprobe_update_arg(&tw->tp.args[i]), the fetch instruction's
immediate address remains 0.

Won't this cause the probe to silently fail by attempting to read from
address 0 at runtime?

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