Re: [RFC PATCH 08/13] mm/kwatch: add hardware breakpoint backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Repeated calls to `kwatch_hwbp_prealloc` write out of bounds of the `kwatch_wp_slots` array, leading to a heap buffer overflow.
- [High] CPU hotplug online callback overwrites active watchpoints with the dummy address, breaking them on newly onlined CPUs.
- [High] A static global per-CPU rate limit in `kwatch_hwbp_arm` silently drops arming requests, causing independent watchpoints to fail.
- [High] Missing IPI completion tracking for arm requests allows `csd_arm` to be reused while still pending, causing subsequent arm requests to silently fail.
- [High] `kwatch_hwbp_get` locklessly accesses `kwatch_wp_slots` while `kwatch_hwbp_free` frees it without synchronization.
- [Medium] Uninitialized kernel memory leaked to the trace ring buffer when the stack depth is less than KWATCH_STACK_DEPTH.
- [Medium] Function calls with side effects in `TP_printk` arguments cause trace buffer corruption.
- [Medium] `kwatch_hwbp_handler` logs the globally shared address instead of the local address, recording incorrect data during concurrent updates.
- [Low] 64-bit division in tracepoint printk causes linker errors on 32-bit architectures.
--

--- Patch [8]: [RFC PATCH 08/13] mm/kwatch: add hardware breakpoint backend ---
commit e30fa607776b2ca60d90cdf9ad4735ab9b2801f7
Author: Jinchao Wang <wangjinchao600@gmail.com>

mm/kwatch: add hardware breakpoint backend

Manage a preallocated pool of wide (per-CPU) perf hardware breakpoints.
All breakpoints are registered up front against a dummy address; arming
a watchpoint only re-points an already-registered event, so the arm path
can run from a kprobe handler.

> diff --git a/include/trace/events/kwatch.h b/include/trace/events/kwatch.h
> new file mode 100644
> index 0000000000000..edb95405c3869
> --- /dev/null
> +++ b/include/trace/events/kwatch.h
[ ... ]
> +	TP_fast_assign(
> +		unsigned int i;
> +
> +		__entry->ip = ip;
> +		__entry->sp = sp;
> +		__entry->addr = addr;
> +		__entry->time_ns = time_ns;
> +		__entry->stack_nr = min_t(unsigned int, stack_nr,
> +					  KWATCH_STACK_DEPTH);
> +		for (i = 0; i < __entry->stack_nr; i++)
> +			__entry->stack[i] = stack_entries[i];
> +	),

[Severity: Medium]
Will this leave the remaining elements in __entry->stack uninitialized when
stack_nr is less than KWATCH_STACK_DEPTH?

Because this memory comes from the tracing ring buffer, leaking uninitialized
kernel data might allow userspace to read stale kernel trace records.

> +
> +	TP_printk("KWatch HIT: time=%llu.%06lu ip=%pS addr=0x%lx%s",
> +		  __entry->time_ns / 1000000000ULL,
> +		  (unsigned long)((__entry->time_ns / 1000ULL) % 1000000ULL),

[Severity: Low]
Is it safe to use 64-bit division and modulo operators here?

Native 64-bit division on __entry->time_ns can cause build failures due to
unresolved __udivdi3 symbols on 32-bit architectures. Could div64_u64() or
do_div() be used instead?

> +		  (void *)__entry->ip, __entry->addr,
> +		  kwatch_trace_print_stack(p, __entry->stack,
> +					   __entry->stack_nr))
> +);

[Severity: Medium]
Can executing kwatch_trace_print_stack() as an argument to TP_printk()
corrupt the trace output buffer?

Due to argument evaluation order, kwatch_trace_print_stack() could evaluate
before trace_seq_printf(). Since it writes directly to the shared trace_seq
buffer and inserts a null byte:

    trace_seq_putc(p, 0);

trace_seq_printf() would append the main event string after the null byte.
When userspace reads the trace, it stops at the null byte, resulting in a
mangled or truncated trace log.

> diff --git a/mm/kwatch/hwbp.c b/mm/kwatch/hwbp.c
> new file mode 100644
> index 0000000000000..19498ba038260
> --- /dev/null
> +++ b/mm/kwatch/hwbp.c
[ ... ]
> +static void kwatch_hwbp_handler(struct perf_event *bp,
> +				struct perf_sample_data *data,
> +				struct pt_regs *regs)
> +{
> +	struct kwatch_watchpoint *wp = bp->overflow_handler_context;
> +	unsigned long stack_entries[KWATCH_STACK_DEPTH];
> +	unsigned int stack_nr;
> +
> +	if (!kwatch_probe_validate_hit(regs, wp->arm_tsk))
> +		return;
> +
> +	stack_nr = stack_trace_save_regs(regs, stack_entries, KWATCH_STACK_DEPTH, 2);
> +	trace_kwatch_hit(instruction_pointer(regs), kernel_stack_pointer(regs),
> +			 wp->attr.bp_addr, local_clock(),
> +			 stack_entries, stack_nr);
> +}

[Severity: Medium]
Could this tracepoint log the wrong address during a concurrent update?

If another CPU changes the watchpoint address, it updates the global
wp->attr.bp_addr and sends IPIs. If a hit on the old hardware breakpoint
triggers here before the IPI is received, it will log the newly updated
global wp->attr.bp_addr instead of the local perf event's configured
address (bp->attr.bp_addr).

[ ... ]
> +static int kwatch_hwbp_cpu_online(unsigned int cpu)
> +{
> +	struct perf_event_attr attr;
> +	struct kwatch_watchpoint *wp;
> +	struct perf_event *bp;
> +
> +	mutex_lock(&kwatch_all_wp_mutex);
> +	list_for_each_entry(wp, &kwatch_all_wp_list, list) {
> +		attr = wp->attr;
> +		attr.bp_addr = (unsigned long)&kwatch_dummy_holder;

[Severity: High]
Does this accidentally force newly onlined CPUs to monitor the dummy address
instead of the active watchpoint address?

If a CPU is onlined on a system with active watchpoints, they will silently
fail to trigger on the new CPU because it explicitly configures the hardware
breakpoint for kwatch_dummy_holder instead of the active wp->attr.bp_addr.

[ ... ]
> +int kwatch_hwbp_get(struct kwatch_watchpoint **out_wp)
> +{
> +	struct kwatch_watchpoint *wp;
> +	int i;
> +
> +	/*
> +	 * Per-slot cmpxchg claim: safe for concurrent consumers on any CPU,
> +	 * unlike llist_del_first() which requires a single consumer.
> +	 */
> +	for (i = 0; i < kwatch_wp_nr; i++) {
> +		wp = kwatch_wp_slots[i];

[Severity: High]
Is it safe to access kwatch_wp_slots here without holding a lock or
using RCU?

If an error occurs during preallocation or a module unload triggers
kwatch_hwbp_free(), the array can be freed:

    kwatch_wp_nr = 0;
    kfree(kwatch_wp_slots);

If kwatch_hwbp_get() reads kwatch_wp_nr before it is zeroed and dereferences
the array after it is freed, it could lead to a use-after-free.

[ ... ]
> +void kwatch_hwbp_arm(struct kwatch_watchpoint *wp, unsigned long addr, u16 len)
> +{
> +	static DEFINE_PER_CPU(u64, last_ipi_time);
> +	int cur_cpu;
> +	call_single_data_t *csd;
> +	int cpu;
> +	bool is_disarm = (addr == (unsigned long)&kwatch_dummy_holder);
> +
> +	wp->attr.bp_addr = addr;
> +	wp->attr.bp_len = len;
> +
> +	if (!is_disarm)
> +		wp->arm_tsk = current;
> +
> +	/* ensure attr update visible to other cpu before sending IPI */
> +	smp_wmb();
> +
> +	atomic_set(&wp->pending_ipis, 1);
> +	cur_cpu = get_cpu();
> +
> +	if (!is_disarm) {
> +		u64 now = local_clock();
> +		u64 last = this_cpu_read(last_ipi_time);
> +
> +		if (now - last < 1000000ULL) {
> +			put_cpu();
> +			return;
> +		}

[Severity: High]
Could this static global per-CPU rate limit silently break watchpoints?

If two distinct watchpoints are armed on the same CPU within 1 millisecond,
the second request will return early here. However, this happens after the
global wp->attr.bp_addr and wp->arm_tsk have already been modified.

This leaves the hardware monitoring the old address while the metadata claims
it is armed, causing the new watchpoint to miss events.

> +		this_cpu_write(last_ipi_time, now);
> +	}
> +	for_each_online_cpu(cpu) {
> +		if (cpu == cur_cpu)
> +			continue;
> +
> +		if (is_disarm)
> +			atomic_inc(&wp->pending_ipis);
> +
> +		csd = per_cpu_ptr(is_disarm ? wp->csd_disarm : wp->csd_arm,
> +				  cpu);
> +		if (smp_call_function_single_async(cpu, csd) && is_disarm)
> +			kwatch_hwbp_try_recycle(wp);
> +	}

[Severity: High]
Should arm requests also handle the case where the IPI fails with -EBUSY?

If a watchpoint is rapidly toggled, the second arm attempt can try to queue
csd_arm while the remote CPU is still processing the first one. This causes
smp_call_function_single_async() to return -EBUSY.

Because the error is only checked for disarm requests (the && is_disarm
check), the failure to arm is silently ignored, and the new address is never
programmed on the busy CPU.

[ ... ]
> +int kwatch_hwbp_prealloc(u16 max_watch, enum kwatch_access_type access_type)
> +{
[ ... ]
> +	kwatch_wp_slots = kcalloc(success, sizeof(*kwatch_wp_slots),
> +				  GFP_KERNEL);
> +	if (!kwatch_wp_slots) {
> +		kwatch_hwbp_free();
> +		return -ENOMEM;
> +	}
> +	mutex_lock(&kwatch_all_wp_mutex);
> +	list_for_each_entry(wp, &kwatch_all_wp_list, list)
> +		kwatch_wp_slots[kwatch_wp_nr++] = wp;

[Severity: Critical]
Will subsequent calls to kwatch_hwbp_prealloc() cause a heap buffer
overflow?

If this function is called more than once, it allocates a new
kwatch_wp_slots array sized only for the new success count. However, the
static kwatch_all_wp_list retains elements from prior calls, and the static
kwatch_wp_nr is never reset.

Iterating the accumulated global list and writing elements via
kwatch_wp_slots[kwatch_wp_nr++] will write out of bounds of the newly
allocated array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=8