On Thu, Jan 2, 2025 at 1:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2025-01-01 at 15:37 -0500, Emil Tsalapatis wrote:
> > Add the bpf_iter_num_* kfuncs called by bpf_for in special_kfunc_list,
> > and allow the calls even while holding a spin lock.
> >
> > Signed-off-by: Emil Tsalapatis (Meta) <emil@xxxxxxxxxxxxxxx>
> > ---
>
> Reviewed-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
>
> > @@ -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.
> --- 8< -------------------------------------
> static int loop_cb(__u64 index, void *ctx)
> {
> return 0;
> }
>
> SEC("tc")
> __success __failure_unpriv __msg_unpriv("")
> __retval(0)
> int bpf_loop_inside_locked_region2(void)
> {
> const int zero = 0;
> struct val *val;
>
> val = bpf_map_lookup_elem(&map_spin_lock, &zero);
> if (!val)
> return -1;
>
> bpf_spin_lock(&val->l);
> bpf_loop(10, loop_cb, NULL, 0);
> bpf_spin_unlock(&val->l);
>
> return 0;
> }
> ------------------------------------- >8 ---
>
>
>
