Re: [RFC PATCH 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:
- [High] Concurrent hardware breakpoint hits during kwatch_probe_start() cause a NULL pointer dereference in kwatch_probe_validate_hit().
- [High] Context pool slots are permanently leaked if a task hits a kwatch probe while `kwatch_probe_ctx.epoch == 0`.
- [High] The logic to determine if the probe is using int3 falsely evaluates to `false` for ftrace-enabled functions, rejecting normal executions.
- [High] Dangling `task_struct` pointers in `kwatch_ctx_pool` cause ABA aliasing and permanent hardware watchpoint leaks when a traced task terminates abnormally.
- [Medium] The NMI context detection relies on an x86-specific assumption about kprobe traps, which fails to reject NMI contexts on architectures like ARM64.
--

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

This commit adds the core kwatch probe lifecycle runtime, tracking per-task
nesting depth through entry and exit kretprobes to open and close watch
windows. It includes an epoch versioning scheme to invalidate stale
per-task state and counts rejections for functions in NMI-like contexts.

> diff --git a/mm/kwatch/probe.c b/mm/kwatch/probe.c
> new file mode 100644
> index 0000000000000..af6e0af45c10c
> --- /dev/null
> +++ b/mm/kwatch/probe.c
> @@ -0,0 +1,263 @@

[ ... ]

> +/*
> + * True if the probed function itself runs in an NMI-like context.
> + * int3-based kprobe delivery adds one NMI-like layer of its own;
> + * delivery is pinned at registration so the subtraction stays exact.
> + */
> +static bool kwatch_probed_ctx_in_nmi(bool via_int3)
> +{
> +	return (preempt_count() & NMI_MASK) > (via_int3 ? NMI_OFFSET : 0);
> +}

[Severity: Medium]
Does this NMI context check rely on x86-specific kprobe trap behavior?

On architectures like ARM64, the brk trap does not add an NMI_OFFSET to the
preempt count. If the probe is hit inside a genuine NMI on these architectures,
could this evaluate to false and fail to reject the NMI context?

[ ... ]

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

[Severity: High]
Can this cause a NULL pointer dereference if a hardware watchpoint is hit
concurrently during kwatch reconfiguration?

In kwatch_probe_start(), kwatch_probe_ctx is cleared:

    memset(&kwatch_probe_ctx, 0, sizeof(kwatch_probe_ctx));

This temporarily sets cfg to NULL. Since stopping a kwatch session
unregisters kprobes but might not synchronously disarm active hardware
watchpoints, could a thread hit an active watchpoint, invoke
kwatch_probe_validate_hit(), and attempt to read kwatch_probe_ctx.cfg->depth
while cfg is NULL?

> +		return true;
> +
> +	return false;
> +}

[ ... ]

> +static bool kwatch_tsk_ctx_check(enum kwatch_probe_position pos)
> +{
> +	struct kwatch_tsk_ctx *ctx = kwatch_tsk_ctx_get(true);
> +	u32 epoch;
> +
> +	if (unlikely(!ctx))
> +		return false;
> +
> +	/* Pairs with smp_store_release() in kwatch_probe_start/stop() */
> +	epoch = smp_load_acquire(&kwatch_probe_ctx.epoch);
> +
> +	if (unlikely(ctx->epoch != epoch))
> +		kwatch_tsk_ctx_reset(ctx, epoch);
> +
> +	if (unlikely(!epoch))
> +		return false;

[Severity: High]
Does this early return cause a permanent context pool slot leak?

If a task enters or exits the kprobe while the global epoch is temporarily 0
during kwatch_probe_start() or kwatch_probe_stop(), this check bypasses the
KWATCH_PROBE_POSITION_EXIT state machine.

Because kwatch_tsk_ctx_put() is completely skipped when epoch is 0, does the
context slot remain permanently allocated to the thread, potentially leading
to a denial of service if the pool is exhausted?

> +
> +	switch (pos) {

[ ... ]

> +static int kwatch_lifecycle_exit(struct kretprobe_instance *ri,
> +				 struct pt_regs *regs)
> +{
> +	struct kwatch_tsk_ctx *ctx = kwatch_tsk_ctx_get(false);
> +
> +	if (unlikely(!ctx))
> +		return 0;
> +
> +	if (!kwatch_tsk_ctx_check(KWATCH_PROBE_POSITION_EXIT))
> +		return 0;
> +
> +	if (ctx->depth == kwatch_probe_ctx.cfg->depth) {
> +		struct kwatch_watchpoint *wp = xchg(&ctx->wp, NULL);
> +
> +		if (wp)
> +			kwatch_hwbp_put(wp);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Are context slots and active watchpoints permanently leaked if the probed
task terminates abnormally?

If a task receives a fatal signal while inside the probed function, do_exit()
flushes the kretprobe without calling kwatch_lifecycle_exit(), meaning this
exclusive cleanup path is bypassed.

Since kwatch_tsk_ctx_get() uses the raw task_struct pointer as a lookup key,
could a new task that happens to reuse the freed task_struct memory address
erroneously inherit the corrupted context state and active watchpoint?

[ ... ]

> +int kwatch_probe_start(struct kwatch_config *cfg)
> +{
> +	static u32 next_epoch;
> +	u32 current_epoch;
> +	int ret;

[ ... ]

> +	kwatch_probe_ctx.rp.entry_handler = kwatch_lifecycle_entry;
> +	kwatch_probe_ctx.rp.handler = kwatch_lifecycle_exit;
> +	kwatch_probe_ctx.rp.kp.symbol_name = cfg->func_name;
> +
> +	ret = register_kretprobe(&kwatch_probe_ctx.rp);
> +	if (ret < 0) {
> +		unregister_kprobe(&kwatch_probe_ctx.pin_kp);
> +		return ret;
> +	}
> +	kwatch_probe_ctx.rp_via_int3 = !kprobe_ftrace(&kwatch_probe_ctx.rp.kp);

[Severity: High]
Does this logic incorrectly evaluate to false for ftrace-enabled functions?

The child kprobe (rp.kp) retains KPROBE_FLAG_FTRACE even when the aggregate
probe is forced to use INT3 due to the companion probe's post_handler. 

Because rp_via_int3 becomes false, does the trap's artificial NMI increment
make kwatch_probed_ctx_in_nmi() always evaluate to true, falsely identifying
normal contexts as NMIs and rejecting all valid executions?

> +
> +	if (cfg->func_offset) {

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