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?
