Re: [PATCH bpf-next v3 1/7] bpf: Consolidate locks and reference state in verifier state

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-11-28 at 04:18 +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 28 Nov 2024 at 04:03, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Thu, 2024-11-28 at 03:54 +0100, Kumar Kartikeya Dwivedi wrote:
> > 
> > [...]
> > 
> > > > > --- a/kernel/bpf/log.c
> > > > > +++ b/kernel/bpf/log.c
> > > > > @@ -756,6 +756,7 @@ static void print_reg_state(struct bpf_verifier_env *env,
> > > > >  void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_func_state *state,
> > > > >                         bool print_all)
> > > > >  {
> > > > > +     struct bpf_verifier_state *vstate = env->cur_state;
> > > > 
> > > > This is not always true.
> > > > For example, __mark_chain_precision does 'print_verifier_state(env, func, true)'
> > > > for func obtained as 'func = st->frame[fr];' where 'st' iterates over parents
> > > > of env->cur_state.
> > > 
> > > Looking through the code, I'm thinking the only proper fix is
> > > explicitly passing in the verifier state, I was hoping there would be
> > > a link from func_state -> verifier_state but it is not the case.
> > > Regardless, explicitly passing in the verifier state is probably cleaner. WDYT?
> > 
> > Seems like it is (I'd also pass the frame number, instead of function
> > state pointer, just to make it clear where the function state comes from,
> > but feel free to ignore this suggestion).
> 
> I made this change, but not passing the frame number: while most call
> sites have the frame number (or pass curframe), it needs to be
> obtained explicitly for some, so I think it won't be worth it.

Understood, thank you.