Re: [PATCH bpf-next v3 2/7] bpf: Refactor {acquire,release}_reference_state

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-11-28 at 05:30 +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 28 Nov 2024 at 05:13, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:
> > 
> > Overall looks good, but please take a look at a few notes below.
> > 
> > [...]
> > 
> > > @@ -1349,77 +1350,69 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
> > >   * On success, returns a valid pointer id to associate with the register
> > >   * On failure, returns a negative errno.
> > >   */
> > > -static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
> > > +static struct bpf_reference_state *acquire_reference_state(struct bpf_verifier_env *env, int insn_idx, bool gen_id)
> > >  {
> > >       struct bpf_verifier_state *state = env->cur_state;
> > >       int new_ofs = state->acquired_refs;
> > > -     int id, err;
> > > +     int err;
> > > 
> > >       err = resize_reference_state(state, state->acquired_refs + 1);
> > >       if (err)
> > > -             return err;
> > > -     id = ++env->id_gen;
> > > -     state->refs[new_ofs].type = REF_TYPE_PTR;
> > > -     state->refs[new_ofs].id = id;
> > > +             return NULL;
> > > +     if (gen_id)
> > > +             state->refs[new_ofs].id = ++env->id_gen;
> > 
> > Nit: state->refs[new_ods].id might end up with garbage value if 'gen_id' is false.
> >      The resize_reference_state() uses realloc_array(),
> >      which allocates memory with GFP_KERNEL, but without __GFP_ZERO flag.
> >      This is not a problem with current patch, as you always check
> >      reference type before checking id, but most of the data strucures
> >      in verifier are zero initialized just in case.
> 
> We end up assigning to s->id if gen_id is false, e.g.
> acquire_lock_state, so I think we'll be fine without __GFP_ZERO.

Oh, I see, thank you for explaining.

[...]