On Thu, 28 Nov 2024 at 04:22, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Thu, 2024-11-28 at 04:18 +0100, Kumar Kartikeya Dwivedi wrote:
> > On Thu, 28 Nov 2024 at 04:03, Eduard Zingerman <eddyz87@xxxxxxxxx> 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.
>
Ok, scratch the previous reply, I forgot you can actually do
func->frameno to get it, I was trying dumb things (like func -
st->frame).
I do agree it's better to pass the frameno, just for the off chance
that you end up passing vstate and funcs that mismatch.
So I ended up making the change in the end. Sorry for the confusion.
