diff mbox series

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

Message ID 20241127165846.2001009-2-memxor@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series IRQ save/restore | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers warning 8 maintainers not CCed: kpsingh@kernel.org jolsa@kernel.org song@kernel.org haoluo@google.com john.fastabend@gmail.com yonghong.song@linux.dev martin.lau@linux.dev sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 67 this patch: 67
netdev/checkpatch warning WARNING: line length of 101 exceeds 80 columns WARNING: line length of 110 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kumar Kartikeya Dwivedi Nov. 27, 2024, 4:58 p.m. UTC 
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>
---
 include/linux/bpf_verifier.h |  11 ++--
 kernel/bpf/log.c             |  11 ++--
 kernel/bpf/verifier.c        | 112 ++++++++++++++++-------------------
 3 files changed, 64 insertions(+), 70 deletions(-)

Comments

Eduard Zingerman Nov. 28, 2024, 2:39 a.m. UTC | #1 
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;
>  

[...]
Kumar Kartikeya Dwivedi Nov. 28, 2024, 2:54 a.m. UTC | #2 
On Thu, 28 Nov 2024 at 03:39, Eduard Zingerman <eddyz87@gmail.com> 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@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.

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;
> >
>
> [...]
>
Eduard Zingerman Nov. 28, 2024, 3:03 a.m. UTC | #3 
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).

[...]
Kumar Kartikeya Dwivedi Nov. 28, 2024, 3:18 a.m. UTC | #4 
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.

>
> [...]
>
Eduard Zingerman Nov. 28, 2024, 3:22 a.m. UTC | #5 
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.
Kumar Kartikeya Dwivedi Nov. 28, 2024, 3:32 a.m. UTC | #6 
On Thu, 28 Nov 2024 at 04:22, Eduard Zingerman <eddyz87@gmail.com> 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@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.
>

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.
diff mbox series

Patch

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;
 	/* 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;
 	const struct bpf_reg_state *reg;
 	int i;
 
@@ -843,11 +844,11 @@  void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_func_st
 			break;
 		}
 	}
-	if (state->acquired_refs && state->refs[0].id) {
-		verbose(env, " refs=%d", state->refs[0].id);
-		for (i = 1; i < state->acquired_refs; i++)
-			if (state->refs[i].id)
-				verbose(env, ",%d", state->refs[i].id);
+	if (vstate->acquired_refs && vstate->refs[0].id) {
+		verbose(env, " refs=%d", vstate->refs[0].id);
+		for (i = 1; i < vstate->acquired_refs; i++)
+			if (vstate->refs[i].id)
+				verbose(env, ",%d", vstate->refs[i].id);
 	}
 	if (state->in_callback_fn)
 		verbose(env, " cb");
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c4ebb326785..f8313e95eb8e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1279,15 +1279,17 @@  static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
 	return arr ? arr : ZERO_SIZE_PTR;
 }
 
-static int copy_reference_state(struct bpf_func_state *dst, const struct bpf_func_state *src)
+static int copy_reference_state(struct bpf_verifier_state *dst, const struct bpf_verifier_state *src)
 {
 	dst->refs = copy_array(dst->refs, src->refs, src->acquired_refs,
 			       sizeof(struct bpf_reference_state), GFP_KERNEL);
 	if (!dst->refs)
 		return -ENOMEM;
 
-	dst->active_locks = src->active_locks;
 	dst->acquired_refs = src->acquired_refs;
+	dst->active_locks = src->active_locks;
+	dst->active_preempt_locks = src->active_preempt_locks;
+	dst->active_rcu_lock = src->active_rcu_lock;
 	return 0;
 }
 
@@ -1304,7 +1306,7 @@  static int copy_stack_state(struct bpf_func_state *dst, const struct bpf_func_st
 	return 0;
 }
 
-static int resize_reference_state(struct bpf_func_state *state, size_t n)
+static int resize_reference_state(struct bpf_verifier_state *state, size_t n)
 {
 	state->refs = realloc_array(state->refs, state->acquired_refs, n,
 				    sizeof(struct bpf_reference_state));
@@ -1349,7 +1351,7 @@  static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
  */
 static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
 {
-	struct bpf_func_state *state = cur_func(env);
+	struct bpf_verifier_state *state = env->cur_state;
 	int new_ofs = state->acquired_refs;
 	int id, err;
 
@@ -1367,7 +1369,7 @@  static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
 static int acquire_lock_state(struct bpf_verifier_env *env, int insn_idx, enum ref_state_type type,
 			      int id, void *ptr)
 {
-	struct bpf_func_state *state = cur_func(env);
+	struct bpf_verifier_state *state = env->cur_state;
 	int new_ofs = state->acquired_refs;
 	int err;
 
@@ -1384,7 +1386,7 @@  static int acquire_lock_state(struct bpf_verifier_env *env, int insn_idx, enum r
 }
 
 /* release function corresponding to acquire_reference_state(). Idempotent. */
-static int release_reference_state(struct bpf_func_state *state, int ptr_id)
+static int release_reference_state(struct bpf_verifier_state *state, int ptr_id)
 {
 	int i, last_idx;
 
@@ -1404,7 +1406,7 @@  static int release_reference_state(struct bpf_func_state *state, int ptr_id)
 	return -EINVAL;
 }
 
-static int release_lock_state(struct bpf_func_state *state, int type, int id, void *ptr)
+static int release_lock_state(struct bpf_verifier_state *state, int type, int id, void *ptr)
 {
 	int i, last_idx;
 
@@ -1425,10 +1427,9 @@  static int release_lock_state(struct bpf_func_state *state, int type, int id, vo
 	return -EINVAL;
 }
 
-static struct bpf_reference_state *find_lock_state(struct bpf_verifier_env *env, enum ref_state_type type,
+static struct bpf_reference_state *find_lock_state(struct bpf_verifier_state *state, enum ref_state_type type,
 						   int id, void *ptr)
 {
-	struct bpf_func_state *state = cur_func(env);
 	int i;
 
 	for (i = 0; i < state->acquired_refs; i++) {
@@ -1447,7 +1448,6 @@  static void free_func_state(struct bpf_func_state *state)
 {
 	if (!state)
 		return;
-	kfree(state->refs);
 	kfree(state->stack);
 	kfree(state);
 }
@@ -1461,6 +1461,7 @@  static void free_verifier_state(struct bpf_verifier_state *state,
 		free_func_state(state->frame[i]);
 		state->frame[i] = NULL;
 	}
+	kfree(state->refs);
 	if (free_self)
 		kfree(state);
 }
@@ -1471,12 +1472,7 @@  static void free_verifier_state(struct bpf_verifier_state *state,
 static int copy_func_state(struct bpf_func_state *dst,
 			   const struct bpf_func_state *src)
 {
-	int err;
-
-	memcpy(dst, src, offsetof(struct bpf_func_state, acquired_refs));
-	err = copy_reference_state(dst, src);
-	if (err)
-		return err;
+	memcpy(dst, src, offsetof(struct bpf_func_state, stack));
 	return copy_stack_state(dst, src);
 }
 
@@ -1493,9 +1489,10 @@  static int copy_verifier_state(struct bpf_verifier_state *dst_state,
 		free_func_state(dst_state->frame[i]);
 		dst_state->frame[i] = NULL;
 	}
+	err = copy_reference_state(dst_state, src);
+	if (err)
+		return err;
 	dst_state->speculative = src->speculative;
-	dst_state->active_rcu_lock = src->active_rcu_lock;
-	dst_state->active_preempt_lock = src->active_preempt_lock;
 	dst_state->in_sleepable = src->in_sleepable;
 	dst_state->curframe = src->curframe;
 	dst_state->branches = src->branches;
@@ -5496,7 +5493,7 @@  static bool in_sleepable(struct bpf_verifier_env *env)
 static bool in_rcu_cs(struct bpf_verifier_env *env)
 {
 	return env->cur_state->active_rcu_lock ||
-	       cur_func(env)->active_locks ||
+	       env->cur_state->active_locks ||
 	       !in_sleepable(env);
 }
 
@@ -7850,15 +7847,15 @@  static int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg
  * Since only one bpf_spin_lock is allowed the checks are simpler than
  * reg_is_refcounted() logic. The verifier needs to remember only
  * one spin_lock instead of array of acquired_refs.
- * cur_func(env)->active_locks remembers which map value element or allocated
+ * env->cur_state->active_locks remembers which map value element or allocated
  * object got locked and clears it after bpf_spin_unlock.
  */
 static int process_spin_lock(struct bpf_verifier_env *env, int regno,
 			     bool is_lock)
 {
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
+	struct bpf_verifier_state *cur = env->cur_state;
 	bool is_const = tnum_is_const(reg->var_off);
-	struct bpf_func_state *cur = cur_func(env);
 	u64 val = reg->var_off.value;
 	struct bpf_map *map = NULL;
 	struct btf *btf = NULL;
@@ -7925,7 +7922,7 @@  static int process_spin_lock(struct bpf_verifier_env *env, int regno,
 			return -EINVAL;
 		}
 
-		if (release_lock_state(cur_func(env), REF_TYPE_LOCK, reg->id, ptr)) {
+		if (release_lock_state(env->cur_state, REF_TYPE_LOCK, reg->id, ptr)) {
 			verbose(env, "bpf_spin_unlock of different lock\n");
 			return -EINVAL;
 		}
@@ -9679,7 +9676,7 @@  static int release_reference(struct bpf_verifier_env *env,
 	struct bpf_reg_state *reg;
 	int err;
 
-	err = release_reference_state(cur_func(env), ref_obj_id);
+	err = release_reference_state(env->cur_state, ref_obj_id);
 	if (err)
 		return err;
 
@@ -9757,9 +9754,7 @@  static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int calls
 			callsite,
 			state->curframe + 1 /* frameno within this callchain */,
 			subprog /* subprog number within this prog */);
-	/* Transfer references to the callee */
-	err = copy_reference_state(callee, caller);
-	err = err ?: set_callee_state_cb(env, caller, callee, callsite);
+	err = set_callee_state_cb(env, caller, callee, callsite);
 	if (err)
 		goto err_out;
 
@@ -9992,14 +9987,14 @@  static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		const char *sub_name = subprog_name(env, subprog);
 
 		/* Only global subprogs cannot be called with a lock held. */
-		if (cur_func(env)->active_locks) {
+		if (env->cur_state->active_locks) {
 			verbose(env, "global function calls are not allowed while holding a lock,\n"
 				     "use static function instead\n");
 			return -EINVAL;
 		}
 
 		/* Only global subprogs cannot be called with preemption disabled. */
-		if (env->cur_state->active_preempt_lock) {
+		if (env->cur_state->active_preempt_locks) {
 			verbose(env, "global function calls are not allowed with preemption disabled,\n"
 				     "use static function instead\n");
 			return -EINVAL;
@@ -10333,11 +10328,6 @@  static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 		caller->regs[BPF_REG_0] = *r0;
 	}
 
-	/* Transfer references to the caller */
-	err = copy_reference_state(caller, callee);
-	if (err)
-		return err;
-
 	/* for callbacks like bpf_loop or bpf_for_each_map_elem go back to callsite,
 	 * there function call logic would reschedule callback visit. If iteration
 	 * converges is_state_visited() would prune that visit eventually.
@@ -10502,11 +10492,11 @@  record_func_key(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
 
 static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exit)
 {
-	struct bpf_func_state *state = cur_func(env);
+	struct bpf_verifier_state *state = env->cur_state;
 	bool refs_lingering = false;
 	int i;
 
-	if (!exception_exit && state->frameno)
+	if (!exception_exit && cur_func(env)->frameno)
 		return 0;
 
 	for (i = 0; i < state->acquired_refs; i++) {
@@ -10523,7 +10513,7 @@  static int check_resource_leak(struct bpf_verifier_env *env, bool exception_exit
 {
 	int err;
 
-	if (check_lock && cur_func(env)->active_locks) {
+	if (check_lock && env->cur_state->active_locks) {
 		verbose(env, "%s cannot be used inside bpf_spin_lock-ed region\n", prefix);
 		return -EINVAL;
 	}
@@ -10539,7 +10529,7 @@  static int check_resource_leak(struct bpf_verifier_env *env, bool exception_exit
 		return -EINVAL;
 	}
 
-	if (check_lock && env->cur_state->active_preempt_lock) {
+	if (check_lock && env->cur_state->active_preempt_locks) {
 		verbose(env, "%s cannot be used inside bpf_preempt_disable-ed region\n", prefix);
 		return -EINVAL;
 	}
@@ -10727,7 +10717,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			env->insn_aux_data[insn_idx].storage_get_func_atomic = true;
 	}
 
-	if (env->cur_state->active_preempt_lock) {
+	if (env->cur_state->active_preempt_locks) {
 		if (fn->might_sleep) {
 			verbose(env, "sleepable helper %s#%d in non-preemptible region\n",
 				func_id_name(func_id), func_id);
@@ -10784,7 +10774,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			struct bpf_func_state *state;
 			struct bpf_reg_state *reg;
 
-			err = release_reference_state(cur_func(env), ref_obj_id);
+			err = release_reference_state(env->cur_state, ref_obj_id);
 			if (!err) {
 				bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
 					if (reg->ref_obj_id == ref_obj_id) {
@@ -11746,7 +11736,7 @@  static int ref_set_non_owning(struct bpf_verifier_env *env, struct bpf_reg_state
 {
 	struct btf_record *rec = reg_btf_record(reg);
 
-	if (!cur_func(env)->active_locks) {
+	if (!env->cur_state->active_locks) {
 		verbose(env, "verifier internal error: ref_set_non_owning w/o active lock\n");
 		return -EFAULT;
 	}
@@ -11765,12 +11755,11 @@  static int ref_set_non_owning(struct bpf_verifier_env *env, struct bpf_reg_state
 
 static int ref_convert_owning_non_owning(struct bpf_verifier_env *env, u32 ref_obj_id)
 {
-	struct bpf_func_state *state, *unused;
+	struct bpf_verifier_state *state = env->cur_state;
+	struct bpf_func_state *unused;
 	struct bpf_reg_state *reg;
 	int i;
 
-	state = cur_func(env);
-
 	if (!ref_obj_id) {
 		verbose(env, "verifier internal error: ref_obj_id is zero for "
 			     "owning -> non-owning conversion\n");
@@ -11860,9 +11849,9 @@  static int check_reg_allocation_locked(struct bpf_verifier_env *env, struct bpf_
 	}
 	id = reg->id;
 
-	if (!cur_func(env)->active_locks)
+	if (!env->cur_state->active_locks)
 		return -EINVAL;
-	s = find_lock_state(env, REF_TYPE_LOCK, id, ptr);
+	s = find_lock_state(env->cur_state, REF_TYPE_LOCK, id, ptr);
 	if (!s) {
 		verbose(env, "held lock and object are not in the same allocation\n");
 		return -EINVAL;
@@ -12789,17 +12778,17 @@  static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		return -EINVAL;
 	}
 
-	if (env->cur_state->active_preempt_lock) {
+	if (env->cur_state->active_preempt_locks) {
 		if (preempt_disable) {
-			env->cur_state->active_preempt_lock++;
+			env->cur_state->active_preempt_locks++;
 		} else if (preempt_enable) {
-			env->cur_state->active_preempt_lock--;
+			env->cur_state->active_preempt_locks--;
 		} else if (sleepable) {
 			verbose(env, "kernel func %s is sleepable within non-preemptible region\n", func_name);
 			return -EACCES;
 		}
 	} else if (preempt_disable) {
-		env->cur_state->active_preempt_lock++;
+		env->cur_state->active_preempt_locks++;
 	} else if (preempt_enable) {
 		verbose(env, "unmatched attempt to enable preemption (kernel function %s)\n", func_name);
 		return -EINVAL;
@@ -15398,7 +15387,7 @@  static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
 		 * No one could have freed the reference state before
 		 * doing the NULL check.
 		 */
-		WARN_ON_ONCE(release_reference_state(state, id));
+		WARN_ON_ONCE(release_reference_state(vstate, id));
 
 	bpf_for_each_reg_in_vstate(vstate, state, reg, ({
 		mark_ptr_or_null_reg(state, reg, id, is_null);
@@ -17750,7 +17739,7 @@  static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
 	return true;
 }
 
-static bool refsafe(struct bpf_func_state *old, struct bpf_func_state *cur,
+static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *cur,
 		    struct bpf_idmap *idmap)
 {
 	int i;
@@ -17758,6 +17747,15 @@  static bool refsafe(struct bpf_func_state *old, struct bpf_func_state *cur,
 	if (old->acquired_refs != cur->acquired_refs)
 		return false;
 
+	if (old->active_locks != cur->active_locks)
+		return false;
+
+	if (old->active_preempt_locks != cur->active_preempt_locks)
+		return false;
+
+	if (old->active_rcu_lock != cur->active_rcu_lock)
+		return false;
+
 	for (i = 0; i < old->acquired_refs; i++) {
 		if (!check_ids(old->refs[i].id, cur->refs[i].id, idmap) ||
 		    old->refs[i].type != cur->refs[i].type)
@@ -17820,9 +17818,6 @@  static bool func_states_equal(struct bpf_verifier_env *env, struct bpf_func_stat
 	if (!stacksafe(env, old, cur, &env->idmap_scratch, exact))
 		return false;
 
-	if (!refsafe(old, cur, &env->idmap_scratch))
-		return false;
-
 	return true;
 }
 
@@ -17850,13 +17845,10 @@  static bool states_equal(struct bpf_verifier_env *env,
 	if (old->speculative && !cur->speculative)
 		return false;
 
-	if (old->active_rcu_lock != cur->active_rcu_lock)
-		return false;
-
-	if (old->active_preempt_lock != cur->active_preempt_lock)
+	if (old->in_sleepable != cur->in_sleepable)
 		return false;
 
-	if (old->in_sleepable != cur->in_sleepable)
+	if (!refsafe(old, cur, &env->idmap_scratch))
 		return false;
 
 	/* for states to be equal callsites have to be the same
@@ -18751,7 +18743,7 @@  static int do_check(struct bpf_verifier_env *env)
 					return -EINVAL;
 				}
 
-				if (cur_func(env)->active_locks) {
+				if (env->cur_state->active_locks) {
 					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
 					    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
 					     (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {