On Tue, Mar 15, 2022 at 3:15 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> Subskeletons are headers which require an already loaded program to
> operate.
>
> For example, when a BPF library is linked into a larger BPF object file,
> the library userspace needs a way to access its own global variables
> without requiring knowledge about the larger program at build time.
>
> As a result, subskeletons require a loaded bpf_object to open().
> Further, they find their own symbols in the larger program by
> walking BTF type data at run time.
>
> At this time, programs, maps, and globals are supported through
> non-owning pointers.
>
> Signed-off-by: Delyan Kratunov <delyank@xxxxxx>
> ---
> .../bpf/bpftool/Documentation/bpftool-gen.rst | 25 +
> tools/bpf/bpftool/bash-completion/bpftool | 14 +-
> tools/bpf/bpftool/gen.c | 595 +++++++++++++++---
> 3 files changed, 549 insertions(+), 85 deletions(-)
>
[...]
> -static void get_header_guard(char *guard, const char *obj_name)
> +static void get_header_guard(char *guard, const char *obj_name, const char *suffix)
> {
> int i;
>
> - sprintf(guard, "__%s_SKEL_H__", obj_name);
> + sprintf(guard, "__%s_%s__", obj_name, suffix);
> for (i = 0; guard[i]; i++)
> guard[i] = toupper(guard[i]);
> }
> @@ -231,6 +231,17 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map
> return NULL;
> }
>
> +static bool bpf_map__is_internal_mmappable(const struct bpf_map *map, char *buf, size_t sz)
nit: abc__def looks like public libbpf API which is a bit confusing,
maybe just "is_internal_mmapable_map"?
> +{
> + if (!bpf_map__is_internal(map) || !(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> + return false;
> +
> + if (!get_map_ident(map, buf, sz))
> + return false;
> +
> + return true;
> +}
> +
[...]
> + /* The datasec member has KIND_VAR but we want the
> + * underlying type of the variable (e.g. KIND_INT).
> + */
> + var = skip_mods_and_typedefs(btf, var->type, NULL);
> +
> + printf("\t\t");
> + /* Func and array members require special handling.
> + * Instead of producing `typename *var`, they produce
> + * `typeof(typename) *var`. This allows us to keep a
> + * similar syntax where the identifier is just prefixed
> + * by *, allowing us to ignore C declaration minutae.
typo: minutiae
> + */
> + needs_typeof = btf_is_array(var) || btf_is_ptr_to_func_proto(btf, var);
> + if (needs_typeof)
> + printf("typeof(");
> +
[...]
> + bpf_object__for_each_map(map, obj) {
> + if (!get_map_ident(map, ident, sizeof(ident)))
> + continue;
> +
> + codegen("\
> + \n\
> + \n\
> + s->maps[%zu].name = \"%s\"; \n\
> + s->maps[%zu].map = &obj->maps.%s; \n\
> + ",
> + i, bpf_map__name(map), i, ident);
> + /* memory-mapped internal maps */
> + if (mmaped && bpf_map__is_internal(map) &&
> + (bpf_map__map_flags(map) & BPF_F_MMAPABLE)) {
minor: probably could have used is_internal_mmapable_map() here as
well, but you probably didn't like overwriting ident here, right?
> + printf("\ts->maps[%zu].mmaped = (void **)&obj->%s;\n",
> + i, ident);
> + }
> + i++;
> + }
> +}
> +
> +static void
> +codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_links)
> +{
> + struct bpf_program *prog;
> + int i;
> +
> + if (!prog_cnt)
> + return;
> +
> + codegen("\
> + \n\
> + \n\
> + /* programs */ \n\
> + s->prog_cnt = %zu; \n\
> + s->prog_skel_sz = sizeof(*s->progs); \n\
> + s->progs = (struct bpf_prog_skeleton *)calloc(s->prog_cnt, s->prog_skel_sz);\n\
> + if (!s->progs) \n\
> + goto err; \n\
> + ",
> + prog_cnt
> + );
> + i = 0;
> + bpf_object__for_each_program(prog, obj) {
> + codegen("\
> + \n\
> + \n\
> + s->progs[%1$zu].name = \"%2$s\"; \n\
> + s->progs[%1$zu].prog = &obj->progs.%2$s;\n\
nit: here and in few other places \n\ are not aligned properly
> + ",
> + i, bpf_program__name(prog));
> +
> + if (populate_links) {
> + codegen("\
> + \n\
> + s->progs[%1$zu].link = &obj->links.%2$s;\n\
> + ",
> + i, bpf_program__name(prog));
> + }
> + i++;
> + }
> +}
> +
[...]
> + /* First, count how many variables we have to find.
> + * We need this in advance so the subskel can allocate the right
> + * amount of storage.
> + */
> + bpf_object__for_each_map(map, obj) {
> + if (!get_map_ident(map, ident, sizeof(ident)))
> + continue;
> +
> + /* Also count all maps that have a name */
> + map_cnt++;
> +
> + if (!bpf_map__is_internal(map))
> + continue;
> + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> + continue;
same, probably could reuse is_internal_mmapable_map() (but I'm
guessing reassigning ident threw you off)
> +
> + map_type_id = bpf_map__btf_value_type_id(map);
> + if (map_type_id < 0) {
well, actually map_type_id == 0 would be wrong here as well, probably
want to skip such maps (e.g., .rodata.str1.1), instead of pretending
that VOID is DATASEC.
> + err = map_type_id;
> + goto out;
> + }
> + map_type = btf__type_by_id(btf, map_type_id);
> +
[...]
> +
> + /* walk through each symbol and emit the runtime representation */
> + bpf_object__for_each_map(map, obj) {
> + if (!bpf_map__is_internal_mmappable(map, ident, sizeof(ident)))
> + continue;
> +
> + map_type_id = bpf_map__btf_value_type_id(map);
> + if (map_type_id < 0)
<= 0 ?
> + /* skip over internal maps with no type*/
> + continue;
> +
> + map_type = btf__type_by_id(btf, map_type_id);
> + var = btf_var_secinfos(map_type);
> + len = btf_vlen(map_type);
> + for (i = 0; i < len; i++, var++) {
> + var_type = btf__type_by_id(btf, var->type);
> + var_name = btf__name_by_offset(btf, var_type->name_off);
> +
> + if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
> + continue;
> +
> + var_ident[0] = '\0';
> + strncat(var_ident, var_name, sizeof(var_ident) - 1);
> + sanitize_identifier(var_ident);
hm... I thought we agreed that we don't need variable name sanitization?..
> +
> + /* Note that we use the dot prefix in .data as the
> + * field access operator i.e. maps%s becomes maps.data
> + */
> + codegen("\
> + \n\
please emit empty line here (in codegen), similar to map and prog
initialization, for consistency
> + s->vars[%4$d].name = \"%1$s\"; \n\
... and if sanitization had any effect, this name would be wrong (it
has to be original var_name)
> + s->vars[%4$d].map = &obj->maps.%3$s; \n\
> + s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
> + ", var_ident, ident, ident, var_idx);
with this %3$s syntax you don't need to specify ident, ident twice
and (void**) -> (void **)?
> +
> + var_idx++;
> + }
> + }
> +
> + codegen_maps_skeleton(obj, map_cnt, false /*mmaped*/);
> + codegen_progs_skeleton(obj, prog_cnt, false /*links*/);
> +
[...]
