On 24/12/02 06:37PM, Alexei Starovoitov wrote:
> On Fri, Nov 29, 2024 at 5:29 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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
