Re: [RFC PATCH bpf-next 02/12] bpf: Make struct_ops tasks_rcu grace period optional

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The patch incorrectly skips the Tasks RCU grace period (`synchronize_rcu_tasks()`) before freeing executable trampoline memory for all `struct_ops` except `tcp_congestion_ops`, leading to Use-After-Free of executable memory.
- [High] RCU-001 violation: `bpf_struct_ops_map_dissoc_progs()` removes the map from `prog->aux->st_ops_assoc` after the regular RCU grace period has already completed, leading to a Use-After-Free.
--

commit 85a451ad8aefbb364a1840344e08d2f34596b96c
Author: Martin KaFai Lau <martin.lau@kernel.org>

bpf: Make struct_ops tasks_rcu grace period optional

This commit makes the tasks RCU grace period optional during struct_ops map
freeing. It introduces a map_free_pre_rcu callback to remove ksyms before the
regular RCU grace period. The tasks RCU grace period is now only explicitly
requested by tcp_congestion_ops.

> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 08791180d71d9..28eab24ef0ed0 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -996,24 +1005,8 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
>  
>  	bpf_struct_ops_map_dissoc_progs(st_map);

[Severity: High]
Does this violate the RCU remove-before-reclaim rule?

Because map->free_after_rcu_gp is set to true in bpf_struct_ops_map_alloc(),
the regular RCU grace period occurs before bpf_struct_ops_map_free() is called.
Leaving bpf_struct_ops_map_dissoc_progs() here means the map is removed
from prog->aux->st_ops_assoc after the grace period has already finished.

Could a concurrent execution of the BPF program acquire the RCU read lock, read
the still-valid aux->st_ops_assoc pointer, and then access the map after it is
freed?

Should bpf_struct_ops_map_dissoc_progs() be moved to the new
bpf_struct_ops_map_free_pre_rcu() callback instead?

> -	/* The struct_ops's function may switch to another struct_ops.
> -	 *
> -	 * For example, bpf_tcp_cc_x->init() may switch to
> -	 * another tcp_cc_y by calling
> -	 * setsockopt(TCP_CONGESTION, "tcp_cc_y").
> -	 * During the switch,  bpf_struct_ops_put(tcp_cc_x) is called
> -	 * and its refcount may reach 0 which then free its
> -	 * trampoline image while tcp_cc_x is still running.
> -	 *
> -	 * A vanilla rcu gp is to wait for all bpf-tcp-cc prog
> -	 * to finish. bpf-tcp-cc prog is non sleepable.
> -	 * A rcu_tasks gp is to wait for the last few insn
> -	 * in the tramopline image to finish before releasing
> -	 * the trampoline image.
> -	 */
> -	synchronize_rcu_mult(call_rcu, call_rcu_tasks);
> +	if (tasks_rcu && IS_ENABLED(CONFIG_TASKS_RCU))
> +		synchronize_rcu_tasks();
>  
>  	__bpf_struct_ops_map_free(map);
>  }

[Severity: Critical]
Does skipping the Tasks RCU grace period for other struct_ops cause a
use-after-free of executable trampoline memory?

All struct_ops programs are executed via dynamically generated trampolines. When
the program returns, the RCU read lock is dropped and preemption is re-enabled.
If a task is preempted while still executing the trampoline epilogue, the
regular RCU grace period will not wait for it.

Without the Tasks RCU wait, could the executable trampoline image be freed
prematurely, causing preempted tasks to execute freed memory when they resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519215841.2984970-1-martin.lau@linux.dev?part=2