Re: [RFC PATCH v2 09/13] mm/kwatch: add probe lifecycle runtime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Use-After-Free and data race in `kwatch_probe_validate_hit()` during session teardown.
- [High] Mid-function offset kprobes incorrectly inherit NMI context rejection, bypassing safety policies.
- [High] Tasks terminating abnormally leak hardware watchpoints and cause pointer ABA vulnerabilities in the lockless context pool.
- [Medium] Race condition causing false NMI context rejections due to uninitialized delivery flag.
- [Low] Unused macro definition `TRAMPOLINE_CHECK_DEPTH`.
--

--- Patch [9]: [RFC PATCH v2 09/13] mm/kwatch: add probe lifecycle runtime ---
commit 4c4a554ca5428e6d0dbf3c8593ac5647b9e7d585
Author: Jinchao Wang <wangjinchao600@gmail.com>
Subject: mm/kwatch: add probe lifecycle runtime

This commit introduces the probe lifecycle runtime for the watch window using a
kretprobe. It handles function entry and exit, tracks nesting depth to manage
hardware watchpoints, and includes logic for NMI rejection and epoch versioning.

> 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
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/atomic.h>
> +#include <linux/kprobes.h>
> +#include <linux/kallsyms.h>
> +#include <linux/percpu.h>
> +#include <linux/preempt.h>
> +#include <linux/sched.h>
> +
> +#include "kwatch.h"
> +#define TRAMPOLINE_CHECK_DEPTH 16

[Severity: Low]
Is this macro used anywhere in the codebase?

[ ... ]

> +static void kwatch_pin_post_handler(struct kprobe *p, struct pt_regs *regs,
> +				    unsigned long flags)
> +{
> +	/* a post_handler pins the probepoint: no jump optimization */
> +}
> +
> +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;

[Severity: Critical]
If a session is stopped, kwatch_probe_stop() resets the epoch but does not
synchronously disarm hardware watchpoints for tasks still executing inside the
probed function.

Could a leftover watchpoint firing later cause this handler path to locklessly
dereference a freed cfg pointer?

Additionally, if a new session starts concurrently, kwatch_probe_start()
clears the context via memset:

kwatch_probe_start() {
    ...
    memset(&kwatch_probe_ctx, 0, sizeof(kwatch_probe_ctx));
    ...
}

Might this result in a torn or null pointer dereference here if a hardware
watchpoint fires during the memset?

[ ... ]

> +static int kwatch_activate_handler(struct kprobe *p, struct pt_regs *regs)
> +{
> +	struct kwatch_tsk_ctx *ctx = kwatch_tsk_ctx_get(false);
> +	unsigned long watch_addr;
> +	u16 watch_len;
> +
> +	if (unlikely(!ctx))
> +		return 0;
> +
> +	if (unlikely(kwatch_probe_is_muted()))
> +		return 0;
> +
> +	if (unlikely(!kwatch_tsk_ctx_check(KWATCH_PROBE_POSITION_ACTIVE)))
> +		return 0;
> +
> +	if (ctx->depth != kwatch_probe_ctx.cfg->depth + 1 || ctx->wp)
> +		return 0;

[Severity: High]
Could an NMI interrupt the probed function after the outer invocation has
already incremented ctx->depth, and then recursively call the same probed
function?

Since kwatch_lifecycle_entry() deliberately skips the ctx->depth increment
for rejected NMIs, would this mid-function offset kprobe falsely pass the
check by reading the outer context's depth and incorrectly arm the hardware
watchpoint using NMI register state?

[ ... ]

> +static int kwatch_lifecycle_entry(struct kretprobe_instance *ri,
> +				  struct pt_regs *regs)
> +{
> +	/*
> +	 * Single policy point: the target function's context is judged once
> +	 * here. A rejected invocation never increments depth, so the offset
> +	 * kprobe path inherits the verdict through the depth check.
> +	 */
> +	if (unlikely(kwatch_probed_ctx_in_nmi(kwatch_probe_ctx.rp_via_int3))) {
> +		atomic_long_inc(&kwatch_nmi_rejected);
> +		return 1; /* NMI context is unsupported: no window, no return hook */
> +	}

[Severity: Medium]
Is there a race window here where rp_via_int3 is read before being
initialized?

kwatch_probe_start() registers the kretprobe, which enables this function to
intercept calls across all CPUs, but it initializes rp_via_int3 slightly
after:

kwatch_probe_start() {
    ...
    ret = register_kretprobe(&kwatch_probe_ctx.rp);
    if (ret < 0) {
        ...
    }
    kwatch_probe_ctx.rp_via_int3 = !kprobe_ftrace(&kwatch_probe_ctx.rp.kp);
    ...
}

If another CPU hits this probe before rp_via_int3 is set, might it evaluate
as false and misclassify a normal trap context as an NMI if the architecture
uses int3 for delivery?

[ ... ]

> +static int kwatch_lifecycle_exit(struct kretprobe_instance *ri,
> +				 struct pt_regs *regs)
> +{
> +	struct kwatch_tsk_ctx *ctx = kwatch_tsk_ctx_get(false);

[Severity: High]
If a task is killed by a fatal signal (like SIGKILL) or calls do_exit()
directly while inside the probed function, it bypasses this kretprobe exit
handler.

Does this permanently leak the hardware watchpoint assigned to ctx->wp?

Furthermore, kwatch_tsk_ctx_get() matches raw pointers for the active task:

mm/kwatch/task_ctx.c:kwatch_tsk_ctx_get() {
    ...
    t = READ_ONCE(kwatch_ctx_pool[idx].task);
    if (t == current)
        return &kwatch_ctx_pool[idx];
    ...
}

If the slab allocator reuses the dead task's task_struct memory address for a
new task, will the new task erroneously inherit the corrupted state and leaked
watchpoint from the pool?

[ ... ]

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