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. > 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. > + 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/179005108298.388919.4535333252892590932.stgit@devnote2?part=10
