I see, thank you for the feedback. in that case I will send another
version that handles bpf_loop.
On Sat, Jan 4, 2025 at 7:11 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Sat, 2025-01-04 at 14:25 -0500, Emil Tsalapatis wrote:
>
> [...]
>
> > > > @@ -19048,7 +19066,7 @@ static int do_check(struct bpf_verifier_env *env)
> > > > if (env->cur_state->active_locks) {
> > > > if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> > > > (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> > > > - (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
> > > > + (insn->off != 0 || !kfunc_spin_allowed(insn->imm)))) {
> > > > verbose(env, "function calls are not allowed while holding a lock\n");
> > > > return -EINVAL;
> > > > }
> > >
> > >
> > > Nit: technically, 'bpf_loop' is a helper function independent of iter_num API.
> > > I suggest to change the name to is_bpf_iter_num_api_kfunc.
> > > Also, if we decide that loops are ok with spin locks,
> > > the condition above should be adjusted to allow calls to bpf_loop,
> > > e.g. to make the following test work:
> > >
> >
> > (Sorry for the duplicate, accidentally didn't send the email in plaintext)
> >
> > Will do, bpf_iter_num_api_kfunc is more reasonable. For bpf_loops
> > AFAICT we would need to ensure the callback cannot sleep,
> > which would need extra checks/changes to the verifier compared to
> > bpf_for. IMO we can deal with it in a separate patch if we think
> > allowing it is a good idea.
>
> Not really, callbacks are verified "in-line". When a function call to
> a function calling synchronous callback is verified, verifier steps
> into callback body some number of times. If a sleeping call would
> be discovered during callback function verification, verifier would
> see that spin lock is currently taken and report error. So, this is
> really just a check for particular helper call.
>
> [...]
>
