Re: [PATCH bpf-next 1/2] bpftool: improve skeleton backwards compat with old buggy libbpfs

[Eduard Zingerman <eddyz87@gmail.com>]

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@meta.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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?

[...]

> @@ -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++;
>  	}

[...]