Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] kprobes: slot leak and stale optinsn reuse via register_aggr_kprobe() - [High] kprobes: missing arch_disarm_kprobe() in unoptimize_kprobe() on force - [High] kprobes: premature unhash of rescued aggrprobe in add_new_kprobe() --
commit 1b0eca43598590c99ad643a8f10e407a46a7cbcb Author: Masami Hiramatsu (Google) <[email protected]> kprobes: Fix slot reclaim and sibling re-opt in optprobe state machine This commit moves gone optimized probes with remaining child probes onto the cooling list in kill_optimized_kprobe() and reclaims their instruction slots in optprobe_finalize_generation() after Tasks RCU completes. It also re-optimizes blocked sibling probes in optprobe_finalize_generation() once a disabled probe finishes cooling. > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index 0ef3a8701edad..53725c09ef9b0 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -725,10 +725,19 @@ static void optprobe_finalize_generation(struct > optprobe_generation *gen) > list_move(&op->list, > &optprobe_gens[optprobe_cur_gen].freeing_list); > kick_kprobe_optimizer(); > } else { > + struct kprobe *_p; > + > /* Still in use; now safely disarmed */ > list_del_init(&op->list); > - if (!kprobe_disabled(&op->kp)) > + if (kprobe_gone(&op->kp)) { > + arch_remove_optimized_kprobe(op); > + } else if (!kprobe_disabled(&op->kp)) { > optimize_kprobe(&op->kp); > + } else { > + _p = get_optimized_kprobe(op->kp.addr); > + if (unlikely(_p)) > + optimize_kprobe(_p); > + } [Severity: High] Can this re-optimize a sibling probe _p while a software breakpoint is still armed at op->kp.addr? When an optimized probe is disabled via disable_kprobe(), __disable_kprobe() calls __disarm_kprobe(), which calls unoptimize_kprobe(p, false) and skips arch_disarm_kprobe(p) because kprobe_queued_unopt(p) is true. Because the disabled child probe remains on op->kp.list, kprobe_disabled(&op->kp) is true while kprobe_unused(&op->kp) is false. If unoptimize_kprobe(&op->kp, true) is then called with force = true before do_unoptimize_kprobes() runs (for example, when an overlapping probe is armed via __arm_kprobe() -> get_optimized_kprobe() -> unoptimize_kprobe(_p, true), or when a child probe with a post_handler is added), force_unoptimize_kprobe() replaces the jump at op->kp.addr with a software breakpoint. However, unlike do_unoptimize_kprobes(): kernel/kprobes.c:do_unoptimize_kprobes() { ... if (kprobe_disabled(&op->kp) && !kprobe_gone(&op->kp)) arch_disarm_kprobe(&op->kp); ... } unoptimize_kprobe() only calls arch_disarm_kprobe(&op->kp) when kprobe_unused(&op->kp) is true: kernel/kprobes.c:unoptimize_kprobe() { ... force_unoptimize_kprobe(op); if (kprobe_unused(&op->kp)) { if (!kprobe_gone(&op->kp)) arch_disarm_kprobe(&op->kp); hlist_del_init_rcu(&op->kp.hlist); list_move(&op->list, &cur->freeing_list); } else { list_move(&op->list, &cur->cooling_list); } ... } When optprobe_finalize_generation() finishes cooling op and runs this else branch, op->list is emptied (making kprobe_disarmed(&op->kp) true) and optimize_kprobe(_p) is called on the overlapping probe while the breakpoint at op->kp.addr is still armed in kernel text. Wouldn't arch_optimize_kprobes() then copy the undisarmed breakpoint instruction into _p->optinsn.copied_insn, and leave an orphaned breakpoint in kernel text if op is later unregistered? [Severity: High] In the same optprobe state machine, what happens when an unused aggregator probe on unoptimizing_list or cooling_list is rescued by register_aggr_kprobe() for a new kprobe p that has a post_handler? In commit 9db5fa1da371 ("kprobes: Do not overwrite jump instruction when re-arming unoptimizing probe"), reuse_unused_kprobe() was changed to leave KPROBE_FLAG_DISABLED set on ap and leave op on unoptimizing_list or cooling_list: kernel/kprobes.c:reuse_unused_kprobe() { struct optimized_kprobe *op; op = container_of(ap, struct optimized_kprobe, kp); WARN_ON_ONCE(list_empty(&op->list)); if (!kprobe_optready(ap)) return -EINVAL; return 0; } register_aggr_kprobe() then calls add_new_kprobe(ap, p), which calls unoptimize_kprobe(ap, true) before adding p->list to ap->list: kernel/kprobes.c:add_new_kprobe() { if (p->post_handler) unoptimize_kprobe(ap, true); /* Fall back to normal kprobe */ list_add_rcu(&p->list, &ap->list); } Because ap->list is still empty and KPROBE_FLAG_DISABLED is still set when unoptimize_kprobe(ap, true) runs, kprobe_unused(&op->kp) evaluates to true in both the optprobe_queued_unopt(op) and optprobe_queued_cooling(op) branches of unoptimize_kprobe(). Doesn't unoptimize_kprobe(ap, true) then unhash op->kp.hlist from kprobe_table and move op->list to cur->freeing_list right before register_aggr_kprobe() arms a breakpoint at ap->addr for the unhashed probe? > } > } > > @@ -1084,13 +1093,20 @@ static void kill_optimized_kprobe(struct kprobe *p) > list_del_init(&op->list); > } > } else { > - list_del_init(&op->list); > + if (optprobe_queued_unopt(op)) { > + /* Move to cooling list so optinsn slot is > cooled down */ > + list_move(&op->list, > + > &optprobe_gens[optprobe_cur_gen].cooling_list); > + kick_kprobe_optimizer(); > + } else if (!optprobe_queued_cooling(op) && > !optprobe_queued_freeing(op)) { > + list_del_init(&op->list); > + } > } > } > op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; > > /* > - * Don't remove the slot if it is queued for freeing or unoptimization; > + * Don't remove the slot if it is queued for freeing, unoptimization, > or cooling; > * the optimizer will reclaim it after the quiescence period. > */ > if (!optprobe_queued_freeing(op) && !optprobe_queued_unopt(op) && [Severity: High] Does deferring arch_remove_optimized_kprobe(op) here leak op->optinsn.insn or reuse a stale optinsn trampoline if a new kprobe is registered at the same virtual address before cooling completes? When kill_kprobe() runs on a probe that still has child probes on p->list (!kprobe_unused(p)) while op is on unoptimizing_list or cooling_list, kill_optimized_kprobe() now leaves op on cooling_list with op->optinsn.insn allocated, while ap (&op->kp) remains hashed in kprobe_table with KPROBE_FLAG_GONE set. If a module is loaded at the same virtual address and a new kprobe is registered at ap->addr before optprobe_finalize_generation() runs, __register_kprobe() finds ap in kprobe_table and calls register_aggr_kprobe(): kernel/kprobes.c:register_aggr_kprobe() { ... if (kprobe_gone(ap)) { ret = arch_prepare_kprobe(ap); if (ret) return ret; /* Prepare optimized instructions if possible. */ prepare_optimized_kprobe(ap); ap->flags = (ap->flags & ~KPROBE_FLAG_GONE) | KPROBE_FLAG_DISABLED; } ... } Because register_aggr_kprobe() assumes the old instruction slots were already freed by kill_kprobe(), it calls prepare_optimized_kprobe(ap) -> arch_prepare_optimized_kprobe(op, ap) and clears KPROBE_FLAG_GONE: 1. If arch_prepare_optimized_kprobe() allocates a new slot, it overwrites op->optinsn.insn without freeing the in-flight cooling slot, leaking the old optinsn slot (and since KPROBE_FLAG_GONE is cleared, optprobe_finalize_generation() will not reclaim it either). 2. If arch_prepare_optimized_kprobe() fails before writing op->optinsn.insn (for example, when can_optimize() rejects the new function at ap->addr), op->optinsn.insn still points to the old unloaded module's optinsn slot. Once register_aggr_kprobe() clears KPROBE_FLAG_GONE and arms ap, won't optprobe_finalize_generation() see !kprobe_gone(&op->kp) and !kprobe_disabled(&op->kp), and call optimize_kprobe(&op->kp) using the stale optinsn trampoline from the unloaded module? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/179375029807.247640.13024754173032838618.stgit@devnote2?part=7
