Re: [PATCH bpf-next v3 2/6] bpf: consolidate sleepable context error message printing

[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.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  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 c693dd663cab..c2a63f8c8984 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11574,6 +11574,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";
This check didn't exist before, did it? It looks right, but adding
something into the commit message to explain this would be nice.
Does it mean we can currently call a sleepable helper in the lock
region?
> +	return "non-sleepable context";
> +}
> +
>  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			     int *insn_idx_p)
>  {
> @@ -11634,28 +11647,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. */
> -- 
> 2.47.3