Re: [PATCH bpf-next 23/38] bpf: Check the maps that sleepable callbacks use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: global subprog call under preempt/irq disable bypasses map RCU check
--

commit 44c0901fdd3a21bde77f64657ef1a8f91005cd05
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Check the maps that sleepable callbacks use

This commit restricts map helper calls in sleepable contexts (such as bpf_wq
or bpf_task_work callbacks, or with inner maps) to maps that do not require an
RCU read-side lock unless bpf_rcu_read_lock() is held or the helper runs inside
an RCU callback. It also adds bloom filters to the list of maps usable without
RCU.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1f2a846ae415f..09173d1d66d39 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -10379,12 +10380,57 @@ bool bpf_allow_tail_call_in_subprogs(struct bpf_verifier_env *env)
>  	       bpf_jit_supports_subprog_tailcalls();
>  }
>  
> +/* May the map be used outside of an RCU read side critical section? */
> +static bool map_needs_no_rcu(const struct bpf_map *map)
> +{
[ ... ]
> +}
> +
>  static int check_map_func_compatibility(struct bpf_verifier_env *env,
>  					struct bpf_map *map, int func_id)
>  {
>  	if (!map)
>  		return 0;
>  
> +	/*
> +	 * check_map_prog_compatibility() asks the program.  That misses the
> +	 * sleepable callback of another program, and an inner map.  They may
> +	 * use any map under the RCU read lock.  Disabled preemption or
> +	 * interrupts will not do: the maps ask for rcu_read_lock_held().
> +	 */
> +	if (in_sleepable(env) && !env->cur_state->active_rcu_locks &&
> +	    !in_rcu_callback(env) && !map_needs_no_rcu(map)) {
> +		verbose(env, "map '%s' needs bpf_rcu_read_lock() in sleepable context\n",
> +			map->name);
> +		return -EINVAL;
> +	}

[Severity: High]
Can this check be bypassed if a sleepable callback or sleepable program
disables preemption or local IRQs and then calls a global subprogram that
accesses the map?

When a global subprogram is called in check_func_call(), the verifier records
the calling context using in_sleepable_context(env):

kernel/bpf/verifier.c:check_func_call() {
	...
	if (sub_aux->func_info_aux == BPF_FUNC_GLOBAL) {
		...
		sub_aux->called[in_sleepable_context(env)] = true;
		...
	}
}

However, in_sleepable_context() checks in_rcu_cs(), which treats disabled
preemption (active_preempt_locks) and disabled local IRQs (active_irq_id) as an
RCU critical section even when active_rcu_locks is zero:

kernel/bpf/verifier.c:in_rcu_cs() {
	return env->cur_state->active_rcu_locks ||
	       env->cur_state->active_preempt_locks ||
	       env->cur_state->active_locks ||
	       env->cur_state->active_irq_id ||
	       !in_sleepable(env);
}

kernel/bpf/verifier.c:in_sleepable_context() {
	return !in_rcu_cs(env) && !in_rcu_callback(env);
}

If a bpf_wq or bpf_task_work callback calls bpf_preempt_disable() or
bpf_local_irq_save() without bpf_rcu_read_lock() and then calls a global
subprogram, in_sleepable_context(env) returns false and sets
sub_aux->called[0] = true.

When do_check_subprogs() later verifies that global subprogram via
do_check_common(env, i, 0), state->in_sleepable is initialized to false. Inside
the global subprogram, in_sleepable(env) evaluates to false in
check_map_func_compatibility(), allowing helpers such as bpf_map_lookup_elem()
on a sockhash or an inner RCU-requiring map without holding rcu_read_lock() and
triggering the WARN_ON_ONCE(!rcu_read_lock_held()) in __sock_hash_lookup_elem()
at runtime.

[ ... ]

>
>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017012058.355787-1-alexei.starovoitov@gmail.com?part=23