On 25/08/28 09:58AM, Anton Protopopov wrote:
> On 25/08/25 04:15PM, Eduard Zingerman wrote:
> > On Sat, 2025-08-16 at 18:06 +0000, Anton Protopopov wrote:
> >
>
[...]
> > >
> > > -static int insn_successors(struct bpf_prog *prog, u32 idx, u32 succ[2])
> > > +static int insn_successors_regular(struct bpf_prog *prog, u32 insn_idx, u32 *succ)
> > > {
> > > - struct bpf_insn *insn = &prog->insnsi[idx];
> > > + struct bpf_insn *insn = &prog->insnsi[insn_idx];
> > > int i = 0, insn_sz;
> > > u32 dst;
> > >
> > > insn_sz = bpf_is_ldimm64(insn) ? 2 : 1;
> > > - if (can_fallthrough(insn) && idx + 1 < prog->len)
> > > - succ[i++] = idx + insn_sz;
> > > + if (can_fallthrough(insn) && insn_idx + 1 < prog->len)
> > > + succ[i++] = insn_idx + insn_sz;
> > >
> > > if (can_jump(insn)) {
> > > - dst = idx + jmp_offset(insn) + 1;
> > > + dst = insn_idx + jmp_offset(insn) + 1;
> > > if (i == 0 || succ[0] != dst)
> > > succ[i++] = dst;
> > > }
> > > @@ -24194,6 +24605,36 @@ static int insn_successors(struct bpf_prog *prog, u32 idx, u32 succ[2])
> > > return i;
> > > }
> > >
> > > +static int insn_successors_gotox(struct bpf_verifier_env *env,
> > > + struct bpf_prog *prog,
> > > + u32 insn_idx, u32 **succ)
> > > +{
> > > + struct jt *jt = &env->insn_aux_data[insn_idx].jt;
> > > +
> > > + if (WARN_ON_ONCE(!jt->off || !jt->off_cnt))
> > > + return -EFAULT;
> > > +
> > > + *succ = jt->off;
> > > + return jt->off_cnt;
> > > +}
> > > +
> > > +/*
> > > + * Fill in *succ[0],...,*succ[n-1] with successors. The default *succ
> > > + * pointer (of size 2) may be replaced with a custom one if more
> > > + * elements are required (i.e., an indirect jump).
> > > + */
> > > +static int insn_successors(struct bpf_verifier_env *env,
> > > + struct bpf_prog *prog,
> > > + u32 insn_idx, u32 **succ)
> > > +{
> > > + struct bpf_insn *insn = &prog->insnsi[insn_idx];
> > > +
> > > + if (unlikely(insn_is_gotox(insn)))
> > > + return insn_successors_gotox(env, prog, insn_idx, succ);
> > > +
> > > + return insn_successors_regular(prog, insn_idx, *succ);
> > > +}
> > > +
> >
> > The `prog` parameter can be dropped, as it is accessible from `env`.
> > I don't like the `u32 **succ` part of this interface.
> > What about one of the following alternatives:
> >
> > - u32 *insn_successors(struct bpf_verifier_env *env, u32 insn_idx)
> > and `u32 succ_buf[2]` added to bpf_verifier_env?
>
> I like this variant of yours more than the second one.
>
> Small corrections that this would be
>
> u32 *insn_successors(struct bpf_verifier_env *env, u32 insn_idx, int *succ_num)
>
> to return the number of instructions.
>
> > - int insn_successor(struct bpf_verifier_env *env, u32 insn_idx, u32 succ_num):
> > bool fallthrough = can_fallthrough(insn);
> > bool jump = can_jump(insn);
> > if (succ_num == 0) {
> > if (fallthrough)
> > return <next insn>
> > if (jump)
> > return <jump tgt>
> > } else if (succ_num == 1) {
> > if (fallthrough && jump)
> > return <jmp tgt>
> > } else if (is_gotox) {
> > return <lookup>
> > }
> > return -1;
> >
> > ?
So, insn_successors() actually returns two values: a pointer and a
number elements. This is the same value as "struct bpf_jt" (struct jt
in the sent patch). Wdyt about
struct bpf_jt *insn_successors(struct bpf_verifier_env *env, u32 insn_idx)
? (Maybe bpf_jt is not right name here, "insn_array" is already used
in the map, maybe smth with "successors"?)
