Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:48 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          | 98 ++++++++++++++++++++++++++++------
> >  tools/include/uapi/linux/bpf.h | 10 ++++
> >  4 files changed, 104 insertions(+), 16 deletions(-)
> >
> 
> [...]
> 
> > +/*
> > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > + * this case expect that every file descriptor in the array is either a map or
> > + * a BTF. 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;
> > +       }
> > +
> > +       /*
> > +        * Unlike "unused" maps which do not appear in the BPF program,
> > +        * BTFs are visible, so no reason to refcnt them now
> 
> What does "BTFs are visible" mean? I find this behavior surprising,
> tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> Why?

This functionality is added to catch maps, and work with them during
verification, which aren't otherwise referenced by program code. The
actual application is those "instructions set" maps for static keys.
All other objects are "visible" during verification.

> > +        */
> > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > +               return 0;
> > +
> > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > +       return PTR_ERR(map);
> > +}
> > +
> > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > +{
> > +       size_t size = sizeof(int);
> > +       int ret;
> > +       int fd;
> > +       u32 i;
> > +
> > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > +
> > +       /*
> > +        * The only difference between old (no fd_array_cnt is given) and new
> > +        * APIs is that in the latter case the fd_array is expected to be
> > +        * continuous and is scanned for map fds right away
> > +        */
> > +       if (!attr->fd_array_cnt)
> > +               return 0;
> > +
> > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> 
> potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> less than INT_MAX/4?

Right. So, probably cap to (UINT_MAX/size)?

> > +                       return -EFAULT;
> > +
> > +               ret = add_fd_from_fd_array(env, fd);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> [...]