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 10:51AM, Andrii Nakryiko 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.

Yes, makes sense

> >
> > > > +
> > > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > > +       return PTR_ERR(map);
> > > > +}
> > > > +
> > > > +static int env_init_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > >
> > > What an odd name... why is 'env_' there?
> >
> 
> [...]
> 
> > > I don't get this feature.
> > > Why bother copying and checking for validity?
> > > What does it buy ?
> >
> > So, the main reason for this whole change is to allow unrelated maps, which
> > aren't referenced by a program directly, to be noticed and available during the
> > verification. Thus, I want to go through the array here and add them to
> > used_maps. (In a consequent patch, "instuction sets" maps are treated
> > additionally at this point as well.)
> >
> > The reason to discard that copy here was that "old api" when fd_array_cnt is 0
> > is still valid and in this case we can't copy fd_array in full. Later during
> > the verification fd_array elements are accessed individually by copying from
> > bpfptr. I thought that freeing this copy here is more readable than to add
> > a check at every such occasion.
> 
> I think Alexei meant why you need to make an entire copy of fd_array,
> if you can just read one element at a time (still with
> copy_from_bpfptr_offset()), validate/add map/btf from that FD, and
> move to the next element. No need to make a copy of an array.
> 
> >
> > > pw-bot: cr
> >