Re: [PATCH v1 bpf-next 08/11] bpf, x86: add support for indirect jumps

[Anton Protopopov <a.s.protopopov@gmail.com>]

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"?)