Re: [PATCH v3 bpf-next 5/7] selftests/bpf: Add tests for fd_array_cnt

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/02 06:37PM, Alexei Starovoitov wrote:
> On Fri, Nov 29, 2024 at 5:29 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > Add a new set of tests to test the new field in PROG_LOAD-related
> > part of bpf_attr: fd_array_cnt.
> >
> > Add the following test cases:
> >
> >   * fd_array_cnt/no-fd-array: program is loaded in a normal
> >     way, without any fd_array present
> >
> >   * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
> >     check that they're bound to the program
> >
> >   * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
> >     only two of which are unique
> >
> >   * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
> >     fd_array which is also referenced from within the program
> >
> >   * fd_array_cnt/fd-array-trash-input: pass array with some trash
> >
> >   * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)
> >
> >   * fd_array_cnt/fd-array-2big: pass too large array
> >
> > All the tests above are using the bpf(2) syscall directly,
> > no libbpf involved.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  kernel/bpf/verifier.c                         |  30 +-
> >  .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
> >  2 files changed, 355 insertions(+), 15 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index d172f6974fd7..7102d85f580d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -22620,7 +22620,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >         env->ops = bpf_verifier_ops[env->prog->type];
> >         ret = init_fd_array(env, attr, uattr);
> >         if (ret)
> > -               goto err_free_aux_data;
> > +               goto err_release_maps;
> >
> >         env->allow_ptr_leaks = bpf_allow_ptr_leaks(env->prog->aux->token);
> >         env->allow_uninit_stack = bpf_allow_uninit_stack(env->prog->aux->token);
> > @@ -22773,11 +22773,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >             copy_to_bpfptr_offset(uattr, offsetof(union bpf_attr, log_true_size),
> >                                   &log_true_size, sizeof(log_true_size))) {
> >                 ret = -EFAULT;
> > -               goto err_release_maps;
> > +               goto err_ext;
> >         }
> >
> >         if (ret)
> > -               goto err_release_maps;
> > +               goto err_ext;
> >
> >         if (env->used_map_cnt) {
> >                 /* if program passed verifier, update used_maps in bpf_prog_info */
> > @@ -22787,7 +22787,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >
> >                 if (!env->prog->aux->used_maps) {
> >                         ret = -ENOMEM;
> > -                       goto err_release_maps;
> > +                       goto err_ext;
> >                 }
> >
> >                 memcpy(env->prog->aux->used_maps, env->used_maps,
> > @@ -22801,7 +22801,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >                                                           GFP_KERNEL);
> >                 if (!env->prog->aux->used_btfs) {
> >                         ret = -ENOMEM;
> > -                       goto err_release_maps;
> > +                       goto err_ext;
> >                 }
> >
> >                 memcpy(env->prog->aux->used_btfs, env->used_btfs,
> > @@ -22817,15 +22817,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >
> >         adjust_btf_func(env);
> >
> > -err_release_maps:
> > -       if (!env->prog->aux->used_maps)
> > -               /* if we didn't copy map pointers into bpf_prog_info, release
> > -                * them now. Otherwise free_used_maps() will release them.
> > -                */
> > -               release_maps(env);
> > -       if (!env->prog->aux->used_btfs)
> > -               release_btfs(env);
> > -
> > +err_ext:
> >         /* extension progs temporarily inherit the attach_type of their targets
> >            for verification purposes, so set it back to zero before returning
> >          */
> > @@ -22838,7 +22830,15 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >  err_unlock:
> >         if (!is_priv)
> >                 mutex_unlock(&bpf_verifier_lock);
> > -err_free_aux_data:
> > +err_release_maps:
> > +       if (!env->prog->aux->used_maps)
> > +               /* if we didn't copy map pointers into bpf_prog_info, release
> > +                * them now. Otherwise free_used_maps() will release them.
> > +                */
> > +               release_maps(env);
> > +       if (!env->prog->aux->used_btfs)
> > +               release_btfs(env);
> > +
> >         vfree(env->insn_aux_data);
> >         kvfree(env->insn_hist);
> 
> verifier.c hunk shouldn't be mixed with the selftests.
> 
> Looks like it should be in patch 3?

Sorry, wrong `rebase -i`

> Not sure what it does though.

Yeah, thanks. I've simplified this part of Patch 3,
this diff will not appear in v4