Re: [RFC PATCH v2 06/13] mm/kwatch: add lockless per-task context pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Concurrent lockless accesses to the global `kwatch_ctx_pool` lack RCU or READ_ONCE protection, leading to Use-After-Free or NULL pointer dereferences.
- [High] NMI reentrancy between slot claim and release causes premature release, double free, and cross-task data corruption.
- [High] Hash table lookup degenerates to an unconditional O(N) full array scan on negative lookups, causing massive NMI latency.
- [High] Context slots permanently leak if a task exits abruptly, causing state corruption and ABA problems when the task_struct is reallocated.
- [High] Missing release/acquire memory barriers when dynamically publishing `kwatch_ctx_pool` expose lockless readers to uninitialized state and UB.
- [Medium] Large contiguous memory allocation with kcalloc() risks frequent failure on fragmented systems.
--

--- Patch [6]: [RFC PATCH v2 06/13] mm/kwatch: add lockless per-task context pool ---
commit 48d8ef7e220842e6317681d38fcc2886436cae7d
Author: Jinchao Wang <wangjinchao600@gmail.com>

mm/kwatch: add lockless per-task context pool

A task that enters the watched function needs somewhere to keep its
window state (nesting depth, owned watchpoint, config epoch). The
lookup runs in kprobe and NMI-like contexts, so it must not allocate
or take locks.

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

[ ... ]

> @@ -0,0 +1,125 @@
> +int kwatch_tsk_ctx_prealloc(u16 max_concurrency)
> +{
> +	if (max_concurrency < KWATCH_CTX_POOL_MIN)
> +		max_concurrency = KWATCH_CTX_POOL_MIN;
> +	else if (max_concurrency > KWATCH_CTX_POOL_MAX)
> +		max_concurrency = KWATCH_CTX_POOL_MAX;
> +
> +	/*
> +	 * Set the size/mask only when actually allocating, so they can never
> +	 * drift out of sync with the live pool if prealloc is ever called
> +	 * again without a matching free.
> +	 */
> +	if (unlikely(!kwatch_ctx_pool)) {
> +		kwatch_ctx_pool_size = roundup_pow_of_two(max_concurrency);
> +		kwatch_ctx_pool_mask = kwatch_ctx_pool_size - 1;
> +
> +		kwatch_ctx_pool = kcalloc(kwatch_ctx_pool_size,
> +					  sizeof(struct kwatch_tsk_ctx),
> +					  GFP_KERNEL);

[Severity: Medium]
Does this allocation risk frequent failure on fragmented systems?

In kwatch_tsk_ctx_prealloc(), this uses kcalloc with GFP_KERNEL to allocate up
to 32768 elements of struct kwatch_tsk_ctx. Such large physically contiguous
allocations are highly prone to failure when memory is fragmented.

Would using kvcalloc be more appropriate here to allow fallback to
non-contiguous virtual memory mappings?

[Severity: High]
Are there missing memory barriers here when publishing kwatch_ctx_pool?

In kwatch_tsk_ctx_prealloc(), kwatch_ctx_pool_size is initialized before
dynamically assigning kwatch_ctx_pool. Without smp_store_release, the CPU
could reorder these stores.

If a concurrent reader in kwatch_tsk_ctx_get() evaluates !kwatch_ctx_pool
without smp_load_acquire, could it observe kwatch_ctx_pool != NULL but read a
stale value of 0 for kwatch_ctx_pool_size? This would result in ilog2(0),
which returns -1, and cause a bitshift overflow when passed to hash_ptr().

> +		if (!kwatch_ctx_pool)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +struct kwatch_tsk_ctx *kwatch_tsk_ctx_get(bool can_alloc)
> +{
> +	int start_idx, i, idx;
> +	struct task_struct *t;
> +
> +	if (unlikely(!kwatch_ctx_pool))
> +		return NULL;
> +
> +	start_idx = hash_ptr(current, ilog2(kwatch_ctx_pool_size));
> +
> +	for (i = 0; i < kwatch_ctx_pool_size; i++) {
> +		idx = (start_idx + i) & kwatch_ctx_pool_mask;

[Severity: High]
Does this loop cause significant NMI latency?

In kwatch_tsk_ctx_get(), because the open-addressing hash table nullifies the
task pointer instead of using tombstones for deletions, it cannot stop probing
when encountering an empty slot.

On every cache miss, such as when a task first triggers a watchpoint, this
loop unconditionally iterates kwatch_ctx_pool_size times to ensure the task
is not already present. Is it safe to perform an O(N) scan in NMI context?

> +		t = READ_ONCE(kwatch_ctx_pool[idx].task);
> +		if (t == current)
> +			return &kwatch_ctx_pool[idx];

[Severity: High]
Could this falsely match a new task to a dead task's leaked slot?

In kwatch_tsk_ctx_get(), if a task exits abruptly due to a fatal signal and
bypasses the return probe, it will fail to call kwatch_tsk_ctx_put(), leaving
the slot assigned to the dead task_struct pointer.

When the slab allocator reuses the freed memory for a new task, the new task's
current pointer would match this leaked slot. Will the new task erroneously
inherit stale state and potentially trigger a use-after-free on the dangling
watchpoint?

> +	}
> +
> +	if (!can_alloc)
> +		return NULL;
> +
> +	for (i = 0; i < kwatch_ctx_pool_size; i++) {
> +		idx = (start_idx + i) & kwatch_ctx_pool_mask;
> +		t = READ_ONCE(kwatch_ctx_pool[idx].task);
> +		if (!t) {
> +			if (!cmpxchg(&kwatch_ctx_pool[idx].task, NULL, current))
> +				return &kwatch_ctx_pool[idx];

[Severity: High]
Can an NMI interrupting this path cause a premature release or double free?

In kwatch_tsk_ctx_get(), this lockless claim publishes ownership before the
caller can establish safe nesting state. If an NMI interrupts the task
immediately after this claim, enters a watch window, and reclaims the same
slot because t == current, it will unconditionally call kwatch_tsk_ctx_put()
when exiting the watch window.

This releases the slot by writing NULL to ctx->task. When the interrupted task
resumes, it will incorrectly assume ownership of a now-freed slot that might
be claimed by another CPU. Is there a way to prevent this reentrancy issue?

> +		}
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> +void kwatch_tsk_ctx_free(void)
> +{
> +	kfree(kwatch_ctx_pool);
> +	kwatch_ctx_pool = NULL;

[Severity: High]
Does this create a use-after-free regression?

In kwatch_tsk_ctx_free(), kfree is called directly on kwatch_ctx_pool followed
by assigning it to NULL, without any RCU synchronization.

If an NMI or task concurrently executes kwatch_tsk_ctx_get(), won't it
dereference the freed memory? Additionally, since kwatch_tsk_ctx_get()
evaluates kwatch_ctx_pool in loops without READ_ONCE, could the compiler
reload the global pointer after it is set to NULL, causing a NULL pointer
dereference?

> +}

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