Re: [PATCH bpf-next 1/2] bpf: Consolidate sleepable checks in check_helper_call()

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Puranjay Mohan <puranjay@kernel.org> writes:

> check_helper_call() prints the error message for every
> env->cur_state->active* element when calling a sleepable helper.
> Consolidate all of them into a single print statement.
>
> The check for env->cur_state->active_locks was not part of the removed
> print statements and will not be triggered with the consolidated print
> as well because it is checked in do_check() before check_helper_call()
> is even reached.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
>  kernel/bpf/verifier.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1153a828ce8d..e8c4a5f8520d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11549,6 +11549,19 @@ static inline bool in_sleepable_context(struct bpf_verifier_env *env)
>  	       in_sleepable(env);
>  }
>  
> +static const char *non_sleepable_context_description(struct bpf_verifier_env *env)
> +{
> +	if (env->cur_state->active_rcu_locks)
> +		return "rcu_read_lock region";
> +	if (env->cur_state->active_preempt_locks)
> +		return "non-preemptible region";
> +	if (env->cur_state->active_irq_id)
> +		return "IRQ-disabled region";
> +	if (env->cur_state->active_locks)
> +		return "lock region";
> +	return "non-sleepable context";
> +}
> +
>  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			     int *insn_idx_p)
>  {
> @@ -11609,28 +11622,11 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		return err;
>  	}
>  
> -	if (env->cur_state->active_rcu_locks) {
> -		if (fn->might_sleep) {
> -			verbose(env, "sleepable helper %s#%d in rcu_read_lock region\n",
> -				func_id_name(func_id), func_id);
> -			return -EINVAL;
> -		}
> -	}
> -
> -	if (env->cur_state->active_preempt_locks) {
> -		if (fn->might_sleep) {
> -			verbose(env, "sleepable helper %s#%d in non-preemptible region\n",
> -				func_id_name(func_id), func_id);
> -			return -EINVAL;
> -		}
> -	}
> -
> -	if (env->cur_state->active_irq_id) {
> -		if (fn->might_sleep) {
> -			verbose(env, "sleepable helper %s#%d in IRQ-disabled region\n",
> -				func_id_name(func_id), func_id);
> -			return -EINVAL;
> -		}
> +	if (fn->might_sleep && !in_sleepable_context(env)) {
> +		verbose(env, "sleepable helper %s#%d in %s\n",
> +			func_id_name(func_id), func_id,
> +			non_sleepable_context_description(env));
> +		return -EINVAL;
>  	}
>  
>  	/* Track non-sleepable context for helpers. */
>
> base-commit: f620af11c27b8ec9994a39fe968aa778112d1566
> -- 
> 2.47.3