Re: [PATCH bpf-next v1 08/14] bpf: Report non-sleepable kfunc programs accurately

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> index 2c475174a640..df9259fa0ea7 100644
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c
> @@ -1083,7 +1083,9 @@ void bpf_diag_ctx_forbidden(struct bpf_verifier_env *env, u32 insn_idx,
>  	const char *constraint, *context;
>  	u32 depth;
>
> -	if (env->cur_state->active_rcu_locks)
> +	if (!env->cur_state->in_sleepable)
> +		ctx_kind = BPF_DIAG_CONTEXT_NONE;
> +	else if (env->cur_state->active_rcu_locks)
>  		ctx_kind = BPF_DIAG_CONTEXT_RCU;
>  	else if (env->cur_state->active_preempt_locks)
>  		ctx_kind = BPF_DIAG_CONTEXT_PREEMPT;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index da2ec0655b17..3de9e4f617b6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -10731,6 +10731,8 @@ static inline bool in_sleepable_context(struct bpf_verifier_env *env)
>
>  static const char *non_sleepable_context_description(struct bpf_verifier_env *env)
>  {
> +	if (!in_sleepable(env))
> +		return "non-sleepable prog";
>  	if (env->cur_state->active_rcu_locks)
>  		return "rcu_read_lock region";
>  	if (env->cur_state->active_preempt_locks)
> @@ -10739,7 +10741,7 @@ static const char *non_sleepable_context_description(struct bpf_verifier_env *en
>  		return "IRQ-disabled region";
>  	if (env->cur_state->active_locks)
>  		return "lock region";
> -	return "non-sleepable prog";
> +	return "non-sleepable context";

This isn't a bug, but with the new !in_sleepable(env) early return, the
final return "non-sleepable context"; looks unreachable given all three
callers guard on !in_sleepable_context(env) - is the fallback still
needed, or could it be dropped (or kept as the single non-sleepable case)?

[ ... ]

> @@ -10835,7 +10837,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		operation = bpf_diag_fmt(env, "sleepable helper %s#%d",
>  					 func_id_name(func_id), func_id);
>  		bpf_diag_ctx_forbidden(env, insn_idx, operation,
> -			"Move the helper call outside the critical section, or use a non-sleepable helper.");
> +			"Call the helper from a sleepable program outside any critical section, or use a non-sleepable helper.");
>  		return -EINVAL;
>  	}

[ ... ]

> @@ -13760,11 +13762,20 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	}
>
>  	sleepable = bpf_is_kfunc_sleepable(&meta);
> -	if (sleepable && !in_sleepable(env)) {
> -		verbose(env, "program must be sleepable to call sleepable kfunc %s\n", func_name);
> +	if (sleepable && !in_sleepable_context(env)) {
> +		const char *suggestion;
> +
> +		if (in_sleepable(env)) {
> +			verbose(env, "kernel func %s is sleepable within %s\n",
> +				func_name, non_sleepable_context_description(env));
> +			suggestion = "Move the kfunc call outside the critical section, or use a non-sleepable kfunc.";
> +		} else {
> +			verbose(env, "program must be sleepable to call sleepable kfunc %s\n",
> +				func_name);
> +			suggestion = "Mark the program sleepable if the program type allows it, or use a non-sleepable kfunc.";
> +		}
>  		operation = bpf_diag_fmt(env, "sleepable kfunc %s", func_name);
> -		bpf_diag_ctx_forbidden(env, insn_idx, operation,
> -			"Mark the program sleepable if the program type allows it, or use a non-sleepable kfunc.");
> +		bpf_diag_ctx_forbidden(env, insn_idx, operation, suggestion);
>  		return -EACCES;
>  	}

This isn't a bug, but check_kfunc_call() now picks the suggestion per
cause - would it read better for the helper and global-function sites to
do the same instead of merging both causes into one sentence, since "from
a sleepable program" does not apply when the program is already sleepable?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31921026518