On Sun, 4 Feb 2024 at 22:23, Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> 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@xxxxxxxxx>
>
> SGTM with a small nit below.
>
> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
>
> > ---
> > 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 ..."?
>
Ack, I made this change, and also extended the last part with "use a
static function instead" to make it more helpful.
Thanks for the review.
> > + 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;
> > }
>
> [...]
>
