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 04:49PM, Alexei Starovoitov wrote:
> On Wed, Oct 1, 2025 at 3:39 PM Eduard Zingerman <eddyz87@gmail.com> 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.
> >
> > [...]
> >
> > > 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) ?
> >
> > > +     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.
> 
> Speaking of IS_ERR checks...
> https://github.com/kernel-patches/bpf/pull/9895#issuecomment-3352016682
> 
> AI for-the-win!

Nice! Won't be surprised if it was trained just based on Eduard's reviews :)

(Actually, are there any details anywhere about this AI?)