Re: [PATCH v3 1/1] kprobes: Make optprobe optimizer multi-generational and asynchronous

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kprobes: double free/list corruption via double hlist_del_rcu() on freeing_list
--

commit 713397faac31184a877c512a3215f1d8fc48a2f8
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Subject: kprobes: Make optprobe optimizer multi-generational and asynchronous

This commit replaces synchronize_rcu_tasks() with call_rcu_tasks() by
introducing a multi-generational ring buffer. The kprobe optimizer now
unoptimizes probes and defers their cleanup asynchronously.

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 4edd8ca5c6578..33523e9beb884 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c

[ ... ]

> @@ -624,67 +696,145 @@ static void do_free_cleaned_kprobes(void)
>  
>  static void kick_kprobe_optimizer(void);
>  
> -/* Kprobe jump optimizer */
> -static void kprobe_optimizer(void)
> +static void optprobe_generation_rcu_cb(struct rcu_head *rcu)
>  {
> -	guard(mutex)(&kprobe_mutex);
> +	struct optprobe_generation *gen;
> +
> +	gen = container_of(rcu, struct optprobe_generation, rcu);
> +	WRITE_ONCE(gen->ready, true);
> +	wake_up(&kprobe_optimizer_wait);
> +}
> +
> +static void optprobe_finalize_generation(struct optprobe_generation *gen)
> +{
> +	struct optimized_kprobe *op, *tmp;
> +
> +	lockdep_assert_held(&kprobe_mutex);
>  
>  	scoped_guard(cpus_read_lock) {
>  		guard(mutex)(&text_mutex);
>  
> -		/*
> -		 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> -		 * kprobes before waiting for quiesence period.
> -		 */
> -		do_unoptimize_kprobes();
> +		/* Optimization never be done when disarmed */
> +		if (!kprobes_all_disarmed && kprobes_allow_optimization &&
> +		    !list_empty(&gen->optimizing_list))
> +			arch_optimize_kprobes(&gen->optimizing_list);
> +	}
> +
> +	/* Free cleaned kprobes after quiescence period */
> +	do_free_cleaned_kprobes(&gen->freeing_list);
> +
> +	/* Finalize unoptimized kprobes whose quiescence period completed */
> +	list_for_each_entry_safe(op, tmp, &gen->cooling_list, list) {
> +		if (kprobe_unused(&op->kp)) {
> +			/*
> +			 * Unregistered while quiescence period was in flight.
> +			 * Remove from hash list and move to cur_gen's freeing list.
> +			 */
> +			hlist_del_rcu(&op->kp.hlist);
> +			list_move(&op->list, &optprobe_gens[optprobe_cur_gen].freeing_list);

[Severity: High]
Does this code risk list corruption via a double free?

If a kprobe in the cooling_list becomes unused and is unhashed here, it is
moved to the current generation's freeing_list.

When this current generation is later dispatched, do_unoptimize_kprobes()
will iterate over its freeing_list (passed as free_list):

kernel/kprobes.c:do_unoptimize_kprobes() {
    ...
    list_for_each_entry_safe(op, tmp, free_list, list) {
        ...
        if (kprobe_unused(&op->kp)) {
            hlist_del_rcu(&op->kp.hlist);
        }
        ...
}

Will this unconditionally call hlist_del_rcu() a second time on the same
probe, causing a regression by dereferencing LIST_POISON2?

> +			kick_kprobe_optimizer();
> +		} else {
> +			/* Still in use; now safely disarmed */
> +			list_del_init(&op->list);
> +			if (!kprobe_disabled(&op->kp))
> +				optimize_kprobe(&op->kp);
> +		}
> +	}
> +
> +	gen->in_flight = false;
> +	WRITE_ONCE(gen->ready, false);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179077186716.424323.12884899073547396948.stgit@devnote2?part=1