Re: [PATCH bpf-next v3 3/6] bpf: Add KF_FORBID_SLEEP modifier for KF_ACQUIRE kfuncs

[Eduard Zingerman <eddyz87@gmail.com>]

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

On Mon, 2026-02-23 at 09:46 -0800, Puranjay Mohan wrote:

[...]

Would it be possible to move regular spin locks to similar semantics
(reference forbidding sleep)?

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 4872d2a6c42d..65327d2f19c1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -8644,6 +8644,9 @@ static int btf_check_kfunc_protos(struct btf *btf, u32 func_id, u32 func_flags)
>  			return err;
>  	}
>  
> +	if ((func_flags & KF_FORBID_SLEEP) && !(func_flags & KF_ACQUIRE))
> +		return -EINVAL;
> +

Are you sure this check is necessary?

>  	return 0;
>  }
>  
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c2a63f8c8984..a90c3d1b2d72 100644

[...]

> @@ -14214,6 +14228,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;
> +	}
> +

Instead of adding this hunk please do the following:
- add a refactoring patch as in the attachment;
- remove the following check from do_check_insn():

  >   if (env->cur_state->active_locks) {
  >       if ((insn->src_reg == BPF_REG_0 &&
  >            insn->imm != BPF_FUNC_spin_unlock) ||
  >           (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
  >            (insn->off != 0 || !kfunc_spin_allowed(insn->imm)))) {
  >           verbose(env,
  >               "function calls are not allowed while holding a lock\n");
  >           return -EINVAL;
  >       }
  >   }

  Relying instead on `sleepable && !in_sleepable_context()` checks in
  check_kfunc_call() and check_helper_call(). (As one more refactoring patch).

[...]

[-- Attachment #2: check-kfunc-call.diff --]
[-- Type: text/x-patch, Size: 1927 bytes --]

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2ef00f9b94fe..4a5d742fe4a4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14153,34 +14166,22 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 				}
 			}));
 		}
-	} else if (sleepable && env->cur_state->active_rcu_locks) {
-		verbose(env, "kernel func %s is sleepable within rcu_read_lock region\n", func_name);
-		return -EACCES;
-	}
-
-	if (in_rbtree_lock_required_cb(env) && (rcu_lock || rcu_unlock)) {
-		verbose(env, "Calling bpf_rcu_read_{lock,unlock} in unnecessary rbtree callback\n");
-		return -EACCES;
-	}
-
-	if (env->cur_state->active_preempt_locks) {
-		if (preempt_disable) {
-			env->cur_state->active_preempt_locks++;
-		} else if (preempt_enable) {
-			env->cur_state->active_preempt_locks--;
-		} else if (sleepable) {
-			verbose(env, "kernel func %s is sleepable within non-preemptible region\n", func_name);
-			return -EACCES;
-		}
 	} else if (preempt_disable) {
 		env->cur_state->active_preempt_locks++;
 	} else if (preempt_enable) {
-		verbose(env, "unmatched attempt to enable preemption (kernel function %s)\n", func_name);
-		return -EINVAL;
+		if (env->cur_state->active_preempt_locks == 0) {
+			verbose(env, "unmatched attempt to enable preemption (kernel function %s)\n", func_name);
+			return -EINVAL;
+		}
+		env->cur_state->active_preempt_locks--;
+	} else if (sleepable && !in_sleepable_context(env)) {
+		verbose(env, "kernel func %s is sleepable within %s\n",
+			func_name, non_sleepable_context_description(env));
+		return -EACCES;
 	}
 
-	if (env->cur_state->active_irq_id && sleepable) {
-		verbose(env, "kernel func %s is sleepable within IRQ-disabled region\n", func_name);
+	if (in_rbtree_lock_required_cb(env) && (rcu_lock || rcu_unlock)) {
+		verbose(env, "Calling bpf_rcu_read_{lock,unlock} in unnecessary rbtree callback\n");
 		return -EACCES;
 	}