On Thu, Jul 4, 2024 at 1:31 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-07-03 at 17:15 -0700, Andrii Nakryiko wrote:
> > Old versions of libbpf don't handle varying sizes of bpf_map_skeleton
> > struct correctly. As such, BPF skeleton generated by newest bpftool
> > might not be compatible with older libbpf (though only when libbpf is
> > used as a shared library), even though it, by design, should.
> >
> > Going forward libbpf will be fixed, plus we'll release bug fixed
> > versions of relevant old libbpfs, but meanwhile try to mitigate from
> > bpftool side by conservatively assuming older and smaller definition of
> > bpf_map_skeleton, if possible. Meaning, if there are no struct_ops maps.
> >
> > If there are struct_ops, then presumably user would like to have
> > auto-attaching logic and struct_ops map link placeholders, so use the
> > full bpf_map_skeleton definition in that case.
> >
> > Co-developed-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> Silly question, here is a fragment of the profiler.skel.h generated
> with bpftool build (tools/bpf/bpftool/profiler.skel.h):
>
> static inline int
> profiler_bpf__create_skeleton(struct profiler_bpf *obj)
> {
> /* ... */
>
> map = (struct bpf_map_skeleton *)((char *)s->maps + 0 * s->map_skel_sz);
> map->name = "events";
> map->map = &obj->maps.events;
>
> /* ... 4 more like this ... */
>
> /* ... */
>
> s->progs[0].name = "fentry_XXX";
> s->progs[0].prog = &obj->progs.fentry_XXX;
> s->progs[0].link = &obj->links.fentry_XXX;
>
> s->progs[1].name = "fexit_XXX";
> s->progs[1].prog = &obj->progs.fexit_XXX;
> s->progs[1].link = &obj->links.fexit_XXX;
>
> /* ... */
> }
>
> Do we need to handle 'progs' array access in a same way as maps?
Given bpf_prog_skeleton has never been extended yet (and maybe never
will be), I chose not to uglify this unnecessarily. My thinking/hope
is that by the time we get to extending prog_skeleton struct, all
actively used libbpf versions will be patched up and will handle this
correctly without the hacks we have to do for map_skeleton.
>
> [...]
>
> > @@ -878,23 +895,22 @@ codegen_maps_skeleton(struct bpf_object *obj, size_t map_cnt, bool mmaped, bool
> >
> > codegen("\
> > \n\
> > - \n\
> > - s->maps[%zu].name = \"%s\"; \n\
> > - s->maps[%zu].map = &obj->maps.%s; \n\
> > + \n\
> > + map = (struct bpf_map_skeleton *)((char *)s->maps + %zu * s->map_skel_sz);\n\
> > + map->name = \"%s\"; \n\
> > + map->map = &obj->maps.%s; \n\
> > ",
> > - i, bpf_map__name(map), i, ident);
> > + i, bpf_map__name(map), ident);
> > /* memory-mapped internal maps */
> > if (mmaped && is_mmapable_map(map, ident, sizeof(ident))) {
> > - printf("\ts->maps[%zu].mmaped = (void **)&obj->%s;\n",
> > - i, ident);
> > + printf("\tmap->mmaped = (void **)&obj->%s; \n", ident);
> ^^
> nit: this generates extra white space
> > }
> >
> > if (populate_links && bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS) {
> > codegen("\
> > \n\
> > - s->maps[%zu].link = &obj->links.%s;\n\
> > - ",
> > - i, ident);
> > + map->link = &obj->links.%s; \n\
> > + ", ident);
> > }
> > i++;
> > }
>
> [...]
