Re: [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Sep 21, 2021 at 5:42 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> > Refactor internals of libbpf to allow adding custom SEC() handling logic
> > easily from outside of libbpf. To that effect, each SEC()-handling
> > registration sets mandatory program type/expected attach type for
> > a given prefix and can provide three callbacks called at different
> > points of BPF program lifetime:
> >
> >   - init callback for right after bpf_program is initialized and
> >   prog_type/expected_attach_type is set. This happens during
> >   bpf_object__open() step, close to the very end of constructing
> >   bpf_object, so all the libbpf APIs for querying and updating
> >   bpf_program properties should be available;
>
> Do you have a usecase in mind that would set this? USDT?

Don't have specific use case in mind, but this callback gives the
fully constructed `struct bpf_program` access to those custom
callbacks, so if there is any additional book keeping/feature
checking/whatever that needs to be done, this will be the earliest
point where some library/framework will discover the program. Felt
like an important addition, even if libbpf internally has no need for
it (because libbpf can always access struct bpf_program through its
own means).

>
> >   - pre-load callback is called right before BPF_PROG_LOAD command is
> >   called in the kernel. This callbacks has ability to set both
> >   bpf_program properties, as well as program load attributes, overriding
> >   and augmenting the standard libbpf handling of them;
>
> [...]
>
> > @@ -6094,6 +6100,44 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
> >       return 0;
> >  }
> >
> > +static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
> > +
> > +/* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
> > +static int libbpf_preload_prog(struct bpf_program *prog,
> > +                            struct bpf_prog_load_params *attr, long cookie)
> > +{
> > +     /* old kernels might not support specifying expected_attach_type */
> > +     if (prog->sec_def->is_exp_attach_type_optional &&
> > +         !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
> > +             attr->expected_attach_type = 0;
> > +
> > +     if (prog->sec_def->is_sleepable)
> > +             attr->prog_flags |= BPF_F_SLEEPABLE;
> > +
> > +     if ((prog->type == BPF_PROG_TYPE_TRACING ||
> > +          prog->type == BPF_PROG_TYPE_LSM ||
> > +          prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
> > +             int btf_obj_fd = 0, btf_type_id = 0, err;
> > +
> > +             err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
> > +             if (err)
> > +                     return err;
> > +
> > +             /* cache resolved BTF FD and BTF type ID in the prog */
> > +             prog->attach_btf_obj_fd = btf_obj_fd;
> > +             prog->attach_btf_id = btf_type_id;
> > +
> > +             /* but by now libbpf common logic is not utilizing
> > +              * prog->atach_btf_obj_fd/prog->attach_btf_id anymore because
> > +              * this callback is called after attrs were populated by
> > +              * libbpf, so this callback has to update attr explicitly here
> > +              */
> > +             attr->attach_btf_obj_fd = btf_obj_fd;
> > +             attr->attach_btf_id = btf_type_id;
> > +     }
> > +     return 0;
> > +}
> > +
> We talked on VC about some general approach questions I had here, will
> summarize. Discussion touched on changes in patches 5 and 6 as well. I thought
> the pulling of these chunks into libbpf_preload_prog made sense, but wondered
> whether some of this prog-type specific functionality would also be useful to
> "average" custom sec_def writer even if it's not considered 'standard libbpf
> handling', e.g. custom sec_def writer whose SEC produces a PROG_TYPE_TRACING
> is likely to want the find_attach_btf_id niceness as well. So perhaps something
> like the ability to chain the callbacks so that sec_def writer can use libbpf's
> would be useful.
>
> Your response was that you explicitly wanted to avoid doing this because this
> would result in libbpf's callbacks becoming part of the API and stability
> requirements following from that. Furthermore, you don't anticipate libbpf's
> preload callback becoming very complicated and expect that the average
> custom sec_def writer will be familiar enough with libbpf to be able to pull
> out whatever they need.
>
> Response made sense to me, LGTM
>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>