On Thu, 2024-11-28 at 05:30 +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 28 Nov 2024 at 05:13, Eduard Zingerman <eddyz87@xxxxxxxxx> 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.
[...]
