Re: [PATCH v13 09/12] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date: 2026-08-30 05:35:58
Also in:
linux-perf-users, sashiko-reviews
On Sat, 22 Aug 2026 09:42:17 +0000 sashiko-bot@kernel.org wrote:
quoted hunk ↗ jump to hunk
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] Leaked `trigger_data` and `wprobe_data` when trigger registration fails due to unbalanced reference counts. - [Medium] Hardware breakpoints are inadvertently left monitoring the initial wprobe address (unparked) instead of the parked dummy address upon enablement. - [Low] The `clear_wprobe` trigger cannot be removed by echoing its exact printed output if a trigger count is specified. ----- Patch [9]: [PATCH v13 09/12] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers ---commit e794ff7412c73348b335e9a50aceecbbd82d9a0c Author: Masami Hiramatsu (Google) [off-list ref] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers Add 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/ (local)quoted
diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c index a8543766a245b..acb15fd018e80 100644 --- a/kernel/trace/trace_wprobe.c +++ b/kernel/trace/trace_wprobe.c[ ... ]quoted
@@ -756,3 +827,501 @@ static __init int init_wprobe_trace(void)[ ... ]quoted
+static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops, + struct trace_event_file *file, + char *glob, char *cmd, + char *param_and_filter) +{[ ... ]quoted
+ } else if (param && (isalpha(param[0]) || param[0] == '_')) { + char *offs; + + field_str = strsep(¶m, ":");[Severity: Low] Does this code prevent removing the clear_wprobe trigger if it has a count but no field?
Ah, indeed. The trigger shows ":count=N" or ":unlimited" for counter instead of digit value.
If the user sets a count on a clear_wprobe trigger, wprobe_trigger_print()
outputs the trigger as ending with ":count=N". When attempting to remove it
by writing the exact string prepended with '!', this parser checks if the
parameter starts with a letter.
Since "count=N" starts with a letter ('c'), it treats it as an event field
name, and fails with -ENOENT in wprobe_trigger_field_parse(), preventing the
trigger from being unregistered.OK, let me fix it.
[ ... ]quoted
+ /* Soft-enable (register) wprobe event on WPROBE_DEFAULT_CLEAR_ADDRESS */ + if (!trace_event_try_get_ref(wprobe_file->event_call)) { + ret = -ENODEV; + goto out_free_trigger; + } + + ret = trace_event_enable_disable(wprobe_file, 1, 1); + if (ret < 0) + goto out_put; + + if (!clear) + WRITE_ONCE(tw->addr, WPROBE_DEFAULT_CLEAR_ADDRESS);[Severity: Medium] Should we update the target address before soft-enabling the event?
Ah, indeed. We also need to roll it back if soft-enable fails.
The documentation suggests initializing a new wprobe at address 0 and keeping it disabled. When we call trace_event_enable_disable(wprobe_file, 1, 1) here, it installs hardware breakpoints monitoring the current address (e.g., 0). The parked dummy address WPROBE_DEFAULT_CLEAR_ADDRESS is only assigned to tw->addr afterwards, which means the hardware breakpoints are left actively monitoring address 0 and could generate spurious debug exceptions. [ ... ]quoted
+out_disable: + trace_event_enable_disable(wprobe_file, 0, 1); +out_put: + trace_event_put_ref(wprobe_file->event_call); +out_free_trigger: + event_trigger_reset_filter(cmd_ops, trigger_data); + event_trigger_free(trigger_data); + return ret;[Severity: Medium] Can this leak trigger_data and wprobe_data on the error path? When wprobe_trigger_cmd_parse() executes, it increments the trigger_data refcount to 1 via event_trigger_init(). It then calls event_trigger_register() which calls wprobe_register_trigger() and increments the refcount to 2 via data->cmd_ops->init(data). If trace_event_trigger_enable_disable() fails inside wprobe_register_trigger(), it returns an error without balancing the reference count: kernel/trace/trace_wprobe.c:wprobe_register_trigger() { ... ret = trace_event_trigger_enable_disable(file, 1); if (ret < 0) { list_del_rcu(&data->list); update_cond_flag(file); } return ret; } The error path here then calls event_trigger_free(trigger_data), which only decrements the refcount from 2 to 1 and skips freeing the objects.
Ah, OK. It should call the data->cmd_ops->free() in failure path. Thanks,
-- Sashiko AI review · https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=9
-- Masami Hiramatsu (Google) [off-list ref]