Re: [PATCH bpf-next v2 1/2] bpf: Verify global subprogs in each sleepability context

[Eduard Zingerman <eddyz87@gmail.com>]

On Sat, 2026-09-05 at 07:12 +0200, Kumar Kartikeya Dwivedi wrote:
> Global subprograms are verified independently with a fresh verifier root.
> do_check_common() currently seeds that root's in_sleepable state from the
> program, even though a global subprogram can also run from callbacks whose
> execution context differs from the program's main entry point.
> 
> In particular, workqueue and task-work callbacks are sleepable even when
> the containing program is not. A global subprogram of that program is
> therefore verified as non-sleepable, making in_rcu_cs() true and allowing
> loads of RCU-protected kptrs to produce trusted MEM_RCU pointers. The same
> subprogram can then be called from a sleepable callback without a classic
> RCU reader. It can retain such a pointer while the object is freed and use
> it after free.
> 
> The verifier's execution-context predicates are complementary. A state is
> sleepable only when in_sleepable is set and no RCU, preemption, IRQ, or lock
> region is active. Each condition which prevents sleeping also provides RCU
> protection, while in_rcu_cs() treats a non-sleepable state as implicitly
> protected.
> 
> Use this relationship to represent a global subprogram caller with only the
> result of in_sleepable_context(). A protected sleepable caller is normalized
> to in_sleepable=false at the independent verification root. This both
> prevents sleepable operations and makes in_rcu_cs() true without copying
> caller-owned lock state.
> 
> Record whether each global subprogram is called with either in_sleepable
> value and verify it once for every observed value. Walk global subprograms
> in caller-before-callee order so the values propagate through global call
> chains, and repeat until every discovered context has been verified to cover
> asynchronous callback cycles.
> 
> Since a global subprogram may now be verified twice, accumulate both passes
> in subprog_info[].insns_total. This makes BPF_LOG_STATS and per-subprogram
> veristat output account for both contexts instead of reporting only the last
> pass.
> 
> This makes an unprotected callback verify the global subprogram as
> sleepable, turning its RCU-protected kptr load into an untrusted pointer.
> Protected callers and global subprograms which do not depend on implicit RCU
> protection remain valid.
> 
> Fixes: 81f1d7a583fa ("bpf: wq: add bpf_wq_set_callback_impl")
> Fixes: 38aa7003e369 ("bpf: task work scheduling kfuncs")
> Reported-by: Nicholas Carlini <npc@anthropic.com>
> Suggested-by: Nicholas Carlini <npc@anthropic.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Hi Kartikeya,

Apologies for the delayed review. Overall the code lgtm, please see a
few questions and nits below.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5b51e7ee1a3f..f759a020c8a5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

...

> @@ -10804,7 +10804,12 @@ int bpf_get_helper_proto(struct bpf_verifier_env *env, int func_id,
>  	return *ptr && (*ptr)->func ? 0 : -EINVAL;
>  }
>  
> -/* Check if we're in a sleepable context. */
> +/*
> + * This predicate is the inverse of in_rcu_cs(): non-sleepable programs and
> + * every condition that prevents sleeping also provide RCU protection. Global
> + * subprog verification relies on this equivalence to represent the caller's
> + * execution context using only the in_sleepable bit.
> + */

Agree with Alexei, let's just write it down as !in_rcu_cs().
I think it's a second time we argue :)
For the purposes of this patch-set one can just drop the comment above.

>  static inline bool in_sleepable_context(struct bpf_verifier_env *env)
>  {
>  	return !env->cur_state->active_rcu_locks &&
> @@ -19560,13 +19565,14 @@ static void free_states(struct bpf_verifier_env *env)
>  	}
>  }
>  
> -static int do_check_common(struct bpf_verifier_env *env, int subprog)
> +static int do_check_common(struct bpf_verifier_env *env, int subprog, bool in_sleepable)
>  {
>  	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
>  	struct bpf_subprog_info *sub = subprog_info(env, subprog);
>  	struct bpf_prog_aux *aux = env->prog->aux;
>  	struct bpf_verifier_state *state;
>  	struct bpf_reg_state *regs;
> +	u32 old_insns_total = sub->insns_total;
>  	u32 insn_processed = env->insn_processed;
>  	int ret, i;
>  
> @@ -19579,7 +19585,7 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  	state->curframe = 0;
>  	state->speculative = false;
>  	state->branches = 1;
> -	state->in_sleepable = env->prog->sleepable;
> +	state->in_sleepable = in_sleepable;
>  	state->frame[0] = kzalloc_obj(struct bpf_func_state, GFP_KERNEL_ACCOUNT);
>  	if (!state->frame[0]) {
>  		kfree(state);
> @@ -19721,7 +19727,8 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  	 * Accumulate their total counts as total counts of the main or
>  	 * global subprog hosting the async call.
>  	 */
> -	env->subprog_info[subprog].insns_total = env->insn_processed - insn_processed;
> +	env->subprog_info[subprog].insns_total = old_insns_total +
> +						(env->insn_processed - insn_processed);

Wouldn't += work w/o old_insns_total temporary?

>  	return ret;
>  }
>  
> @@ -19749,45 +19756,55 @@ static int do_check_subprogs(struct bpf_verifier_env *env)
>  {
>  	struct bpf_prog_aux *aux = env->prog->aux;
>  	struct bpf_func_info_aux *sub_aux;
> -	int i, ret, new_cnt;
> +	int context, i, j, ret, new_cnt;
>  
>  	if (!aux->func_info)
>  		return 0;
>  
>  	/* exception callback is presumed to be always called */
> -	if (env->exception_callback_subprog)
> -		subprog_aux(env, env->exception_callback_subprog)->called = true;
> +	if (env->exception_callback_subprog) {
> +		sub_aux = subprog_aux(env, env->exception_callback_subprog);
> +		sub_aux->called[env->prog->sleepable] = true;

Do we allow to call throw from the callbacks?
If we do, do we handle the `called[sleepable|not-sleepable]`
management for exception callbacks?

> +	}
>  
>  again:
>  	new_cnt = 0;
> -	for (i = 1; i < env->subprog_cnt; i++) {
> +	/*
> +	 * Walk callers before callees so each global subprog normally sees all
> +	 * of its contexts before it is verified. Async callback cycles can add a
> +	 * context to an earlier subprog, so repeat until every called context is
> +	 * verified.
> +	 */
> +	for (j = env->subprog_cnt - 1; j >= 0; j--) {
> +		i = env->subprog_topo_order[j];
> +		if (!i)
> +			continue;

Nit: is it really necessary to explicitly change the traversal order here?
     the algorithm already explores the function only when 'called' flag is set.

>  		if (!bpf_subprog_is_global(env, i))
>  			continue;
>  
>  		sub_aux = subprog_aux(env, i);
> -		if (!sub_aux->called || sub_aux->verified)
> -			continue;
> +		for (context = 0; context < ARRAY_SIZE(sub_aux->called); context++) {
> +			if (!sub_aux->called[context] || sub_aux->verified[context])
> +				continue;
>  
> -		env->insn_idx = env->subprog_info[i].start;
> -		WARN_ON_ONCE(env->insn_idx == 0);
> -		ret = do_check_common(env, i);
> -		if (ret) {
> -			return ret;
> -		} else if (env->log.level & BPF_LOG_LEVEL) {
> -			verbose(env, "Func#%d ('%s') is safe for any args that match its prototype\n",
> -				i, bpf_subprog_name(env, i));
> -		}
> +			env->insn_idx = env->subprog_info[i].start;
> +			WARN_ON_ONCE(env->insn_idx == 0);
> +			ret = do_check_common(env, i, context);
> +			if (ret)
> +				return ret;
> +			if (env->log.level & BPF_LOG_LEVEL)
> +				verbose(env, "Func#%d ('%s') is safe for any args "
> +					"that match its prototype\n",
> +					i, bpf_subprog_name(env, i));
>  
> -		/* We verified new global subprog, it might have called some
> -		 * more global subprogs that we haven't verified yet, so we
> -		 * need to do another pass over subprogs to verify those.
> -		 */
> -		sub_aux->verified = true;
> -		new_cnt++;
> +			sub_aux->verified[context] = true;
> +			new_cnt++;
> +		}
>  	}
>  
> -	/* We can't loop forever as we verify at least one global subprog on
> -	 * each pass.
> +	/*
> +	 * We can't loop forever as each pass verifies at least one new context,
> +	 * and there are only two contexts per global subprog.
>  	 */
>  	if (new_cnt)
>  		goto again;

...