Re: [PATCH v2 bpf-next 3/6] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov <aspsk@isovalent.com>]

On 24/11/26 12:40PM, Alexei Starovoitov wrote:
> On Tue, Nov 26, 2024 at 10:51 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Nov 26, 2024 at 9:27 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > On 24/11/25 05:38PM, Alexei Starovoitov wrote:
> > > > On Tue, Nov 19, 2024 at 2:17 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, 113 insertions(+), 15 deletions(-)
> > > > >
> >
> > [...]
> >
> > > > > +/*
> > > > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is given.  In
> > > > > + * this case expect that every file descriptor in the array is either a map or
> > > > > + * a BTF, or a hole (0). Everything else is considered to be trash.
> > > > > + */
> > > > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > > > +{
> > > > > +       struct bpf_map *map;
> > > > > +       CLASS(fd, f)(fd);
> > > > > +       int ret;
> > > > > +
> > > > > +       map = __bpf_map_get(f);
> > > > > +       if (!IS_ERR(map)) {
> > > > > +               ret = add_used_map(env, map);
> > > > > +               if (ret < 0)
> > > > > +                       return ret;
> > > > > +               return 0;
> > > > > +       }
> > > > > +
> > > > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > > > +               return 0;
> > > > > +
> > > > > +       if (!fd)
> > > > > +               return 0;
> > > >
> > > > This is not allowed in new apis.
> > > > zero cannot be special.
> > >
> > > I thought that this is ok since I check for all possible valid FDs by this
> > > point: if fd=0 is a valid map or btf, then we will return it by this point.
> > >
> > > Why I wanted to keep 0 as a valid value, even if it is not pointing to any
> > > map/btf is for case when, like in libbpf gen, fd_array is populated with map
> > > fds starting from 0, and with btf fds from some offset, so in between there may
> > > be 0s. This is probably better to disallow this, and, if fd_array_cnt != 0,
> > > then to check if all [0...fd_array_cnt) elements are valid.
> >
> > If fd_array_cnt != 0 we can define that fd_array isn't sparse anymore
> > and every entry has to be valid. Let's do that.
> 
> Exactly.
> libbpf gen_loader has a very simplistic implementation of
> add_map_fd() and add_kfunc_btf_fd().
> It leaves gaps only to keep implementation simple.
> It can and probably should be changed to make it contiguous.

Ok, makes sense. I will send v3 based on all the comments.

For the libbpf gen_loader part, I can address this in one of the follow-ups
(when fd_array_cnt is actually set in libbpf).