Re: [PATCH v8 8/9] tracing: wprobe: Add wprobe event trigger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uncancelled asynchronous work causes NULL pointer dereference when disabling wprobes.
- [High] State corruption in wprobe_trigger_cmd_parse when registering clear_wprobe.
- [High] Use-After-Free in wprobe triggers due to incorrect RCU grace period handling.
- [Medium] Memory leak of trigger_data->filter on error path in wprobe_trigger_cmd_parse.
- [Low] Build failure due to static declaration of event_trigger_free.
--

--- Patch [8]: [PATCH v8 8/9] tracing: wprobe: Add wprobe event trigger ---
commit aadb441dbb3ae8cb3459288e9388e6fa92216b82
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date:   Thu Jul 16 02:53:46 2026 +0000

    tracing: wprobe: Add wprobe event trigger
    
    Add wprobe event trigger to set and clear the watch event dynamically.
    This allows us to set an watchpoint on a given local variables and
    a slab object instead of static objects.

> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 2f07c5c4ffc87..b4f964839350f 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1982,6 +1982,7 @@ trigger_data_alloc(struct event_command *cmd_ops, char *cmd, char *param,
>  		   void *private_data);
>  extern void trigger_data_free(struct event_trigger_data *data);
>  extern int event_trigger_init(struct event_trigger_data *data);
> +extern void event_trigger_free(struct event_trigger_data *data);

[Severity: Low]
Does this code build successfully when CONFIG_WPROBE_TRIGGERS is enabled?

The patch declares event_trigger_free() as extern here and calls it from
trace_wprobe.c, but its definition in kernel/trace/trace_events_trigger.c
remains static.

>  extern int trace_event_trigger_enable_disable(struct trace_event_file *file,
>  					      int trigger_enable);
>  extern void update_cond_flag(struct trace_event_file *file);
> diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> index 942ad2810d369..69ad48ac210c6 100644
> --- a/kernel/trace/trace_wprobe.c
> +++ b/kernel/trace/trace_wprobe.c
[ ... ]
> @@ -197,9 +208,42 @@ static void __unregister_trace_wprobe(struct trace_wprobe *tw)
>  	}
>  }
>  
> +static int trace_wprobe_update_local(struct trace_wprobe *tw, unsigned long addr)
> +{
> +	struct perf_event *bp = *this_cpu_ptr(tw->bp_event);

[Severity: High]
If a wprobe trigger has recently fired, tw->irq_work and tw->work may be queued.
If a user disables the wprobe, __unregister_trace_wprobe() clears tw->bp_event
to NULL.

When tw->work runs concurrently and calls trace_wprobe_update_local(), will
this_cpu_ptr(tw->bp_event) calculate a garbage per-CPU offset from NULL and cause
a panic when dereferencing bp?

Should the pending work be cancelled when the wprobe is unregistered?

> +	struct perf_event_attr attr = bp->attr;
> +
> +	attr.bp_addr = addr;
> +	return modify_wide_hw_breakpoint_local(bp, &attr);
> +}
[ ... ]
> +static void wprobe_trigger_free(struct event_trigger_data *data)
> +{
> +	struct wprobe_trigger_data *wprobe_data = data->private_data;
> +
> +	if (WARN_ON_ONCE(data->ref <= 0))
> +		return;
> +
> +	data->ref--;
> +	if (!data->ref) {
> +		/* Remove the SOFT_MODE flag */
> +		trace_event_enable_disable(wprobe_data->file, 0, 1);
> +		trace_event_put_ref(wprobe_data->file->event_call);
> +		trigger_data_free(data);
> +
> +		/* Wait for RCU readers (tracepoint executions) to finish */
> +		call_rcu(&wprobe_data->rcu, wprobe_trigger_data_rcu_free);

[Severity: High]
Since triggers can be attached to faultable syscall tracepoints (which execute
in a sleepable RCU Tasks Trace context), is call_rcu() sufficient here?

If a sleeping tracepoint reader is iterating triggers when this unregistration
occurs, the normal RCU grace period will expire and wprobe_data will be freed.
When the tracepoint wakes up and executes wprobe_trigger(), won't it dereference
the freed wprobe_data?

Should this use tracepoint_synchronize_unregister() to safely wait for all
tracepoint contexts?

> +	}
> +}
> +
> +static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,
> +				    struct trace_event_file *file,
> +				    char *glob, char *cmd,
> +				    char *param_and_filter)
> +{
[ ... ]
> +	ret = event_trigger_set_filter(cmd_ops, file, filter, trigger_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Soft-enable (register) wprobe event on WPROBE_DEFAULT_CLEAR_ADDRESS */
> +	if (!trace_event_try_get_ref(wprobe_file->event_call))
> +		return -ENODEV;

[Severity: Medium]
If trace_event_try_get_ref() fails, this returns -ENODEV directly.

Since event_trigger_reset_filter() is bypassed on this error path, does this
leak the dynamically allocated filter memory?

> +
> +	WRITE_ONCE(tw->addr, WPROBE_DEFAULT_CLEAR_ADDRESS);

[Severity: High]
Registering a clear_wprobe trigger on an already-active wprobe is permitted.

However, this unconditionally overwrites tw->addr with
WPROBE_DEFAULT_CLEAR_ADDRESS. When the clear_wprobe trigger fires, it checks
if tw->addr != WPROBE_DEFAULT_CLEAR_ADDRESS.

Since it has already been set to WPROBE_DEFAULT_CLEAR_ADDRESS here, will this
prevent the clear trigger from actually clearing the hardware breakpoints,
thereby corrupting the wprobe state and permanently leaking hardware breakpoints?

> +	ret = trace_event_enable_disable(wprobe_file, 1, 1);
> +	if (ret < 0) {
> +		trace_event_put_ref(wprobe_file->event_call);
> +		event_trigger_reset_filter(cmd_ops, trigger_data);
> +		return ret;
> +	}
> +	ret = event_trigger_register(cmd_ops, file, glob, trigger_data);
> +	if (ret) {
> +		event_trigger_reset_filter(cmd_ops, trigger_data);
> +		trace_event_enable_disable(wprobe_file, 0, 1);
> +		trace_event_put_ref(wprobe_file->event_call);
> +		synchronize_rcu();

[Severity: High]
Similar to call_rcu() above, is synchronize_rcu() sufficient here for
tracepoints that rely on RCU Tasks Trace?

> +		return ret;
> +	}

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