Re: [PATCH v17 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 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.

>  	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