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) <[email protected]> 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.wangjinchao...@gmail.com/ > 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 [ ... ] > @@ -756,3 +827,501 @@ static __init int init_wprobe_trace(void) [ ... ] > +static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops, > + struct trace_event_file *file, > + char *glob, char *cmd, > + char *param_and_filter) > +{ [ ... ] > + } 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? 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. [ ... ] > + /* 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? 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. [ ... ] > +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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=9
