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) <[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.wangjinchao600@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(&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