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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Tue, 22 Sep 2026 04:43:24 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] tracing/wprobe: NMI deadlock in wprobe_trigger() when count is not specified
> - [High] tracing/wprobe: CPU hotplug race in __register_trace_wprobe() misses new CPUs
> - [Medium] tracing/wprobe: TOCTOU race in wprobe_trigger() corrupts watchpoint address
> --
> 
> commit b9f67542e6bf772512bb8cbf3c3011379c301eb4
> 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..f1d6aa320f213 100644
> > --- a/kernel/trace/trace_wprobe.c
> > +++ b/kernel/trace/trace_wprobe.c
> > @@ -193,20 +217,130 @@ static int __register_trace_wprobe(struct trace_wprobe *tw)
> 
> [ ... ]
> 
> >  	tw->bp_event = register_wide_hw_breakpoint(&attr, wprobe_perf_handler, tw);
> >  	if (IS_ERR_PCPU(tw->bp_event)) {
> >  		int ret = PTR_ERR_PCPU(tw->bp_event);
> >  
> >  		tw->bp_event = NULL;
> >  		return ret;
> >  	}
> >  
> > +	mutex_lock(&wprobe_mutex);
> > +	list_add(&tw->active_list, &active_wprobes);
> > +	mutex_unlock(&wprobe_mutex);
> > +
> 
> [Severity: High]
> Does this code have a race condition with CPU hotplug?
> 
> Looking at __register_trace_wprobe(), it calls register_wide_hw_breakpoint()
> which installs breakpoints and releases the hotplug lock internally.
> 
> If a new CPU comes online immediately after that lock is released, but before
> this wprobe is added to active_wprobes under wprobe_mutex, could the
> wprobe_cpu_online() callback miss the wprobe? This would leave the new CPU
> without hardware breakpoints installed for this probe.

Hmm, OK. Taking a cpus_read_lock() for both can avoid this issue.

> 
> >  	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -767,9 +923,608 @@ static int trace_wprobe_show(struct seq_file *m, struct dyn_event *ev)
> 
> [ ... ]
> 
> > +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 target_addr, addr = 0, flags;
> > +
> > +	if (wprobe_data->field) {
> > +		if (!rec)
> > +			return;
> > +		addr = *(unsigned long *)((char *)rec + wprobe_data->offset);
> > +		addr += wprobe_data->adjust;
> > +	}
> > +
> > +	raw_spin_lock_irqsave(&tw->lock, flags);
> > +
> 
> [Severity: High]
> Could this raw_spin_lock_irqsave() cause a deadlock if interrupted by an NMI?
> 
> When no count limit is specified (EVENT_TRIGGER_FL_COUNT is not set),
> wprobe_trigger() takes tw->lock before calling __wprobe_count_func().
> 
> If an NMI interrupts a CPU already holding tw->lock and hits a trace event
> that fires the same uncounted trigger, it will attempt to acquire tw->lock
> again here. Since the in_nmi() check is located inside __wprobe_count_func(),
> the NMI would spin forever before realizing it shouldn't proceed.

Ah, indeed. It should check in_nmi() earlier than the spin lock.

> 
> > +	if (!(data->flags & EVENT_TRIGGER_FL_COUNT)) {
> > +		if (!__wprobe_count_func(data, addr)) {
> > +			atomic_inc(&tw->missed);
> > +			goto out;
> > +		}
> > +	}
> 
> [Severity: Medium]
> Is there a time-of-check to time-of-use (TOCTOU) race here for counted
> triggers?
> 
> For counted triggers, wprobe_count_func() is called locklessly by the trace
> core, which evaluates __wprobe_count_func(). Then, wprobe_trigger() is called
> and takes tw->lock. Because EVENT_TRIGGER_FL_COUNT is set, the state
> validation check (__wprobe_count_func()) is explicitly skipped under the lock.
> 
> If a counted trigger fires concurrently on multiple CPUs, both might
> observe the unset address locklessly. Could the second CPU unconditionally
> overwrite tw->addr, breaking the exclusive trigger constraint?

Hmm, indeed. I think the current event trigger subsystem is not well
designed for the concurrency. Let me move the data->count update to
wprobe_trigger() inside tw->lock critical region.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>