Re: [PATCH v16 10/13] tracing/wprobe: Add set_wprobe and clear_wprobe event triggers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing RCU synchronization in the error path of wprobe trigger registration leads to a Use-After-Free of hardware breakpoint perf_event structures.
- [High] Hardware breakpoint addresses are not updated for offline CPUs, leading to stale addresses when CPUs come online.
- [Medium] The 'count' parameter is decremented on trigger evaluation misses, causing the trigger to exhaust its count without actually modifying the watchpoint.
- [Medium] The wprobe event permanently leaks as enabled when the trigger is unregistered because `EVENT_FILE_FL_SOFT_DISABLED_BIT` is manually cleared.
- [Low] A field named 'unlimited' is silently ignored by `clear_wprobe` if no count keyword is provided.
--

commit 146ca411781d7203b266aa3745d30641c87b97d2
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

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 9c6100b63e8d9..55a201504bb0b 100644
> --- a/kernel/trace/trace_wprobe.c
> +++ b/kernel/trace/trace_wprobe.c

[ ... ]

> @@ -231,6 +303,12 @@ static struct trace_wprobe *alloc_trace_wprobe(const char *group,
> 
> +static void wprobe_work_func(struct work_struct *work)
> +{
> +	struct trace_wprobe *tw = container_of(work, struct trace_wprobe, work);
> +
> +	on_each_cpu(wprobe_smp_update_func, tw, true);

[Severity: High]
Could this leave hardware breakpoints with stale addresses on offline CPUs?

on_each_cpu() only broadcasts to currently online CPUs, so offline CPUs
won't execute trace_wprobe_update_local(). When an offline CPU comes
online, won't the perf event core reinstall the hardware breakpoint using
the old, stale address since it wasn't updated here?

[ ... ]

> @@ -773,3 +866,538 @@ static __init int init_wprobe_trace(void)

[ ... ]

> +static void wprobe_trigger(struct event_trigger_data *data,
> +			   struct trace_buffer *buffer,  void *rec,
> +			   struct ring_buffer_event *event)
> +{
> +	struct wprobe_trigger_data *wprobe_data = data->private_data;
> +	struct trace_wprobe *tw = wprobe_data->tw;
> +	unsigned long addr = 0, flags;
> +	bool changed = false;

[ ... ]

> +	raw_spin_lock_irqsave(&tw->lock, flags);
> +
> +	if (!wprobe_data->clear) {
> +		if (!trace_wprobe_is_valid_addr(addr, tw->len)) {
> +			atomic_inc(&tw->missed);
> +			goto out;
> +		}
> +		if (tw->addr == WPROBE_DEFAULT_CLEAR_ADDRESS) {
> +			/* Skip if a previous work is still propagating the address */
> +			if (atomic_read(&tw->work_pending)) {
> +				atomic_inc(&tw->missed);
> +				goto out;
> +			}
> +			WRITE_ONCE(tw->addr, addr);
> +			changed = true;
> +			clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &wprobe_data->file->flags);

[Severity: Medium]
Does manually clearing EVENT_FILE_FL_SOFT_DISABLED_BIT cause the event to leak
as enabled?

When the trigger is unregistered, wprobe_trigger_free() calls
trace_event_enable_disable(file, 0, 1). Because the soft-disabled bit was
cleared here, won't the core event logic skip the unregister step, leaving
the wprobe tracepoint permanently enabled in the kernel?

[ ... ]

> +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] == '_')) {
> +		if (strncmp(param, "count=", 6) != 0 &&
> +		    strcmp(param, "unlimited") != 0) {

[Severity: Low]
Is this intended to skip fields named "unlimited"?

If a user configures a clear_wprobe trigger on an event that happens to have
a field named "unlimited", this parser matches the keyword and skips parsing
it as a field, which defaults to an unconditional clear action.

[ ... ]

> +static int wprobe_register_trigger(char *glob,
> +				   struct event_trigger_data *data,
> +				   struct trace_event_file *file)
> +{
> +	int ret = 0;
> +
> +	lockdep_assert_held(&event_mutex);
> +
> +	/* The same wprobe is not accept on the same file (event) */
> +	if (wprobe_trigger_find_same(data, file))
> +		return -EEXIST;
> +
> +	if (data->cmd_ops->init) {
> +		ret = data->cmd_ops->init(data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	list_add_rcu(&data->list, &file->triggers);
> +
> +	update_cond_flag(file);
> +	ret = trace_event_trigger_enable_disable(file, 1);
> +	if (ret < 0) {
> +		list_del_rcu(&data->list);
> +		update_cond_flag(file);
> +		if (data->cmd_ops->free)
> +			data->cmd_ops->free(data);
> +	}

[Severity: High]
Does this error path need RCU synchronization?

If trace_event_trigger_enable_disable() fails, list_del_rcu() is called
without a subsequent synchronize_rcu(). The cmd_ops->free callback
(wprobe_trigger_free) will then synchronously disable the wprobe, which
frees the tw->bp_event array.

Could a concurrent tracepoint reader still observe the trigger data,
execute wprobe_trigger(), and dereference the freed tw->bp_event via
this_cpu_ptr() inside trace_wprobe_update_local()?

[ ... ]

> +static struct event_command trigger_wprobe_set_cmd = {
> +	.name			= SET_WPROBE_STR,
> +	.trigger_type		= ETT_EVENT_WPROBE,
> +	/* This triggers after when the event is recorded. */
> +	.flags			= EVENT_CMD_FL_NEEDS_REC,
> +	.parse			= wprobe_trigger_cmd_parse,
> +	.reg			= wprobe_register_trigger,
> +	.unreg			= wprobe_unregister_trigger,
> +	.set_filter		= set_trigger_filter,
> +	.trigger		= wprobe_trigger,
> +	.count_func		= event_trigger_count,

[Severity: Medium]
Will this unconditionally exhaust the trigger's count parameter even on misses?

Because .count_func is set to event_trigger_count, the count is decremented
before wprobe_trigger() runs. If the trigger fails its implicit checks inside
wprobe_trigger() (such as checking if the field matches the watched address),
the count has already been consumed.

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