Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Yonghong Song <yonghong.song@linux.dev>]

On 2/4/24 4:02 AM, Kumar Kartikeya Dwivedi wrote:
> Currently, calling any helpers, kfuncs, or subprogs except the graph
> data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
> is not allowed. One of the original motivations of this decision was to
> force the BPF programmer's hand into keeping the bpf_spin_lock critical
> section small, and to ensure the execution time of the program does not
> increase due to lock waiting times. In addition to this, some of the
> helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
>
> However, when it comes to subprog calls, atleast for static subprogs,
> the verifier is able to explore their instructions during verification.
> Therefore, it is similar in effect to having the same code inlined into
> the critical section. Hence, not allowing static subprog calls in the
> bpf_spin_lock critical section is mostly an annoyance that needs to be
> worked around, without providing any tangible benefit.
>
> Unlike static subprog calls, global subprog calls are not safe to permit
> within the critical section, as the verifier does not explore them
> during verification, therefore whether the same lock will be taken
> again, or unlocked, cannot be ascertained.
>
> Therefore, allow calling static subprogs within a bpf_spin_lock critical
> section, and only reject it in case the subprog linkage is global.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

SGTM with a small nit below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   kernel/bpf/verifier.c                                  | 10 +++++++---
>   tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
>   2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 64fa188d00ad..f858c959753b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>   	if (subprog_is_global(env, subprog)) {
>   		const char *sub_name = subprog_name(env, subprog);
>   
> +		/* Only global subprogs cannot be called with a lock held. */
> +		if (env->cur_state->active_lock.ptr) {
> +			verbose(env, "function calls are not allowed while holding a lock\n");

Maybe explicit to mention "global function calls are not allowed ..."?

> +			return -EINVAL;
> +		}
> +
>   		if (err) {
>   			verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
>   				subprog, sub_name);
> @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
>   
>   				if (env->cur_state->active_lock.ptr) {
>   					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> -					    (insn->src_reg == BPF_PSEUDO_CALL) ||
>   					    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>   					     (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
>   						verbose(env, "function calls are not allowed while holding a lock\n");
> @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
>   					return -EINVAL;
>   				}
>   process_bpf_exit_full:
> -				if (env->cur_state->active_lock.ptr &&
> -				    !in_rbtree_lock_required_cb(env)) {
> +				if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
>   					verbose(env, "bpf_spin_unlock is missing\n");
>   					return -EINVAL;
>   				}

[...]