Re: [PATCH v13 09/12] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers

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)

[ ... ]

[ ... ]

+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(&param, ":");

[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