Re: [PATCH bpf-next 2/5] bpf: Add KF_FORBID_SLEEP modifier for KF_ACQUIRE kfuncs

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2026-02-18 at 10:25 -0800, Puranjay Mohan wrote:

[...]

> @@ -10510,6 +10514,8 @@ static int release_reference_nomark(struct bpf_verifier_state *state, int ref_ob
>  		if (state->refs[i].type != REF_TYPE_PTR)
>  			continue;
>  		if (state->refs[i].id == ref_obj_id) {
> +			if (state->refs[i].forbid_sleep)
> +				state->forbid_sleep_count--;

Nit: I'd do it inside release_reference_state() just to have a
     fool-proof point for decrementing this counter.
     As it stands release_reference_state() is called from two
     more places and one need to think if .forbid_sleep_count
     needs to be handled there as well (it is not, at the moment).

>  			release_reference_state(state, i);
>  			return 0;
>  		}
> @@ -10847,7 +10853,8 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  
>  		if (env->subprog_info[subprog].might_sleep &&
>  		    (env->cur_state->active_rcu_locks || env->cur_state->active_preempt_locks ||
> -		     env->cur_state->active_irq_id || !in_sleepable(env))) {
> +		     env->cur_state->active_irq_id || env->cur_state->forbid_sleep_count ||
> +		     !in_sleepable(env))) {

The whole expression is the same as '!in_sleepable_context(env)', right?

>  			verbose(env, "global functions that may sleep are not allowed in non-sleepable context,\n"
>  				     "i.e., in a RCU/IRQ/preempt-disabled section, or in\n"
>  				     "a non-sleepable BPF program context\n");
> @@ -11435,6 +11442,11 @@ static int check_resource_leak(struct bpf_verifier_env *env, bool exception_exit
>  		return -EINVAL;
>  	}
>  
> +	if (check_lock && env->cur_state->forbid_sleep_count) {
> +		verbose(env, "%s cannot be used inside nosleep region\n", prefix);
> +		return -EINVAL;
> +	}
> +

Is this check really necessary?
Some resource is forbidding sleep here, so there would be an error report already.

[...]

> @@ -11658,6 +11671,14 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		}
>  	}
>  
> +	if (env->cur_state->forbid_sleep_count) {
> +		if (fn->might_sleep) {
> +			verbose(env, "sleepable helper %s#%d in nosleep region\n",
> +				func_id_name(func_id), func_id);
> +			return -EINVAL;
> +		}
> +	}
> +

Wdyt about consolidating this code as something like:

  if (fn->might_sleep && !in_sleepable_context(env)) {
	verbose(env, "sleepable helper %s#%d in %s region\n",
		func_id_name(func_id), func_id, non_sleepable_context_description(env));
	return -EINVAL;
  }

?

[...]

> @@ -14113,6 +14139,11 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		return -EACCES;
>  	}
>  
> +	if (sleepable && env->cur_state->forbid_sleep_count) {
> +		verbose(env, "sleepable kfunc %s in nosleep region\n", func_name);
> +		return -EACCES;
> +	}
> +

Above this check there is:

        if (sleepable && !in_sleepable(env)) {
                verbose(env, "program must be sleepable to call sleepable kfunc %s\n", func_name);
                return -EACCES;
        }

Should it be `sleepable && !in_sleepable_context(env)` instead?
If it should, then this hunk would be redundant.

[...]