Re: [PATCH 7/8] kprobes: Fix slot reclaim and sibling re-opt in optprobe state machine

[sashiko-bot@kernel.org]

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) <mhiramat@kernel.org>

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