On 24/03/15 10:29, Andrii Nakryiko wrote:
> On Fri, Mar 15, 2024 at 10:23 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > 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.
>
> 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?
