On Tue, 22 Sep 2026 04:43:24 +0000
[email protected] 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) <[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.wangjinchao...@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) <[email protected]>
