Re: [PATCH v1 bpf-next 3/9] bpf: expose how xlated insns map to jitted insns

[Anton Protopopov <aspsk@isovalent.com>]

On Fri, Mar 29, 2024 at 03:44:04PM -0700, Andrii Nakryiko wrote:
> On Thu, Mar 28, 2024 at 9:37 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > On 24/03/15 10:29, Andrii Nakryiko wrote:
> > > On Fri, Mar 15, 2024 at 10:23 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, Mar 15, 2024 at 9:33 AM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > > >
> > > > > On Thu, Mar 14, 2024 at 2:41 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Mar 14, 2024 at 1:06 PM Andrii Nakryiko
> > > > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > > > >
> > > > > > > > > What could work and what I am proposing is to pass a list of bound
> > > > > > > > > maps in prog_load attributes. Then such maps can be used during the
> > > > > > > > > verification. For normal maps
> > > > > > > > >
> > > > > > > > >   prog = prog_load(attr={.bound_maps=maps})
> > > > > > > > >
> > > > > > > > > will be semantically the same as
> > > > > > > > >
> > > > > > > > >   prog = prog_load()
> > > > > > > > >   bpf_prog_bind_map(prog, maps)
> > > > > > > >
> > > > > > > > Instead of a whole new api, let's make libbpf insert
> > > > > > > > ld_imm64 r0, map
> > > > > > > > as the first insn for this case for now.
> > > > > > >
> > > > > > > This sounds like a big hack and unnecessary complication, tbh. I'd
> > > > > > > like to avoid having to do this in libbpf.
> > > > > > >
> > > > > > > But I think we almost have this already supported. In BPF_PROG_LOAD
> > > > > > > UAPI we have fd_array property, right? I think right now we lazily
> > > > > > > refcnt referenced maps. But I think it makes total sense to just
> > > > > > > define that bpf_prog will keep references to all BPF objects passed in
> > > > > > > through fd_array, WDYT? Verifier will just iterate all provided FDs,
> > > > > > > determine kind of BPF object it is (and reject unknown ones), and then
> > > > > > > just take refcnts on each of them once. On prog free we'll just do the
> > > > > > > same in reverse and be done with it.
> > > > > > >
> > > > > > > It also can be used as a batch and single-step (in the sense it will
> > > > > > > be done as part of program load instead of a separate command)
> > > > > > > alternative for bpf_prog_bind_map(), I suppose.
> > > > > >
> > > > > > fd_array approach also works. There can be map and btf fds in there.
> > > > > > I would only bind maps this way.
> > > > >
> > > > > Any reason why we should have non-uniform behavior between maps and
> > > > > BTFs? Seems more error-prone to have a difference here, tbh.
> > > >
> > > > because maps are only held in used_maps while btfs are held
> > > > in used_btfs and in kfunc_btf_tab.
> > > > And looking at btf_fd it's not clear whether it will be in ld_imm64
> > > > and hence used_btf or it's kfunc and will be in kfunc_btf_tab.
> > > > All btfs can be stored unconditionally in used_btf,
> > > > but that's unnecessary refcnt inc and module_get too.
> > > > Doesn't hurt, but makes it harder to reason about everything.
> > > > At least to me.
> > > > I guess if the whole refcnt of maps and btfs is factored out
> > > > and cleaned up into uniform used_maps/used_btf then it's ok,
> > > > but fd_array is optional. So it feels messy.
> > >
> > > Yeah, I was imagining that we'd iterate fd_array (if it's provided)
> > > and add any map/btf into used_{map,btf}, refcnt. Then during
> > > verification we'll just know that any referenced map or btf from
> > > fd_array is already refcounted, so we wouldn't do it there. But I
> > > haven't looked at kfunc_btf_tab, if that's causing some troubles with
> > > this approach, then it's fine by me.
> > >
> > > The assumption was that a uniform approach will be less messy and
> > > simplify code and reasoning about the behavior, not the other way. If
> > > that's not the case we can do it just for maps for now.
> >
> > fd_array is sent in attrs without specifying its size, so individual
> > fds are copied to kernel as needed. Therefore, this is not possible
> > to pass extra fds (not used directly by the program) without changing
> > the API. So either a pair of new fields, say, (fd_extra,fd_extra_len),
> > or just another field fd_array_len should be added. What sounds better?
> 
> I'd say we should extend fd_array with (optional) fd_array_cnt and
> have the following logic:
> 
>   - if fd_array != NULL and fd_array_cnt == 0, then only maps/btfs
> referenced from BPF program instructions should be refcnt/fetched;
>   - if fd_array != NULL and fd_array_cnt > 0, we can eagerly fetch all
> FDs and refcnt, as discussed. If any instruction uses the fd index
> which is >= fd_array_cnt, that's an error (the user didn't provide a
> big enough FD array and we can now detect this).
> 
> WDYT?

Yes, thanks, I've thought the same way. I will use this API