Re: [PATCH v5 bpf-next 09/15] bpf: make bpf_insn_successors to return a pointer

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

On 25/10/01 03:39PM, Eduard Zingerman wrote:
> On Tue, 2025-09-30 at 12:51 +0000, Anton Protopopov wrote:
> > The bpf_insn_successors() function is used to return successors
> > to a BPF instruction. So far, an instruction could have 0, 1 or 2
> > successors. Prepare the verifier code to introduction of instructions
> > with more than 2 successors (namely, indirect jumps).
> > 
> > To do this, introduce a new struct, struct bpf_iarray, containing
> > an array of bpf instruction indexes and make bpf_insn_successors
> > to return a pointer of that type. The storage for all instructions
> > is allocated in the env->succ, which holds an array of size 2,
> > to be used for all instructions.
> > 
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> (but please fix the IS_ERR things, see below).
> 
> [...]
> 
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -509,6 +509,15 @@ struct bpf_map_ptr_state {
> >  #define BPF_ALU_SANITIZE		(BPF_ALU_SANITIZE_SRC | \
> >  					 BPF_ALU_SANITIZE_DST)
> >  
> > +/*
> > + * An array of BPF instructions.
> > + * Primary usage: return value of bpf_insn_successors.
> > + */
> > +struct bpf_iarray {
> > +	int off_cnt;
> > +	u32 off[];
> > +};
> > +
> 
> Tbh, the names `off` and `off_cnt` are a bit strange in context of
> instruction successors.

insn_offsets / insn_offset_cnt?

> [...]
> 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 705535711d10..6c742d2f4c04 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -17770,6 +17770,22 @@ static int mark_fastcall_patterns(struct bpf_verifier_env *env)
> >  	return 0;
> >  }
> >  
> > +static struct bpf_iarray *iarray_realloc(struct bpf_iarray *old, size_t n_elem)
> > +{
> > +	size_t new_size = sizeof(struct bpf_iarray) + n_elem * 4;
> 
> Nit: n_elem * 4 -> n_elem * sizeof(*old->off) ?

Yes, thanks.

> > +	struct bpf_iarray *new;
> > +
> > +	new = kvrealloc(old, new_size, GFP_KERNEL_ACCOUNT);
> > +	if (!new) {
> > +		/* this is what callers always want, so simplify the call site */
> > +		kvfree(old);
> > +		return NULL;
> > +	}
> > +
> > +	new->off_cnt = n_elem;
> > +	return new;
> > +}
> 
> [...]
> 
> > @@ -24325,14 +24342,18 @@ static int compute_live_registers(struct bpf_verifier_env *env)
> >  		for (i = 0; i < env->cfg.cur_postorder; ++i) {
> >  			int insn_idx = env->cfg.insn_postorder[i];
> >  			struct insn_live_regs *live = &state[insn_idx];
> > -			int succ_num;
> > -			u32 succ[2];
> > +			struct bpf_iarray *succ;
> >  			u16 new_out = 0;
> >  			u16 new_in = 0;
> >  
> > -			succ_num = bpf_insn_successors(env->prog, insn_idx, succ);
> > -			for (int s = 0; s < succ_num; ++s)
> > -				new_out |= state[succ[s]].in;
> > +			succ = bpf_insn_successors(env, insn_idx);
> > +			if (IS_ERR(succ)) {
> 
> This error check is no longer necessary.

Yes, thanks. I've removed these checks, but these chunks escaped
to the "indirect jumps" patch. Moved them back.

> [...]