On Fri, Nov 17, 2023 at 1:53 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2023-11-17 at 11:47 -0500, Andrii Nakryiko wrote:
> [...]
> > > @@ -10479,8 +10481,14 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > > break;
> > > case BPF_FUNC_loop:
> > > update_loop_inline_state(env, meta.subprogno);
> > > - err = push_callback_call(env, insn, insn_idx, meta.subprogno,
> > > - set_loop_callback_state);
> > > + if (env->log.level & BPF_LOG_LEVEL2)
> > > + verbose(env, "frame%d callback_depth=%u\n",
> > > + env->cur_state->curframe, cur_func(env)->callback_depth);
> >
> > btw, is this a debugging leftover or intentional? If the latter, why
> > is it done only for bpf_loop()? Maybe push_callback_call() be a better
> > place for it?
>
> Intentional...
>
> >
> > > + if (cur_func(env)->callback_depth < regs[BPF_REG_1].umax_value)
> > > + err = push_callback_call(env, insn, insn_idx, meta.subprogno,
> > > + set_loop_callback_state);
> > > + else
> > > + cur_func(env)->callback_depth = 0;
> >
> > I guess it's actually a bit more interesting to know that we stopped
> > iterating because umax is reached. But I'm actually not sure whether
> > we should be adding these logs at all, though I don't have a strong
> > preference.
>
> ... it is not obvious to infer current depth looking at the log, so I
> think something should be printed. Note about stopped iteration sounds
> good, and it could be placed here, not in the push_callback_call(), e.g.:
>
> if (cur_func(env)->callback_depth < regs[BPF_REG_1].umax_value)
> err = push_callback_call(env, insn, insn_idx, meta.subprogno,
> set_loop_callback_state);
> else {
> cur_func(env)->callback_depth = 0;
> if (env->log.level & BPF_LOG_LEVEL2)
> verbose(env, "bpf_loop iteration limit reached\n");
> }
>
> wdyt?
>
>
Sure, I don't mind.
