On Thu, 28 Nov 2024 at 03:39, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:
> > Currently, state for RCU read locks and preemption is in
> > bpf_verifier_state, while locks and pointer reference state remains in
> > bpf_func_state. There is no particular reason to keep the latter in
> > bpf_func_state. Additionally, it is copied into a new frame's state and
> > copied back to the caller frame's state everytime the verifier processes
> > a pseudo call instruction. This is a bit wasteful, given this state is
> > global for a given verification state / path.
> >
> > Move all resource and reference related state in bpf_verifier_state
> > structure in this patch, in preparation for introducing new reference
> > state types in the future.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
>
> lgtm, but please fix the 'print_verifier_state' note below.
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> > ---
> > include/linux/bpf_verifier.h | 11 ++--
> > kernel/bpf/log.c | 11 ++--
> > kernel/bpf/verifier.c | 112 ++++++++++++++++-------------------
> > 3 files changed, 64 insertions(+), 70 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index f4290c179bee..af64b5415df8 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -315,9 +315,6 @@ struct bpf_func_state {
> > u32 callback_depth;
> >
> > /* The following fields should be last. See copy_func_state() */
> > - int acquired_refs;
> > - int active_locks;
> > - struct bpf_reference_state *refs;
> > /* The state of the stack. Each element of the array describes BPF_REG_SIZE
> > * (i.e. 8) bytes worth of stack memory.
> > * stack[0] represents bytes [*(r10-8)..*(r10-1)]
> > @@ -419,9 +416,13 @@ struct bpf_verifier_state {
> > u32 insn_idx;
> > u32 curframe;
> >
> > - bool speculative;
> > + struct bpf_reference_state *refs;
> > + u32 acquired_refs;
> > + u32 active_locks;
> > + u32 active_preempt_locks;
> > bool active_rcu_lock;
> > - u32 active_preempt_lock;
> > +
> > + bool speculative;
>
> Nit: pahole says there are two holes here:
>
> $ pahole kernel/bpf/verifier.o
> ...
> struct bpf_verifier_state {
> struct bpf_func_state * frame[8]; /* 0 64 */
> /* --- cacheline 1 boundary (64 bytes) --- */
> struct bpf_verifier_state * parent; /* 64 8 */
> u32 branches; /* 72 4 */
> u32 insn_idx; /* 76 4 */
> u32 curframe; /* 80 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct bpf_reference_state * refs; /* 88 8 */
> u32 acquired_refs; /* 96 4 */
> u32 active_locks; /* 100 4 */
> u32 active_preempt_locks; /* 104 4 */
> u32 active_irq_id; /* 108 4 */
> bool active_rcu_lock; /* 112 1 */
> bool speculative; /* 113 1 */
> bool used_as_loop_entry; /* 114 1 */
> bool in_sleepable; /* 115 1 */
> u32 first_insn_idx; /* 116 4 */
> u32 last_insn_idx; /* 120 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> /* --- cacheline 2 boundary (128 bytes) --- */
> struct bpf_verifier_state * loop_entry; /* 128 8 */
> u32 insn_hist_start; /* 136 4 */
> u32 insn_hist_end; /* 140 4 */
> u32 dfs_depth; /* 144 4 */
> u32 callback_unroll_depth; /* 148 4 */
> u32 may_goto_depth; /* 152 4 */
>
> /* size: 160, cachelines: 3, members: 22 */
> /* sum members: 148, holes: 2, sum holes: 8 */
>
> maybe move the 'refs' pointer?
> e.g. moving it after 'parent' makes both holes disappear.
Ack, will fix.
>
> > /* If this state was ever pointed-to by other state's loop_entry field
> > * this flag would be set to true. Used to avoid freeing such states
> > * while they are still in use.
> > diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> > index 4a858fdb6476..8b52e5b7504c 100644
> > --- 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?
>
> > const struct bpf_reg_state *reg;
> > int i;
> >
>
> [...]
>
