Re: [PATCH v5 bpf-next 4/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/13 02:18PM, Andrii Nakryiko wrote:
> On Fri, Dec 13, 2024 at 5:08 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > of file descriptors: maps or btfs. This field was introduced as a
> > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > present, indicates that the fd_array is a continuous array of the
> > corresponding length.
> >
> > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > bound to the program, as if it was used by the program. This
> > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > maps can be used by the verifier during the program load.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  include/uapi/linux/bpf.h       |  10 ++++
> >  kernel/bpf/syscall.c           |   2 +-
> >  kernel/bpf/verifier.c          | 106 ++++++++++++++++++++++++++++-----
> >  tools/include/uapi/linux/bpf.h |  10 ++++
> >  4 files changed, 112 insertions(+), 16 deletions(-)
> >
> 
> [...]
> 
> >  int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
> >  {
> >         u64 start_time = ktime_get_ns();
> > @@ -22881,7 +22954,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >                 env->insn_aux_data[i].orig_idx = i;
> >         env->prog = *prog;
> >         env->ops = bpf_verifier_ops[env->prog->type];
> > -       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> >
> >         env->allow_ptr_leaks = bpf_allow_ptr_leaks(env->prog->aux->token);
> >         env->allow_uninit_stack = bpf_allow_uninit_stack(env->prog->aux->token);
> > @@ -22904,6 +22976,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> >         if (ret)
> >                 goto err_unlock;
> >
> > +       ret = process_fd_array(env, attr, uattr);
> > +       if (ret)
> > +               goto err_release_maps;
> 
> I think this should be goto skip_full_check, so that we can finalize
> verifier log (you do log an error if fd_array FD is invalid, right?)

Right... Thanks for catching it!

> If this is the only issue, this can probably be just patched up while applying.
> 
> > +
> >         mark_verifier_state_clean(env);
> >
> >         if (IS_ERR(btf_vmlinux)) {
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 4162afc6b5d0..2acf9b336371 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -1573,6 +1573,16 @@ union bpf_attr {
> >                  * If provided, prog_flags should have BPF_F_TOKEN_FD flag set.
> >                  */
> >                 __s32           prog_token_fd;
> > +               /* The fd_array_cnt can be used to pass the length of the
> > +                * fd_array array. In this case all the [map] file descriptors
> > +                * passed in this array will be bound to the program, even if
> > +                * the maps are not referenced directly. The functionality is
> > +                * similar to the BPF_PROG_BIND_MAP syscall, but maps can be
> > +                * used by the verifier during the program load. If provided,
> > +                * then the fd_array[0,...,fd_array_cnt-1] is expected to be
> > +                * continuous.
> > +                */
> > +               __u32           fd_array_cnt;
> >         };
> >
> >         struct { /* anonymous struct used by BPF_OBJ_* commands */
> > --
> > 2.34.1
> >
> >