On Sun, 4 Feb 2024 at 22:33, David Vernet <void@xxxxxxxxxxxxx> wrote: > > On Sun, Feb 04, 2024 at 12:02:05PM +0000, 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@xxxxxxxxx> > > Looks good, thanks for this improvement. I had the same suggestion as > Yonghong in [0], and also left a question below. > > [0]: https://lore.kernel.org/all/2e008ab1-44b8-4d1b-a86d-1f347d7630e6@xxxxxxxxx/ > > Acked-by: David Vernet <void@xxxxxxxxxxxxx> > > > --- > > 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"); > > + 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) { > > Can we do the same thing here for the RCU check below? It seems like the > exact same issue, as we're already allowed to call subprogs from within > an RCU read region, but the verifier will get confused and think we > haven't unlocked by the time we return to the caller. > > Assuming that's the case, we can take care of it in a separate patch > set. Makes sense, I'll send a separate patch for the RCU fix. Thanks for the review.
