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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing infrastructure for dynamic address updates leaves checks as dead code.
- [Low] Misleading error message returned for malformed wprobe definitions.
- [Low] Comment refers to non-existent 'set_wprobe/clear_wprobe' triggers.
--

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

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

Add a new probe event for the hardware breakpoint called wprobe-event.
This wprobe allows user to trace (watch) the memory access at the
specified memory address.

> 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
> @@ -0,0 +1,746 @@

[ ... ]

> +static void wprobe_trace_handler(struct trace_wprobe *tw,
> +				 unsigned long addr,
> +				 struct pt_regs *regs,
> +				 struct trace_event_file *trace_file)
> +{
> +	struct wprobe_trace_entry_head *entry;
> +	struct trace_event_call *call = trace_probe_event_call(&tw->tp);
> +	struct trace_event_buffer fbuffer;
> +	int dsize;
> +
> +	if (WARN_ON_ONCE(call != trace_file->event_call))
> +		return;
> +
> +	if (trace_trigger_soft_disabled(trace_file))
> +		return;
> +
> +	if (READ_ONCE(tw->addr) != addr)
> +		return;

[Severity: Medium]
The changelog states this uses READ_ONCE(tw->addr) to safely check
dynamically updated addresses. Does this patch actually include the
infrastructure to update tw->addr dynamically?

It appears tw->addr is only set during initialization, and the SMP/IRQ
work update functions are absent. Does this make the check against addr
effectively constant?

> +
> +	dsize = __get_data_size(&tw->tp, (void *)addr, NULL);

[ ... ]

> +static int register_trace_wprobe_event(struct trace_wprobe *tw)
> +{
> +	struct trace_wprobe *old_tw;
> +	int ret;
> +
> +	guard(mutex)(&event_mutex);
> +
> +	old_tw = find_trace_wprobe(trace_probe_name(&tw->tp),
> +				   trace_probe_group_name(&tw->tp));
> +	if (old_tw) {
> +		/*
> +		 * Wprobe does not support sibling probes because the event
> +		 * trigger (set_wprobe/clear_wprobe) identifies the target
> +		 * wprobe by its event name. Having multiple wprobes sharing
> +		 * the same event name would make the target ambiguous.
> +		 */

[Severity: Low]
This isn't a bug, but the comment references 'set_wprobe/clear_wprobe'
triggers. Do these triggers actually exist in the tracing subsystem, or
should this refer to standard triggers like enable_event/disable_event?

> +		trace_probe_log_set_index(0);
> +		trace_probe_log_err(0, WPROBE_NO_SIBLING);
> +		return -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);
> +			return -EINVAL;
> +		}

[Severity: Low]
If a user provides an invalid prefix (like w10:event), this will return a
BAD_MAXACT_TYPE error.

Since hardware watchpoints do not support maxactive, will logging
"Maxactive is only for function exit" be misleading to users debugging
their probe definitions?

> +		event = &argv[0][2];
> +	}

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