Re: [PATCH bpf-next 1/4] bpf: replace env->cur_hist_ent with a getter function

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-02-28 at 11:46 -0800, Andrii Nakryiko wrote:
[...]

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 011d54a1dc53..759ef089b33c 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3304,24 +3304,34 @@ static bool is_jmp_point(struct bpf_verifier_env *env, int insn_idx)
> >         return env->insn_aux_data[insn_idx].jmp_point;
> >  }
> > 
> > +static struct bpf_jmp_history_entry *get_jmp_hist_entry(struct bpf_verifier_state *st,
> > +                                                       u32 hist_end, int insn_idx)
> > +{
> > +       if (hist_end > 0 && st->jmp_history[hist_end - 1].idx == insn_idx)
> > +               return &st->jmp_history[hist_end - 1];
> > +       return NULL;
> > +}
> > +
> >  /* for any branch, call, exit record the history of jmps in the given state */
> >  static int push_jmp_history(struct bpf_verifier_env *env, struct bpf_verifier_state *cur,
> >                             int insn_flags)
> >  {
> > +       struct bpf_jmp_history_entry *p, *cur_hist_ent;
> >         u32 cnt = cur->jmp_history_cnt;
> > -       struct bpf_jmp_history_entry *p;
> >         size_t alloc_size;
> > 
> > +       cur_hist_ent = get_jmp_hist_entry(cur, cnt, env->insn_idx);
> > +
> 
> This is, generally speaking, not correct to do. You can have a tight
> loop where the instruction with the same insn_idx is executed multiple
> times and so we'll get multiple consecutive entries in jmp_history
> with the same insn_idx. We shouldn't reuse hist_ent for all of them,
> each simulated instruction execution should have its own entry in jump
> history.

You are correct.

> It's fine to use get_jmp_hist_entry() in backtracking, though.
> 
> I'll look through the rest of patches more closely first before
> suggesting any alternatives. But what you do in this patch is not 100%
> correct.

No need, the patch-set does not rely on capability to push entry for
several states simultaneously. I'll just drop this patch from v2.

[...]