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.
[...]
