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 <[email protected]> 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/[email protected]?part=3
