On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> wrote:
>
> On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > > 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.
That's your specific intended use case, but API is semantically more
generic and shouldn't tailor to your specific interpretation on how it
will/should be used. I think this is a landmine to add reference to
just BPF maps and not to BTF objects, we won't be able to retrofit the
proper and uniform treatment later without extra flags or backwards
compatibility breakage.
Even though we don't need extra "detached" BTF objects associated with
BPF program, right now, I can anticipate some interesting use case
where we might want to attach additional BTF objects to BPF programs
(for whatever reasons, BTFs are a convenient bag of strings and
graph-based types, so could be useful for extra
debugging/metadata/whatever information).
So I can see only two ways forward. Either we disable BTFs in fd_array
if fd_array_cnt>0, which will prevent its usage from light skeleton,
so not great. Or we bump refcount both BPF maps and BTFs in fd_array.
The latter seems saner and I don't think is a problem at all, we
already have used_btfs that function similarly to used_maps.
>
> > > + */
> > > + 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)?
either that or use check_mul_overflow()
>
> > > + return -EFAULT;
> > > +
> > > + ret = add_fd_from_fd_array(env, fd);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > [...]
