Re: [PATCH 3/3] kprobes: Arm reused optprobe on cooling list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kprobes: reuse_unused_kprobe() arms cooling probe when new probe is disabled
--

commit c03ba118d217fb856e6a911233d52037e4e98250
Author: Mani Tofigh <mt4022@columbia.edu>

kprobes: Arm reused optprobe on cooling list

This commit arms an unused aggregator kprobe in reuse_unused_kprobe() when
it is still on an optprobe cooling list so that a newly registered probe
at the same address will fire before optimization finishes.

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 84a5f68476eb1..59c5689299ce3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c

[ ... ]

> @@ -1067,6 +1068,13 @@ static int reuse_unused_kprobe(struct kprobe *ap)
>  	if (!kprobe_optready(ap))
>  		return -EINVAL;
>  
> +	if (optprobe_queued_cooling(op)) {
> +		/* Already disarmed. It will be optimized again upon finalize */
> +		if (!kprobes_all_disarmed)
> +			__arm_kprobe(ap);
> +		return 0;
> +	}

[Severity: High]
What happens here if the newly registered kprobe has KPROBE_FLAG_DISABLED
set, such as when __register_trace_kprobe() registers a new tracefs
kprobe_events entry before enabling it?

In __register_kprobe() -> register_aggr_kprobe(), reuse_unused_kprobe(ap)
is called before checking whether the new probe p is disabled:

kernel/kprobes.c:register_aggr_kprobe() {
	...
		} else if (kprobe_unused(ap)) {
			/* This probe is going to die. Rescue it */
			ret = reuse_unused_kprobe(ap);
			if (ret)
				return ret;
		}
	...
	if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
		ap->flags &= ~KPROBE_FLAG_DISABLED;
		if (!kprobes_all_disarmed) {
			/* Arm the breakpoint again. */
			ret = arm_kprobe(ap);
	...
}

Because reuse_unused_kprobe(ap) unconditionally clears KPROBE_FLAG_DISABLED
on ap and calls __arm_kprobe(ap) when op is on a cooling_list, it bypasses
the !kprobe_disabled(p) check in register_aggr_kprobe(). This arms a
breakpoint in kernel text (and later jump-optimizes ap in
optprobe_finalize_generation()) even though all child probes on ap->list
are disabled.

Furthermore, if the disabled child probe p is later unregistered without
ever being enabled, __unregister_kprobe_top() calls __disable_kprobe(p),
which sees kprobe_disabled(p) and returns early without setting
KPROBE_FLAG_DISABLED on ap or disarming it:

kernel/kprobes.c:__disable_kprobe() {
	...
	if (kprobe_disabled(p))
		return orig_p;
	...
}

Then in __unregister_kprobe_top(), kprobe_disarmed(ap) is false because
KPROBE_FLAG_DISABLED is not set on ap:

kernel/kprobes.c:__unregister_kprobe_top() {
	...
	if (ap == p ||
		(list_is_singular(&ap->list) && kprobe_disarmed(ap))) {
		/*
		 * !disarmed could be happen if the probe is under delayed
		 * unoptimizing.
		 */
		hlist_del_rcu(&ap->hlist);
		return 0;
	}
	...
}

Does this leave the aggregator struct optimized_kprobe permanently hashed
and leaked in kprobe_table with an empty ap->list and an active breakpoint
or jump instruction patched into kernel text?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-kprobes-optprobe-state-v1-0-6aaac612abe6@columbia.edu?part=3