On Tue, Nov 26, 2024 at 9:27 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> wrote:
>
> On 24/11/25 05:38PM, Alexei Starovoitov wrote:
> > On Tue, Nov 19, 2024 at 2:17 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 | 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.
>
> > > +
> > > + 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
>
