Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/10 10:18AM, Andrii Nakryiko wrote:
> On Tue, Dec 10, 2024 at 7:57 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > >
> > > > This makes total sense to treat all BPF objects in fd_array the same
> > > > way. With BTFs the problem is that, currently, a btf fd can end up
> > > > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > > > it is to merge those two.
> > >
> > > So, currently during program load BTFs are parsed from file
> > > descriptors and are stored in two places: env->used_btfs and
> > > env->prog->aux->kfunc_btf_tab:
> > >
> > >   1) env->used_btfs populated only when a DW load with the
> > >      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
> > >
> > >   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
> > >      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
> > >      sorted by offset to allow faster search
> > >
> > > So, to merge them something like this might be done:
> > >
> > >   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
> > >      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
> > >      above.
> > >
> > >   2) On program load change (1) to add a btf to this new sorted
> > >      used_btfs. As there is no corresponding offset, just use
> > >      offset=-1 (not literally like this, as bsearch() wants unique
> > >      keys, so by offset=-1 an array of btfs, aka, old used_maps,
> > >      should be stored)
> > >
> > > Looks like this, conceptually, doesn't change things too much: kfuncs
> > > btfs will still be searchable in log(n) time, the "normal" btfs will
> > > still be searched in used_btfs in linear time.
> > >
> > > (The other way is to just allow kfunc btfs to be loaded from fd_array
> > > if fd_array_cnt != 0, as it is done now, but as you've mentioned
> > > before, you had other use cases in mind, so this won't work.)
> >
> > This is getting a bit too complex.
> > I think Andrii is asking to keep BTFs if they are in fd_array.
> > No need to combine kfunc_btf_tab and used_btfs.
> > I think adding BTFs from fd_array to prog->aux->used_btfs
> > should do it.
> 
> Exactly, no need to do major changes, let's just add those BTFs into
> used_btfs, that's all.

Added. However, I have a question here: how to add proper selftests? The btfs
listed in env->used_btfs are later copied to prog->aux->used_btfs, and are
never actually exposed to user-space in any way. So, one test I can think of is

  * passing a btf fd in fd_array on prog load
  * closing this btf fd and checking that id exists before closing the program
    (requires to wait until rcu sync to be sure that the btf wasn't destroyed,
    but still is refcounted)

Is this enough?

(I assume exposing used_btfs to user-space is also out of scope of this patch
set, right?)