Re: [RFC PATCH v2 13/13] Documentation/dev-tools: document KWatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition during CPU offline leads to spurious WARN_ONCE and potential Use-After-Free.
- [High] KWatch blindly filters out hits from the armed task's callees and local interrupts.
- [Medium] Missing Kconfig dependency on TRACING/TRACEPOINTS.
--

--- Patch [13]: [RFC PATCH v2 13/13] Documentation/dev-tools: document KWatch ---
commit 2276ea12010818a130be33709ca8b2c8928c26a9
Author: Jinchao Wang <wangjinchao600@gmail.com>

Documentation/dev-tools: document KWatch

Describe what KWatch is for, how it compares with KASAN and KFENCE,
the debugfs configuration interface, the watch expression syntax,
how to read hits from the trace buffer (including after a crash),
and the current limitations.

> diff --git a/mm/kwatch/Kconfig b/mm/kwatch/Kconfig
> new file mode 100644
> index 0000000000000..6ec9aa448ecec
> --- /dev/null
> +++ b/mm/kwatch/Kconfig
> @@ -0,0 +1,28 @@
> +config KWATCH
> +	tristate "Kernel Watch Framework"
> +	depends on PERF_EVENTS && HAVE_HW_BREAKPOINT && DEBUG_FS
> +	depends on HAVE_REINSTALL_HW_BREAKPOINT
> +	depends on KPROBES && KRETPROBES
> +	depends on STACKTRACE

[Severity: Medium]
Should this configuration depend on or select TRACING or TRACEPOINTS?

Since KWatch outputs successful memory access hits exclusively through the
kwatch_hit tracepoint, building without tracepoint support results in KWatch
compiling and consuming hardware debug registers, but silently failing to
report any hits when memory corruption occurs.

> +	help
> +	  A generalized hardware-assisted memory monitor utility.

[ ... ]

> diff --git a/mm/kwatch/hwbp.c b/mm/kwatch/hwbp.c
> new file mode 100644
> index 0000000000000..d1e93754cce82
> --- /dev/null
> +++ b/mm/kwatch/hwbp.c

[ ... ]

> +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 this sequence open a race condition during CPU offline?

If an asynchronous IPI from kwatch_hwbp_arm() interrupts the offlining CPU
exactly between the unregistration and the pointer being cleared:

	unregister_hw_breakpoint(bp);
	/* IPI arrives here */
	per_cpu(*wp->event, cpu) = NULL;

The IPI handler in kwatch_hwbp_arm_local() will read the non-NULL,
unregistered breakpoint and pass it to modify_wide_hw_breakpoint_local().

Could this access unlinked and potentially freed memory (if the RCU grace
period elapses), corrupting debug registers or triggering a spurious
WARN_ONCE?

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

[ ... ]

> diff --git a/mm/kwatch/probe.c b/mm/kwatch/probe.c
> new file mode 100644
> index 0000000000000..249aa50c9f782
> --- /dev/null
> +++ b/mm/kwatch/probe.c

[ ... ]

> +bool kwatch_probe_validate_hit(struct pt_regs *regs,
> +			       struct task_struct *arm_tsk)
> +{
> +	struct kwatch_tsk_ctx *ctx = kwatch_tsk_ctx_get(false);
> +	const struct kwatch_config *cfg = kwatch_probe_ctx.cfg;
> +
> +	if (unlikely(!ctx || !cfg))
> +		return true;
> +
> +	if (arm_tsk != current || ctx->depth != cfg->depth + 1)
> +		return true;
> +
> +	return false;

[Severity: High]
Does this logic inadvertently filter out genuine corruptions?

Returning false when arm_tsk == current and ctx->depth == cfg->depth + 1
prevents hits from being logged for the current function. However, since
depth is only incremented when recursively entering the watched function,
does this also silently ignore memory corruptions caused by the watched
function's callees, or by any interrupt handler that preempts the watched
task on the same CPU?

> +}

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