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