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.
> /* 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.
> const struct bpf_reg_state *reg;
> int i;
>
[...]
