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

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

Puranjay Mohan <puranjay@kernel.org> writes:

> check_kfunc_call() has multiple scattered checks that reject sleepable
> kfuncs in various non-sleepable contexts (RCU, preempt-disabled, IRQ-
> disabled). These are the same conditions already checked by
> in_sleepable_context(), so replace them with a single consolidated
> check.
>
> This also simplifies the preempt lock tracking by flattening the nested
> if/else structure into a linear chain: preempt_disable increments,
> preempt_enable checks for underflow and decrements, and the sleepable
> check uses in_sleepable_context() which covers all non-sleepable
> contexts uniformly.
>
> No functional change since in_sleepable_context() checks all the same
> state (active_rcu_locks, active_preempt_locks, active_locks,
> active_irq_id, in_sleepable).
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  kernel/bpf/verifier.c | 35 ++++++++++++-----------------------
>  1 file changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e8c4a5f8520d..c26139b96c6a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14153,34 +14153,23 @@ 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)) {
nit: it may be a little bit more readable if we put this check
separately, not in else if, so we get the next structure:
```
if (rcu_lock) {

} else if (rcu_unlock) {

} else if (preempt_disable) {

} else if (preempt_enable) {

}

if (sleepable && !in_sleepable_context(env)) {

}
```
the motivation is that logically this looks separated from the
active_*_lock accounting.
Overall the change looks like an improvement.
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
> +		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;
>  	}
>  
> -- 
> 2.47.3