On Fri, Mar 15, 2024 at 9:33 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Mar 14, 2024 at 2:41 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Thu, Mar 14, 2024 at 1:06 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> 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.
