Re: [RFC PATCH v2 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:
- [High] Modifying the trace_seq buffer directly during TP_printk argument evaluation prepends the stack trace and corrupts the trace output.
- [High] Race condition in CPU offline callback leading to Use-After-Free in the IPI handler.
- [High] Concurrent read and write of the shared `wp->attr` structure across CPUs can result in torn reads and invalid hardware breakpoint configuration.
- [High] Untracked csd_arm IPIs can execute after the watchpoint is freed if the target CPU goes offline.
- [High] The `wp->arm_tsk` pointer is assigned to `current` without holding a reference, causing a Use-After-Free if the task exits.
- [High] Missing NULL check on `register_wide_hw_breakpoint` return value causes a kernel panic if hardware breakpoints are unsupported.
- [Medium] Newly onlined CPUs are unconditionally forced to watch the dummy address, missing hits if the watchpoint is currently active.
- [Medium] Per-CPU throttling of arm IPIs causes unrelated watchpoints to be silently ignored on remote CPUs.
- [Medium] A race gap between `register_wide_hw_breakpoint` and `cpuhp_setup_state_nocalls` drops watchpoints on concurrently hotplugged CPUs.
--

--- Patch [8]: [RFC PATCH v2 08/13] mm/kwatch: add hardware breakpoint backend ---
commit 6c7794f69c285985dbf3f0583ef594237690a973
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
> index 0000000000000..8a2ec6811ad49
> --- /dev/null
> +++ b/include/trace/events/kwatch.h

[ ... ]

> +	TP_printk("KWatch HIT: time=%llu.%06u ip=%pS addr=0x%lx%s",
> +		  div_u64(__entry->time_ns, 1000000000ULL),
> +		  (unsigned int)(div_u64(__entry->time_ns, 1000ULL) % 1000000ULL),
> +		  (void *)__entry->ip, __entry->addr,
> +		  kwatch_trace_print_stack(p, __get_dynamic_array(stack),
> +					   __entry->stack_nr))

[Severity: High]
Does evaluating kwatch_trace_print_stack() as a TP_printk() argument modify
the trace_seq buffer prematurely?

Because C argument evaluation order is unsequenced, calling trace_seq_printf()
directly inside kwatch_trace_print_stack() mutates the trace_seq buffer p
before or while the main format string is processed.

This causes the output to be written out-of-order or truncated.

> diff --git a/mm/kwatch/hwbp.c b/mm/kwatch/hwbp.c
> index 0000000000000..d1e93754cce82
> --- /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;

[Severity: High]
Does this code dereference a freed task_struct if the arming task exits?

When armed, wp->arm_tsk is assigned to current without taking a reference
count. If the task exits, a subsequent breakpoint hit will dereference a
freed task, leading to a use-after-free and kernel panic.

[ ... ]

> +static void kwatch_hwbp_arm_local(void *info)
> +{
> +	struct kwatch_watchpoint *wp = info;
> +	struct perf_event *bp;
> +	unsigned long flags;
> +	int cpu, err;
> +
> +	local_irq_save(flags);
> +
> +	cpu = smp_processor_id();
> +	bp = per_cpu(*wp->event, cpu);

[Severity: High]
Can this dereference a freed perf_event or a NULL pointer?

If a concurrent CPU offline event unregisters and frees the perf_event, the
per-CPU pointer might be read here before it is nullified in
kwatch_hwbp_cpu_offline().

Additionally, if register_wide_hw_breakpoint() returned NULL during
initialization (which IS_ERR_PCPU does not catch), this will attempt to
access the NULL pointer.

> +
> +	if (unlikely(!bp))
> +		goto out;
> +
> +	kwatch_probe_mute(true);
> +	barrier();
> +
> +	/*
> +	 * On success this also updates the per-CPU bp->attr, so the hit
> +	 * handler reports what THIS CPU is watching instead of the shared
> +	 * wp->attr, which another CPU may be re-pointing.
> +	 */
> +	err = modify_wide_hw_breakpoint_local(bp, &wp->attr);

[Severity: High]
Can this read a torn or partially updated wp->attr structure?

The shared wp->attr structure is modified by kwatch_hwbp_arm() on another
CPU without locking or WRITE_ONCE(). Remote CPUs executing this IPI
concurrently can observe torn reads, leading to an invalid hardware
breakpoint configuration.

[ ... ]

> +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: Medium]
Will this unconditional override cause a newly onlined CPU to miss active hits?

If a watchpoint is currently armed and active, bringing a CPU online forces
the new CPU to watch the dummy address instead of the currently watched
address.

[ ... ]

> +static int kwatch_hwbp_cpu_offline(unsigned int cpu)
> +{
> +	struct kwatch_watchpoint *wp;
> +	struct perf_event *bp;
> +
> +	mutex_lock(&kwatch_all_wp_mutex);
> +	list_for_each_entry(wp, &kwatch_all_wp_list, list) {
> +		bp = per_cpu(*wp->event, cpu);
> +		if (bp) {
> +			unregister_hw_breakpoint(bp);
> +			per_cpu(*wp->event, cpu) = NULL;

[Severity: High]
Does unregistering the breakpoint before setting the pointer to NULL create a
race condition?

If an IPI broadcast from kwatch_hwbp_arm() occurs concurrently, it can read
the stale pointer in kwatch_hwbp_arm_local() and access the freed memory.

> +		}
> +	}
> +	mutex_unlock(&kwatch_all_wp_mutex);
> +	return 0;
> +}

[ ... ]

> +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);
> +	bool skip_remote = false;
> +
> +	wp->attr.bp_addr = addr;
> +	wp->attr.bp_len = len;

[Severity: High]
Can mutating these shared fields concurrently cause torn reads?

Because wp->attr is modified without locking or WRITE_ONCE(), remote CPUs
processing the arm IPI via modify_wide_hw_breakpoint_local() might read
partially updated states.

> +
> +	if (!is_disarm)
> +		wp->arm_tsk = current;

[Severity: High]
Can this result in a use-after-free on current?

No reference count is taken when storing the current task pointer. If the
task exits, a subsequent breakpoint hit will dereference a freed task.

> +
> +	/* ensure attr update visible to other cpu before sending IPI */
> +	smp_wmb();
> +
> +	atomic_set(&wp->pending_ipis, 1);
> +	cur_cpu = get_cpu();
> +
> +	/*
> +	 * Rate-limit only the cross-CPU broadcast, never the local re-point.
> +	 * Arming the current CPU is free and must always reflect this window;
> +	 * only the remote IPI fan-out is throttled to keep a hot function from
> +	 * storming every CPU. A suppressed broadcast means remote CPUs keep
> +	 * watching the previous address for that window (a missed remote-CPU
> +	 * writer is possible) - hence the visible counter, and why kwatch
> +	 * targets low-frequency functions. Disarm is never throttled: the
> +	 * slot must always be released.
> +	 */
> +	if (!is_disarm) {
> +		u64 now = local_clock();
> +		u64 last = this_cpu_read(last_ipi_time);
> +
> +		if (now - last < KWATCH_ARM_IPI_MIN_INTERVAL_NS) {
> +			atomic_long_inc(&kwatch_arm_ipi_suppressed);
> +			skip_remote = true;

[Severity: Medium]
Will this per-CPU throttling drop arm IPIs for entirely independent watchpoints?

Since last_ipi_time is stored per-CPU rather than per-watchpoint, arming two
distinct watchpoints in rapid succession from the same CPU will cause the
second watchpoint to be silently ignored on remote CPUs.

> +		} else {
> +			this_cpu_write(last_ipi_time, now);
> +		}
> +	}
> +
> +	if (!skip_remote) {
> +		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);

[Severity: High]
Can leaving arm IPIs untracked result in a use-after-free?

Because only disarm IPIs are tracked by pending_ipis, an active csd_arm IPI
might still be queued on an offline-going CPU when kwatch_hwbp_free() is
called and frees the csd_arm structure.

[ ... ]

> +void kwatch_hwbp_free(void)
> +{
> +	struct kwatch_watchpoint *wp, *tmp;
> +
> +	kwatch_wp_nr = 0;
> +	kfree(kwatch_wp_slots);
> +	kwatch_wp_slots = NULL;
> +
> +	if (kwatch_hwbp_cpuhp_state != CPUHP_INVALID) {
> +		cpuhp_remove_state_nocalls(kwatch_hwbp_cpuhp_state);
> +		kwatch_hwbp_cpuhp_state = CPUHP_INVALID;
> +	}
> +
> +	mutex_lock(&kwatch_all_wp_mutex);
> +	list_for_each_entry_safe(wp, tmp, &kwatch_all_wp_list, list) {
> +		list_del(&wp->list);
> +
> +		WRITE_ONCE(wp->teardown, true);
> +		atomic_dec(&wp->refcount);
> +
> +		/* Wait for all async IPIs to finish */
> +		while (atomic_read(&wp->refcount) > 0)
> +			cpu_relax();
> +
> +		unregister_wide_hw_breakpoint(wp->event);
> +		free_percpu(wp->csd_arm);

[Severity: High]
Is it safe to free csd_arm here while untracked arm IPIs might still be pending?

Since csd_arm IPIs are not tracked by pending_ipis, a delayed or queued
arm IPI on an offline-going CPU could access the freed csd_arm.

[ ... ]

> +int kwatch_hwbp_prealloc(u16 max_watch)
> +{
> +	struct kwatch_watchpoint *wp;
> +	int success = 0, cpu;
> +	int ret;

[ ... ]

> +		wp->event = register_wide_hw_breakpoint(&wp->attr,
> +							kwatch_hwbp_handler,
> +							wp);
> +		if (IS_ERR_PCPU(wp->event)) {

[Severity: High]
Can wp->event be NULL if hardware breakpoints are unsupported?

register_wide_hw_breakpoint() can return NULL. The IS_ERR_PCPU() check does
not catch a NULL return value, causing a kernel panic when
kwatch_hwbp_arm_local() later attempts to dereference the NULL pointer.

[Severity: Medium]
Does registering breakpoints before setting up CPU hotplug callbacks create a
race gap?

If a CPU comes online between this register_wide_hw_breakpoint() loop and the
cpuhp_setup_state_nocalls() below, the new CPU will miss the watchpoint
creation.

[ ... ]

> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "kwatch:online",
> +					kwatch_hwbp_cpu_online,
> +					kwatch_hwbp_cpu_offline);
> +	if (ret < 0) {
> +		kwatch_hwbp_free();
> +		return ret;
> +	}
> +
> +	kwatch_hwbp_cpuhp_state = ret;
> +	return 0;
> +}

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