Re: [PATCH v5 03/13] kprobes: Expose the optprobe jump window to Tasks RCU

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6005 bytes --]

> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index e6de7ae55bda..74cc48c04417 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h

[ ... ]

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 6337da5cab9e..e460fba83e4a 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -511,6 +511,48 @@ static struct kprobe *get_optimized_kprobe(kprobe_opcode_t *addr)
>  	return NULL;
>  }
>
> +/*
> + * True while kprobe_optimizer() is waiting for its Tasks RCU grace period.
> + * Only in that window can an interruption inside an optprobe's jump region
> + * matter to it, so kprobe_in_optimized_region() does no work otherwise.
> + */
> +static bool kprobe_optimizer_waiting;
> +
> +/**
> + * kprobe_in_optimized_region - Could @addr be inside bytes a jump-optimized
> + *	kprobe replaces?
> + * @addr: kernel text address, typically an interrupted instruction pointer
> + *
> + * kprobe_optimizer() relies on synchronize_rcu_tasks() to wait for tasks that
> + * were interrupted on an instruction boundary inside the region about to be
> + * overwritten by the optimized jump.  Where Tasks RCU is built on
> + * reader-marked trampolines that region has no reader, so the irq-exit
> + * quiescent-state check asks this instead (see rcu_tasks_trampoline_text()).
> + * This is the lockless, conservative form of get_optimized_kprobe(): it does
> + * not care whether the kprobe found is, or ever will be, optimized.  May be
> + * called from any context with preemption disabled; the kprobe hash is
> + * RCU-protected and every free path waits for a grace period after unhashing.
> + *
> + * The hash walk only runs while the optimizer is actually waiting.  A task
> + * that was preempted in such a region before the flag went up is invisible
> + * to that check, so the optimizer first waits those out by their recorded
> + * preemption IP (rcu_tasks_wait_irq_preempted(), whose leading
> + * synchronize_rcu() also publishes the flag to every check in flight).
> + */
> +bool kprobe_in_optimized_region(unsigned long addr)
> +{
> +	int i;
> +
> +	if (!READ_ONCE(kprobe_optimizer_waiting))
> +		return false;
> +
> +	for (i = 1; i < MAX_OPTIMIZED_LENGTH / sizeof(kprobe_opcode_t); i++)
> +		if (get_kprobe((kprobe_opcode_t *)addr - i))
> +			return true;
> +	return false;
> +}
> +NOKPROBE_SYMBOL(kprobe_in_optimized_region);

This isn't a bug, but could the explanation of the kprobe_optimizer_waiting
window live in one place? The flag comment (lines above), the
kprobe_in_optimized_region() kernel-doc, and the Step 2 comment in
kprobe_optimizer() (see below) each describe the same mechanism. A later
change has to keep three copies in sync.

This isn't a bug, but would a name tied to the pending-optimization window
read better at the rcu_tasks_trampoline_text() call site? As written,
'in_optimized_region' suggests an already-optimized probe and hides that
the answer is false whenever the optimizer is not waiting. Something like
kprobe_in_pending_jump_region() or kprobe_optimizer_pending_at() might be
clearer.

The kernel-doc has to explain what the name doesn't convey: "does not care
whether the kprobe found is, or ever will be, optimized" and "The hash walk
only runs while the optimizer is actually waiting."

> +
>  /* Optimization staging list, protected by 'kprobe_mutex' */
>  static LIST_HEAD(optimizing_list);
>  static LIST_HEAD(unoptimizing_list);
> @@ -644,8 +686,16 @@ static void kprobe_optimizer(void)
>  		 * to 2nd-Nth byte of jump instruction. This wait is for avoiding it.
>  		 * Note that on non-preemptive kernel, this is transparently converted
>  		 * to synchronoze_sched() to wait for all interrupts to have completed.
> +		 * kprobe_optimizer_waiting lets a reader-marked-trampoline Tasks RCU
> +		 * recognise tasks interrupted in such a region while we wait, and
> +		 * rcu_tasks_wait_irq_preempted() (a no-op elsewhere) first waits
> +		 * out any that were preempted there before we said so; see
> +		 * kprobe_in_optimized_region().
>  		 */
> +		WRITE_ONCE(kprobe_optimizer_waiting, true);
> +		rcu_tasks_wait_irq_preempted(kprobe_in_optimized_region);
>  		synchronize_rcu_tasks();
> +		WRITE_ONCE(kprobe_optimizer_waiting, false);

This third description of the same mechanism adds to the maintenance burden
mentioned above.

[ ... ]

> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index f03be742be48..eb1388dd8a61 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1005,7 +1005,9 @@ bool __weak arch_rcu_tasks_trampoline_text(unsigned long ip)
>   *    trampolines, kprobe slots and other dynamically allocated text; this
>   *    deliberately does not ask is_ftrace_trampoline() and friends, since
>   *    text being torn down may already be unregistered there);
> - *  - whatever the architecture adds via arch_rcu_tasks_trampoline_text().
> + *  - whatever the architecture adds via arch_rcu_tasks_trampoline_text();
> + *  - the bytes after a kprobe that a pending jump optimization is about to
> + *    overwrite, the one synchronize_rcu_tasks() user with no trampoline.
>   *
>   * A false positive only makes the task a holdout until its next quiescent
>   * event.  Called with interrupts disabled from the irq-exit path.
> @@ -1013,8 +1015,11 @@ bool __weak arch_rcu_tasks_trampoline_text(unsigned long ip)
>  bool rcu_tasks_trampoline_text(unsigned long ip)
>  {
>  	if (core_kernel_text(ip))
> -		return arch_rcu_tasks_trampoline_text(ip);
> -	return !is_module_text_address(ip);
> +		return arch_rcu_tasks_trampoline_text(ip) ||
> +		       kprobe_in_optimized_region(ip);
> +	if (is_module_text_address(ip))
> +		return kprobe_in_optimized_region(ip);
> +	return true;
>  }
>  NOKPROBE_SYMBOL(rcu_tasks_trampoline_text);


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35680506278