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

[Eduard Zingerman <eddyz87@gmail.com>]

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@gmail.com>

lgtm, but please fix the 'print_verifier_state' note below.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> ---
>  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;
>  

[...]