| Message ID | a679538775e08c6f7686c2aec201589b47eda483.1646188795.git.delyank@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Subskeleton support for BPF libraries | expand |
On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@fb.com> 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, only globals are supported, though non-owning references > to links, programs, and other objects may be added as the needs arise. There is a need (I needed it in libusdt, for example). Let's add it from the very beginning, especially that it can be done in *exactly* the same form as for skeletons. > > Signed-off-by: Delyan Kratunov <delyank@fb.com> > --- > tools/bpf/bpftool/gen.c | 322 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 313 insertions(+), 9 deletions(-) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index 145734b4fe41..ea292e09c17b 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -125,14 +125,14 @@ static int codegen_datasec_def(struct bpf_object *obj, > struct btf *btf, > struct btf_dump *d, > const struct btf_type *sec, > - const char *obj_name) > + const char *obj_name, > + bool subskel) seems like subskel's datasec codegen is considerably different (and simpler, really) than skeleton's, I'd add a separate function for it instead of all this if (subskel) special-casing. Main concern for skeleton is generating correct memory layout, while for subskel we just need to generate a list of pointers without any regard to memory layout. > { > const char *sec_name = btf__name_by_offset(btf, sec->name_off); > const struct btf_var_secinfo *sec_var = btf_var_secinfos(sec); > int i, err, off = 0, pad_cnt = 0, vlen = btf_vlen(sec); > char var_ident[256], sec_ident[256]; > bool strip_mods = false; > - why? there should be an empty line between variable block and first statement > if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident))) > return 0; > > @@ -183,7 +183,7 @@ static int codegen_datasec_def(struct bpf_object *obj, > align = 4; > > align_off = (off + align - 1) / align * align; > - if (align_off != need_off) { > + if (align_off != need_off && !subskel) { > printf("\t\tchar __pad%d[%d];\n", > pad_cnt, need_off - off); > pad_cnt++; > @@ -197,6 +197,15 @@ static int codegen_datasec_def(struct bpf_object *obj, > strncat(var_ident, var_name, sizeof(var_ident) - 1); > sanitize_identifier(var_ident); > > + /* to emit a pointer to the type in the map, we need to > + * make sure our btf has that pointer type first. > + */ > + if (subskel) { > + var_type_id = btf__add_ptr(btf, var_type_id); > + if (var_type_id < 0) > + return var_type_id; > + } > + it's unfortunate to have to modify original BTF just to have this '*' added. If I remember correctly, C syntax for pointers has special case only for arrays and func prototypes, so it should work with this logic (not tested, obviously) 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)" 2. otherwise set var_ident to "*<variable>" we'd need to test it for array and func_proto, but it definitely should work for all other cases > printf("\t\t"); > err = btf_dump__emit_type_decl(d, var_type_id, &opts); > if (err) > @@ -205,7 +214,10 @@ static int codegen_datasec_def(struct bpf_object *obj, > > off = sec_var->offset + sec_var->size; > } > - printf(" } *%s;\n", sec_ident); > + if (subskel) > + printf(" } %s;\n", sec_ident); > + else > + printf(" } *%s;\n", sec_ident); > return 0; > } > > @@ -231,7 +243,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map > return NULL; > } > > -static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) > +static int codegen_datasecs(struct bpf_object *obj, const char *obj_name, bool subskel) > { > struct btf *btf = bpf_object__btf(obj); > struct btf_dump *d; > @@ -240,6 +252,13 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) > char map_ident[256]; > int err = 0; > > + /* When generating a subskeleton, we need to emit _pointers_ > + * to the types in the maps. Use a new btf object as storage for these > + * new types as they're not guaranteed to already exist. > + */ > + if (subskel) > + btf = btf__new_empty_split(btf); > + > d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL); > err = libbpf_get_error(d); > if (err) > @@ -264,11 +283,11 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) > * map. It will still be memory-mapped and its contents > * accessible from user-space through BPF skeleton. > */ > - if (!sec) { > + if (!sec && !subskel) { > printf(" struct %s__%s {\n", obj_name, map_ident); > printf(" } *%s;\n", map_ident); > - } else { > - err = codegen_datasec_def(obj, btf, d, sec, obj_name); > + } else if (sec) { > + err = codegen_datasec_def(obj, btf, d, sec, obj_name, subskel); > if (err) > goto out; > } > @@ -276,6 +295,8 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) > > > out: > + if (subskel) > + btf__free(btf); > btf_dump__free(d); > return err; > } > @@ -896,7 +917,7 @@ static int do_skeleton(int argc, char **argv) > > btf = bpf_object__btf(obj); > if (btf) { > - err = codegen_datasecs(obj, obj_name); > + err = codegen_datasecs(obj, obj_name, false); > if (err) > goto out; > } > @@ -1141,6 +1162,287 @@ static int do_skeleton(int argc, char **argv) > return err; > } > > +/* Subskeletons are like skeletons, except they don't own the bpf_object, > + * associated maps, links, etc. Instead, they know about the existence of > + * a certain number of datasec fields and are able to find their locations > + * _at runtime_ from an already loaded bpf_object. > + * > + * This allows for library-like BPF objects to have userspace counterparts > + * with access to their globals without having to know anything about the > + * final BPF object that the library was linked into. > + */ > +static int do_subskeleton(int argc, char **argv) > +{ > + char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")]; use __SUBSKEL_H__ here? > + size_t i, len, file_sz, mmap_sz, sym_sz = 0, sym_idx = 0; > + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts); > + char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data; > + struct bpf_object *obj = NULL; > + const char *file, *var_name; > + char ident[256], var_ident[256]; > + int fd, err = -1, map_type_id; > + const struct bpf_map *map; > + struct btf *btf; > + const struct btf_type *map_type, *var_type; > + const struct btf_var_secinfo *var; > + struct stat st; > + > + if (!REQ_ARGS(1)) { > + usage(); > + return -1; > + } > + file = GET_ARG(); > + > + while (argc) { > + if (!REQ_ARGS(2)) > + return -1; > + > + if (is_prefix(*argv, "name")) { > + NEXT_ARG(); > + > + if (obj_name[0] != '\0') { > + p_err("object name already specified"); > + return -1; > + } > + > + strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1); > + obj_name[MAX_OBJ_NAME_LEN - 1] = '\0'; we should probably force obj_name to be an empty string, so that all map names match their proper section names > + } else { > + p_err("unknown arg %s", *argv); > + return -1; > + } > + > + NEXT_ARG(); > + } > + > + if (argc) { > + p_err("extra unknown arguments"); > + return -1; > + } > + > + if (use_loader) { > + p_err("cannot use loader for subskeletons"); > + return -1; > + } > + > + if (stat(file, &st)) { > + p_err("failed to stat() %s: %s", file, strerror(errno)); > + return -1; > + } > + file_sz = st.st_size; > + mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE)); > + fd = open(file, O_RDONLY); > + if (fd < 0) { > + p_err("failed to open() %s: %s", file, strerror(errno)); > + return -1; > + } > + obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0); > + if (obj_data == MAP_FAILED) { > + obj_data = NULL; > + p_err("failed to mmap() %s: %s", file, strerror(errno)); > + goto out; > + } > + if (obj_name[0] == '\0') > + get_obj_name(obj_name, file); > + opts.object_name = obj_name; > + if (verifier_logs) > + /* log_level1 + log_level2 + stats, but not stable UAPI */ > + opts.kernel_log_level = 1 + 2 + 4; hm.. we shouldn't need this, we are not loading anything into kernel and don't use light skeleton > + obj = bpf_object__open_mem(obj_data, file_sz, &opts); > + err = libbpf_get_error(obj); no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode, so it will get NULL on error and error itself will be in errno > + if (err) { > + char err_buf[256]; > + > + libbpf_strerror(err, err_buf, sizeof(err_buf)); > + p_err("failed to open BPF object file: %s", err_buf); > + obj = NULL; > + goto out; > + } > + > + btf = bpf_object__btf(obj); > + err = libbpf_get_error(btf); same > + if (err) { > + err = -1; > + p_err("need btf type information for %s", obj_name); > + goto out; > + } > + > + /* First, count how many symbols we have to link. */ > + bpf_object__for_each_map(map, obj) { > + if (!bpf_map__is_internal(map)) > + continue; > + > + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) > + continue; > + > + if (!get_map_ident(map, ident, sizeof(ident))) > + continue; > + > + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); if we set obj_name to "", bpf_map__name() should return ELF section name here, so no need to expose this as an API oh, but also bpf_map__btf_value_type_id() should give you this ID directly > + if (map_type_id < 0) { > + err = map_type_id; > + goto out; > + } > + map_type = btf__type_by_id(btf, map_type_id); > + > + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); > + i < len; > + i++, var++) { nit: move those long one-time assignments out of the for() loop and keep it single-line? > + sym_sz++; > + } > + } > + > + get_header_guard(header_guard, obj_name); > + codegen("\ > + \n\ > + /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */ \n\ > + \n\ > + /* THIS FILE IS AUTOGENERATED! */ \n\ > + #ifndef %2$s \n\ > + #define %2$s \n\ > + \n\ > + #include <errno.h> \n\ > + #include <stdlib.h> \n\ > + #include <bpf/libbpf.h> \n\ > + \n\ > + struct %1$s { \n\ > + struct bpf_object *obj; \n\ > + struct bpf_object_subskeleton *subskel; \n\ > + ", obj_name, header_guard); > + > + err = codegen_datasecs(obj, obj_name, true); > + if (err) > + goto out; > + > + /* emit code that will allocate enough storage for all symbols */ > + codegen("\ > + \n\ > + \n\ > + #ifdef __cplusplus \n\ > + static inline struct %1$s *open(const struct bpf_object *src);\n\ > + static inline void %1$s::destroy(struct %1$s *skel);\n\ > + #endif /* __cplusplus */ \n\ > + }; \n\ > + \n\ > + static inline void \n\ > + %1$s__destroy(struct %1$s *skel) \n\ > + { \n\ > + if (!skel) \n\ > + return; \n\ > + if (skel->subskel) \n\ > + bpf_object__destroy_subskeleton(skel->subskel);\n\ > + free(skel); \n\ > + } \n\ > + \n\ > + static inline struct %1$s * \n\ > + %1$s__open(const struct bpf_object *src) \n\ > + { \n\ > + struct %1$s *obj; \n\ > + struct bpf_object_subskeleton *subskel; \n\ > + struct bpf_sym_skeleton *syms; \n\ > + int err; \n\ > + \n\ > + obj = (struct %1$s *)calloc(1, sizeof(*obj)); \n\ > + if (!obj) { \n\ > + errno = ENOMEM; \n\ > + return NULL; \n\ > + } \n\ > + subskel = (struct bpf_object_subskeleton *)calloc(1, sizeof(*subskel));\n\ > + if (!subskel) { \n\ > + errno = ENOMEM; \n\ > + return NULL; \n\ leaking obj here > + } \n\ > + subskel->sz = sizeof(*subskel); \n\ > + subskel->obj = src; \n\ > + subskel->sym_skel_sz = sizeof(struct bpf_sym_skeleton); \n\ > + subskel->sym_cnt = %2$d; \n\ > + obj->subskel = subskel; \n\ > + \n\ > + syms = (struct bpf_sym_skeleton *)calloc(%2$d, sizeof(*syms));\n\ > + if (!syms) { \n\ > + free(subskel); \n\ > + errno = ENOMEM; \n\ > + return NULL; \n\ same, obj is leaked > + } \n\ > + subskel->syms = syms; \n\ > + \n\ > + ", > + obj_name, sym_sz > + ); > + > + /* walk through each symbol and emit the runtime representation > + */ nit: keep this comment single-lined? > + bpf_object__for_each_map(map, obj) { > + if (!bpf_map__is_internal(map)) > + continue; > + > + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) > + continue; > + > + if (!get_map_ident(map, ident, sizeof(ident))) > + continue; this sequence of ifs seems so frequently repeated that it's probably time to add a helper function? > + > + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); > + if (map_type_id < 0) { > + err = map_type_id; > + goto out; > + } > + map_type = btf__type_by_id(btf, map_type_id); > + > + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); > + i < len; > + i++, var++, sym_idx++) { similar as above, var and len assignment doesn't have to inside the for > + var_type = btf__type_by_id(btf, var->type); > + var_name = btf__name_by_offset(btf, var_type->name_off); > + > + var_ident[0] = '\0'; > + strncat(var_ident, var_name, sizeof(var_ident) - 1); > + sanitize_identifier(var_ident); > + > + codegen("\ > + \n\ > + syms[%4$d].name = \"%1$s\"; \n\ > + syms[%4$d].section = \"%3$s\"; \n\ > + syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\ > + ", var_ident, ident, bpf_map__section_name(map), sym_idx); > + } > + } why not assign subskel->sym_cnt here using sym_idx and avoid that extra loop over all variables above? > + > + codegen("\ > + \n\ > + \n\ > + err = bpf_object__open_subskeleton(subskel); \n\ > + if (err) { \n\ > + %1$s__destroy(obj); \n\ > + errno = err; \n\ > + return NULL; \n\ > + } \n\ > + \n\ > + return obj; \n\ > + } \n\ > + ", > + obj_name); > + > + codegen("\ > + \n\ > + \n\ > + #ifdef __cplusplus \n\ > + struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\ > + void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\ > + #endif /* __cplusplus */ \n\ > + \n\ > + #endif /* %2$s */ \n\ > + ", > + obj_name, header_guard); > + err = 0; > +out: > + bpf_object__close(obj); > + if (obj_data) > + munmap(obj_data, mmap_sz); > + close(fd); > + return err; > +} > + > static int do_object(int argc, char **argv) > { > struct bpf_linker *linker; > @@ -1192,6 +1494,7 @@ static int do_help(int argc, char **argv) > fprintf(stderr, > "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n" > " %1$s %2$s skeleton FILE [name OBJECT_NAME]\n" > + " %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n" Quentin will remind you that you should also update the man page and bash completion script :) > " %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n" > " %1$s %2$s help\n" > "\n" > @@ -1788,6 +2091,7 @@ static int do_min_core_btf(int argc, char **argv) > static const struct cmd cmds[] = { > { "object", do_object }, > { "skeleton", do_skeleton }, > + { "subskeleton", do_subskeleton }, > { "min_core_btf", do_min_core_btf}, > { "help", do_help }, > { 0 } > -- > 2.34.1
Thanks Andrii for taking a thorough look! On Wed, 2022-03-02 at 17:46 -0800, Andrii Nakryiko wrote: > There is a need (I needed it in libusdt, for example). Let's add it > from the very beginning, especially that it can be done in *exactly* > the same form as for skeletons. Absolutely, I'll get that into the next version! > seems like subskel's datasec codegen is considerably different (and > simpler, really) than skeleton's, I'd add a separate function for it > instead of all this if (subskel) special-casing. Main concern for > skeleton is generating correct memory layout, while for subskel we > just need to generate a list of pointers without any regard to memory > layout. I'm not 100% convinced given how much code would end up being duplicated but I can go in that direction, if you'd prefer it. > > it's unfortunate to have to modify original BTF just to have this '*' > added. If I remember correctly, C syntax for pointers has special > case only for arrays and func prototypes, so it should work with this > logic (not tested, obviously) > > 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)" > 2. otherwise set var_ident to "*<variable>" > > we'd need to test it for array and func_proto, but it definitely > should work for all other cases A couple of thoughts here: 1. We are not modifing the original BTF, we are layering in-memory-only types on top of it. This ends up working transparently through btf_dump code, which is the source of truth of what "correct" is. I think this is strictly better than the alternative textual modifications to var_ident. 2. I guess we see the change differently - to me it's not just about adding an asterisk but instead working with derivative types. This might come in handy in other contexts that we haven't envisioned yet and I feel is a direction worth supporting overall. 3. We have a full type system with layering and mixed file- and memory-based storage. Why limit ourselves to templating instead of using it in the codegen? If I were writing this from scratch, much of codegen_datasecs would instead create in-memory BTF types and have btf_dump emit them (but that's not the bikeshed I want to paint here!). > > + char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")]; > > use __SUBSKEL_H__ here? Sure, I can introduce the .subskel.h suffix everywhere. > > > + strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1); > > + obj_name[MAX_OBJ_NAME_LEN - 1] = '\0'; > > we should probably force obj_name to be an empty string, so that all > map names match their proper section names Ah, maybe this is why bpf_map__name was not doing the right thing before. I don't really like that we're relying on side effects of the empty obj_name but I'll try it and see if anything breaks (the templating for one will need it anyway). > > > + if (verifier_logs) > > + /* log_level1 + log_level2 + stats, but not stable UAPI */ > > + opts.kernel_log_level = 1 + 2 + 4; > > hm.. we shouldn't need this, we are not loading anything into kernel > and don't use light skeleton You're right, will remove. > > > + obj = bpf_object__open_mem(obj_data, file_sz, &opts); > > + err = libbpf_get_error(obj); > > no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode, > so it will get NULL on error and error itself will be in errno Ah, yes, I won't add new callsites. > > > > > + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); > > if we set obj_name to "", bpf_map__name() should return ELF section > name here, so no need to expose this as an API > > > oh, but also bpf_map__btf_value_type_id() should give you this ID directly TIL, that's not obvious at all. There's a few places in gen.c that could be simplified then - find_type_for_map goes through slicing the complete name and walking over every BTF type to match on the slice. Is there some compatibility reason to do that or is btf_value_type_id always there? > > > + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); > > + i < len; > > + i++, var++) { > > nit: move those long one-time assignments out of the for() loop and > keep it single-line? Yeah, I hate that structure too, I'll clean it up. > > > > > + if (!subskel) { \n\ > > + errno = ENOMEM; \n\ > > + return NULL; \n\ > > leaking obj here Yeah, I noticed that I didn't use __destroy in the subskel, will fix for v2. > > + /* walk through each symbol and emit the runtime representation > > + */ > > nit: keep this comment single-lined? I did initially and checkpatch scolded me :) > > > + bpf_object__for_each_map(map, obj) { > > + if (!bpf_map__is_internal(map)) > > + continue; > > + > > + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) > > + continue; > > + > > + if (!get_map_ident(map, ident, sizeof(ident))) > > + continue; > > this sequence of ifs seems so frequently repeated that it's probably > time to add a helper function? It is and it's annoying me too. I'll look at the whole iteration pattern more closely. > > > > + codegen("\ > > + \n\ > > + syms[%4$d].name = \"%1$s\"; \n\ > > + syms[%4$d].section = \"%3$s\"; \n\ > > + syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\ > > + ", var_ident, ident, bpf_map__section_name(map), sym_idx); > > + } > > + } > > why not assign subskel->sym_cnt here using sym_idx and avoid that > extra loop over all variables above? Good call, will do. > > Quentin will remind you that you should also update the man page and > bash completion script :) Ah, yes, glad to see it's rst and I don't have to suffer groff/mdoc flashbacks! Thanks, Delyan
On Thu, 2022-03-03 at 10:52 -0800, Delyan Kratunov wrote: > > > > > > > > + map_type_id = btf__find_by_name_kind(btf, > > > bpf_map__section_name(map), BTF_KIND_DATASEC); > > > > if we set obj_name to "", bpf_map__name() should return ELF section > > name here, so no need to expose this as an API > > > > > > oh, but also bpf_map__btf_value_type_id() should give you this ID directly > > TIL, that's not obvious at all. There's a few places in gen.c that could be > simplified then - find_type_for_map goes through slicing the complete name and > walking over every BTF type to match on the slice. Is there some compatibility > reason to do that or is btf_value_type_id always there? Unfortunately, the internal datasec maps have value_type_id = key_value_type_id = 0 i.e. void, so bpf_map__btf_value_type_id won't work out of the box. I haven't looked if that's a bug all the way in clang-emitted object or somewhere further on. -- Delyan
On Thu, Mar 3, 2022 at 10:57 AM Delyan Kratunov <delyank@fb.com> wrote: > > Thanks Andrii for taking a thorough look! > > On Wed, 2022-03-02 at 17:46 -0800, Andrii Nakryiko wrote: > > There is a need (I needed it in libusdt, for example). Let's add it > > from the very beginning, especially that it can be done in *exactly* > > the same form as for skeletons. > > Absolutely, I'll get that into the next version! > > > seems like subskel's datasec codegen is considerably different (and > > simpler, really) than skeleton's, I'd add a separate function for it > > instead of all this if (subskel) special-casing. Main concern for > > skeleton is generating correct memory layout, while for subskel we > > just need to generate a list of pointers without any regard to memory > > layout. > > I'm not 100% convinced given how much code would end up being duplicated but I > can go in that direction, if you'd prefer it. > > > > > > it's unfortunate to have to modify original BTF just to have this '*' > > added. If I remember correctly, C syntax for pointers has special > > case only for arrays and func prototypes, so it should work with this > > logic (not tested, obviously) > > > > 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)" > > 2. otherwise set var_ident to "*<variable>" > > > > we'd need to test it for array and func_proto, but it definitely > > should work for all other cases > > A couple of thoughts here: > > 1. We are not modifing the original BTF, we are layering in-memory-only types on > top of it. This ends up working transparently through btf_dump code, which is > the source of truth of what "correct" is. I think this is strictly better than > the alternative textual modifications to var_ident. Yeah, I noticed that you are creating split BTF later. Ok, I don't mind, let's do it this way (due to the horrible pointer-to-array syntax inconsistency). Please leave the comment somewhere here to make it obvious that we are not modifying original BTF > > 2. I guess we see the change differently - to me it's not just about adding an > asterisk but instead working with derivative types. This might come in handy in > other contexts that we haven't envisioned yet and I feel is a direction worth > supporting overall. It's not "just about adding an asterisk", it's about generating a pointer to the type. Split BTF added on top makes it a bit more tolerable (though there is still a bunch of unnecessary complexity and overhead just for that pesky asterisk). Another alternative would be: typeof(char[123]) *my_ptr; This can be done without generating extra BTF. For complex types it's actually even easier to parse, tbh. I initially didn't like it, but now I'm thinking maybe for arrays and func_protos we should do just this? WDYT? > > 3. We have a full type system with layering and mixed file- and memory-based > storage. Why limit ourselves to templating instead of using it in the codegen? > If I were writing this from scratch, much of codegen_datasecs would instead > create in-memory BTF types and have btf_dump emit them (but that's not the > bikeshed I want to paint here!). Maybe, but at the time this code was written we didn't have either split BTF nor BTF writing APIs. > > > > + char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")]; > > > > use __SUBSKEL_H__ here? > > Sure, I can introduce the .subskel.h suffix everywhere. > > > > > > + strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1); > > > + obj_name[MAX_OBJ_NAME_LEN - 1] = '\0'; > > > > we should probably force obj_name to be an empty string, so that all > > map names match their proper section names > > Ah, maybe this is why bpf_map__name was not doing the right thing before. I > don't really like that we're relying on side effects of the empty obj_name but > I'll try it and see if anything breaks (the templating for one will need it > anyway). Yeah, it's due to the fact that we are using "object name" as part of .data, .rodata, and .bss maps (and .kconfig, probably). Historic decision, too bad, but we have what we have. We probably should clean this up in libbpf 1.0, it's too confusing throughout. > > > > > > + if (verifier_logs) > > > + /* log_level1 + log_level2 + stats, but not stable UAPI */ > > > + opts.kernel_log_level = 1 + 2 + 4; > > > > hm.. we shouldn't need this, we are not loading anything into kernel > > and don't use light skeleton > > You're right, will remove. > > > > > > + obj = bpf_object__open_mem(obj_data, file_sz, &opts); > > > + err = libbpf_get_error(obj); > > > > no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode, > > so it will get NULL on error and error itself will be in errno > > Ah, yes, I won't add new callsites. > > > > > > > > > + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); > > > > if we set obj_name to "", bpf_map__name() should return ELF section > > name here, so no need to expose this as an API > > > > > > oh, but also bpf_map__btf_value_type_id() should give you this ID directly > > TIL, that's not obvious at all. There's a few places in gen.c that could be > simplified then - find_type_for_map goes through slicing the complete name and > walking over every BTF type to match on the slice. Is there some compatibility > reason to do that or is btf_value_type_id always there? No legacy reason, we should use btf_value_type_id if necessary/possible (it's going to be DATASEC type for all global var maps, I think). But I just double checked and it seems that we fill out btf_value_type_id only during map creation (that is, during bpf_object__load()). But I don't see any reason to postpone it so late, so let's just move it to the open phase. See bpf_map_find_btf_info() and where it's called. > > > > > > + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); > > > + i < len; > > > + i++, var++) { > > > > nit: move those long one-time assignments out of the for() loop and > > keep it single-line? > > Yeah, I hate that structure too, I'll clean it up. > > > > > > > > > + if (!subskel) { \n\ > > > + errno = ENOMEM; \n\ > > > + return NULL; \n\ > > > > leaking obj here > > Yeah, I noticed that I didn't use __destroy in the subskel, will fix for v2. > > > > > + /* walk through each symbol and emit the runtime representation > > > + */ > > > > nit: keep this comment single-lined? > > I did initially and checkpatch scolded me :) checkpatch still loves 80 character lines, but it was relaxed to 100 a while ago. Checkpatch.pl is not an authority, it's just a suggestion for a lot of cases. > > > > > > + bpf_object__for_each_map(map, obj) { > > > + if (!bpf_map__is_internal(map)) > > > + continue; > > > + > > > + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) > > > + continue; > > > + > > > + if (!get_map_ident(map, ident, sizeof(ident))) > > > + continue; > > > > this sequence of ifs seems so frequently repeated that it's probably > > time to add a helper function? > > It is and it's annoying me too. I'll look at the whole iteration pattern more > closely. > > > > > > > > + codegen("\ > > > + \n\ > > > + syms[%4$d].name = \"%1$s\"; \n\ > > > + syms[%4$d].section = \"%3$s\"; \n\ > > > + syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\ > > > + ", var_ident, ident, bpf_map__section_name(map), sym_idx); > > > + } > > > + } > > > > why not assign subskel->sym_cnt here using sym_idx and avoid that > > extra loop over all variables above? > > Good call, will do. > > > > Quentin will remind you that you should also update the man page and > > bash completion script :) > > Ah, yes, glad to see it's rst and I don't have to suffer groff/mdoc flashbacks! > > Thanks, > Delyan >
On Thu, Mar 3, 2022 at 12:54 PM Delyan Kratunov <delyank@fb.com> wrote: > > On Thu, 2022-03-03 at 10:52 -0800, Delyan Kratunov wrote: > > > > > > > > > > > + map_type_id = btf__find_by_name_kind(btf, > > > > bpf_map__section_name(map), BTF_KIND_DATASEC); > > > > > > if we set obj_name to "", bpf_map__name() should return ELF section > > > name here, so no need to expose this as an API > > > > > > > > > oh, but also bpf_map__btf_value_type_id() should give you this ID directly > > > > TIL, that's not obvious at all. There's a few places in gen.c that could be > > simplified then - find_type_for_map goes through slicing the complete name and > > walking over every BTF type to match on the slice. Is there some compatibility > > reason to do that or is btf_value_type_id always there? > > Unfortunately, the internal datasec maps have value_type_id = key_value_type_id > = 0 i.e. void, so bpf_map__btf_value_type_id won't work out of the box. > > I haven't looked if that's a bug all the way in clang-emitted object or > somewhere further on. Yeah, just replied in another email. We fill it out too late, but it should be possible to move it earlier into open phase. > > -- Delyan
Sorry, missed this question originally. On Fri, 2022-03-04 at 11:29 -0800, Andrii Nakryiko wrote: > Split BTF added on top makes it a bit more > tolerable (though there is still a bunch of unnecessary complexity and > overhead just for that pesky asterisk). > > Another alternative would be: > > typeof(char[123]) *my_ptr; > > This can be done without generating extra BTF. For complex types it's > actually even easier to parse, tbh. I initially didn't like it, but > now I'm thinking maybe for arrays and func_protos we should do just > this? WDYT? typeof is _technically_ not standard C (I think it'll make it into C23). GCC and Clang do both support it but I would guess we'd want the generated userspace code to be compatible with as many toolchains and configurations as possible? (e.g. people using c99 instead of gnu99) Beyond that, I feel that any complexity saved by the typeof is lost in special- casing arrays and function prototypes when the current code is uniform across all types. If you insist, I can go down this path but I'm not super enthusiastic about it (and it's harder to read on top of everything). -- Delyan
On Wed, Mar 9, 2022 at 4:10 PM Delyan Kratunov <delyank@fb.com> wrote: > > Sorry, missed this question originally. > > On Fri, 2022-03-04 at 11:29 -0800, Andrii Nakryiko wrote: > > Split BTF added on top makes it a bit more > > tolerable (though there is still a bunch of unnecessary complexity and > > overhead just for that pesky asterisk). > > > > Another alternative would be: > > > > typeof(char[123]) *my_ptr; > > > > This can be done without generating extra BTF. For complex types it's > > actually even easier to parse, tbh. I initially didn't like it, but > > now I'm thinking maybe for arrays and func_protos we should do just > > this? WDYT? > > typeof is _technically_ not standard C (I think it'll make it into C23). GCC and > Clang do both support it but I would guess we'd want the generated userspace > code to be compatible with as many toolchains and configurations as possible? > (e.g. people using c99 instead of gnu99) that ship has sailed, I'm afraid. btf.h and xsk.h (I'm not even talking about BPF-side headers) both use typeof() > > Beyond that, I feel that any complexity saved by the typeof is lost in special- > casing arrays and function prototypes when the current code is uniform across > all types. > > If you insist, I can go down this path but I'm not super enthusiastic about it > (and it's harder to read on top of everything). I can tell you are not :) I do think that typeof() makes it much easier to follow complicated cases. Just look at the example below: static int blah_fn(int x, int y) { return x + y; } struct S { typeof(char[24]) *my_ptr1; char (*my_ptr2)[24]; int (**my_func1)(int, int); typeof(int (*)(int, int)) *my_func2; }; int main() { static struct S s; char blah[24]; int (*fptr)(int, int); s.my_ptr1 = &blah; s.my_ptr2 = &blah; s.my_func1 = &fptr; s.my_func2 = &fptr; return 0; } Which one is easier to follow, my_ptr1 or my_ptr2? func pointer is a bit more hypothetical (hard to come up with realistic BPF program that would need func pointer type as global variable type), but I still did it for completeness. So it's not like I'm dead set against this split BTF approach, I just do really think that array variable case is better with typeof(). > > -- Delyan
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index 145734b4fe41..ea292e09c17b 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -125,14 +125,14 @@ static int codegen_datasec_def(struct bpf_object *obj, struct btf *btf, struct btf_dump *d, const struct btf_type *sec, - const char *obj_name) + const char *obj_name, + bool subskel) { const char *sec_name = btf__name_by_offset(btf, sec->name_off); const struct btf_var_secinfo *sec_var = btf_var_secinfos(sec); int i, err, off = 0, pad_cnt = 0, vlen = btf_vlen(sec); char var_ident[256], sec_ident[256]; bool strip_mods = false; - if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident))) return 0; @@ -183,7 +183,7 @@ static int codegen_datasec_def(struct bpf_object *obj, align = 4; align_off = (off + align - 1) / align * align; - if (align_off != need_off) { + if (align_off != need_off && !subskel) { printf("\t\tchar __pad%d[%d];\n", pad_cnt, need_off - off); pad_cnt++; @@ -197,6 +197,15 @@ static int codegen_datasec_def(struct bpf_object *obj, strncat(var_ident, var_name, sizeof(var_ident) - 1); sanitize_identifier(var_ident); + /* to emit a pointer to the type in the map, we need to + * make sure our btf has that pointer type first. + */ + if (subskel) { + var_type_id = btf__add_ptr(btf, var_type_id); + if (var_type_id < 0) + return var_type_id; + } + printf("\t\t"); err = btf_dump__emit_type_decl(d, var_type_id, &opts); if (err) @@ -205,7 +214,10 @@ static int codegen_datasec_def(struct bpf_object *obj, off = sec_var->offset + sec_var->size; } - printf(" } *%s;\n", sec_ident); + if (subskel) + printf(" } %s;\n", sec_ident); + else + printf(" } *%s;\n", sec_ident); return 0; } @@ -231,7 +243,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map return NULL; } -static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) +static int codegen_datasecs(struct bpf_object *obj, const char *obj_name, bool subskel) { struct btf *btf = bpf_object__btf(obj); struct btf_dump *d; @@ -240,6 +252,13 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) char map_ident[256]; int err = 0; + /* When generating a subskeleton, we need to emit _pointers_ + * to the types in the maps. Use a new btf object as storage for these + * new types as they're not guaranteed to already exist. + */ + if (subskel) + btf = btf__new_empty_split(btf); + d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL); err = libbpf_get_error(d); if (err) @@ -264,11 +283,11 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) * map. It will still be memory-mapped and its contents * accessible from user-space through BPF skeleton. */ - if (!sec) { + if (!sec && !subskel) { printf(" struct %s__%s {\n", obj_name, map_ident); printf(" } *%s;\n", map_ident); - } else { - err = codegen_datasec_def(obj, btf, d, sec, obj_name); + } else if (sec) { + err = codegen_datasec_def(obj, btf, d, sec, obj_name, subskel); if (err) goto out; } @@ -276,6 +295,8 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name) out: + if (subskel) + btf__free(btf); btf_dump__free(d); return err; } @@ -896,7 +917,7 @@ static int do_skeleton(int argc, char **argv) btf = bpf_object__btf(obj); if (btf) { - err = codegen_datasecs(obj, obj_name); + err = codegen_datasecs(obj, obj_name, false); if (err) goto out; } @@ -1141,6 +1162,287 @@ static int do_skeleton(int argc, char **argv) return err; } +/* Subskeletons are like skeletons, except they don't own the bpf_object, + * associated maps, links, etc. Instead, they know about the existence of + * a certain number of datasec fields and are able to find their locations + * _at runtime_ from an already loaded bpf_object. + * + * This allows for library-like BPF objects to have userspace counterparts + * with access to their globals without having to know anything about the + * final BPF object that the library was linked into. + */ +static int do_subskeleton(int argc, char **argv) +{ + char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")]; + size_t i, len, file_sz, mmap_sz, sym_sz = 0, sym_idx = 0; + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts); + char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data; + struct bpf_object *obj = NULL; + const char *file, *var_name; + char ident[256], var_ident[256]; + int fd, err = -1, map_type_id; + const struct bpf_map *map; + struct btf *btf; + const struct btf_type *map_type, *var_type; + const struct btf_var_secinfo *var; + struct stat st; + + if (!REQ_ARGS(1)) { + usage(); + return -1; + } + file = GET_ARG(); + + while (argc) { + if (!REQ_ARGS(2)) + return -1; + + if (is_prefix(*argv, "name")) { + NEXT_ARG(); + + if (obj_name[0] != '\0') { + p_err("object name already specified"); + return -1; + } + + strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1); + obj_name[MAX_OBJ_NAME_LEN - 1] = '\0'; + } else { + p_err("unknown arg %s", *argv); + return -1; + } + + NEXT_ARG(); + } + + if (argc) { + p_err("extra unknown arguments"); + return -1; + } + + if (use_loader) { + p_err("cannot use loader for subskeletons"); + return -1; + } + + if (stat(file, &st)) { + p_err("failed to stat() %s: %s", file, strerror(errno)); + return -1; + } + file_sz = st.st_size; + mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE)); + fd = open(file, O_RDONLY); + if (fd < 0) { + p_err("failed to open() %s: %s", file, strerror(errno)); + return -1; + } + obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0); + if (obj_data == MAP_FAILED) { + obj_data = NULL; + p_err("failed to mmap() %s: %s", file, strerror(errno)); + goto out; + } + if (obj_name[0] == '\0') + get_obj_name(obj_name, file); + opts.object_name = obj_name; + if (verifier_logs) + /* log_level1 + log_level2 + stats, but not stable UAPI */ + opts.kernel_log_level = 1 + 2 + 4; + obj = bpf_object__open_mem(obj_data, file_sz, &opts); + err = libbpf_get_error(obj); + if (err) { + char err_buf[256]; + + libbpf_strerror(err, err_buf, sizeof(err_buf)); + p_err("failed to open BPF object file: %s", err_buf); + obj = NULL; + goto out; + } + + btf = bpf_object__btf(obj); + err = libbpf_get_error(btf); + if (err) { + err = -1; + p_err("need btf type information for %s", obj_name); + goto out; + } + + /* First, count how many symbols we have to link. */ + bpf_object__for_each_map(map, obj) { + if (!bpf_map__is_internal(map)) + continue; + + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) + continue; + + if (!get_map_ident(map, ident, sizeof(ident))) + continue; + + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); + if (map_type_id < 0) { + err = map_type_id; + goto out; + } + map_type = btf__type_by_id(btf, map_type_id); + + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); + i < len; + i++, var++) { + sym_sz++; + } + } + + get_header_guard(header_guard, obj_name); + codegen("\ + \n\ + /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */ \n\ + \n\ + /* THIS FILE IS AUTOGENERATED! */ \n\ + #ifndef %2$s \n\ + #define %2$s \n\ + \n\ + #include <errno.h> \n\ + #include <stdlib.h> \n\ + #include <bpf/libbpf.h> \n\ + \n\ + struct %1$s { \n\ + struct bpf_object *obj; \n\ + struct bpf_object_subskeleton *subskel; \n\ + ", obj_name, header_guard); + + err = codegen_datasecs(obj, obj_name, true); + if (err) + goto out; + + /* emit code that will allocate enough storage for all symbols */ + codegen("\ + \n\ + \n\ + #ifdef __cplusplus \n\ + static inline struct %1$s *open(const struct bpf_object *src);\n\ + static inline void %1$s::destroy(struct %1$s *skel);\n\ + #endif /* __cplusplus */ \n\ + }; \n\ + \n\ + static inline void \n\ + %1$s__destroy(struct %1$s *skel) \n\ + { \n\ + if (!skel) \n\ + return; \n\ + if (skel->subskel) \n\ + bpf_object__destroy_subskeleton(skel->subskel);\n\ + free(skel); \n\ + } \n\ + \n\ + static inline struct %1$s * \n\ + %1$s__open(const struct bpf_object *src) \n\ + { \n\ + struct %1$s *obj; \n\ + struct bpf_object_subskeleton *subskel; \n\ + struct bpf_sym_skeleton *syms; \n\ + int err; \n\ + \n\ + obj = (struct %1$s *)calloc(1, sizeof(*obj)); \n\ + if (!obj) { \n\ + errno = ENOMEM; \n\ + return NULL; \n\ + } \n\ + subskel = (struct bpf_object_subskeleton *)calloc(1, sizeof(*subskel));\n\ + if (!subskel) { \n\ + errno = ENOMEM; \n\ + return NULL; \n\ + } \n\ + subskel->sz = sizeof(*subskel); \n\ + subskel->obj = src; \n\ + subskel->sym_skel_sz = sizeof(struct bpf_sym_skeleton); \n\ + subskel->sym_cnt = %2$d; \n\ + obj->subskel = subskel; \n\ + \n\ + syms = (struct bpf_sym_skeleton *)calloc(%2$d, sizeof(*syms));\n\ + if (!syms) { \n\ + free(subskel); \n\ + errno = ENOMEM; \n\ + return NULL; \n\ + } \n\ + subskel->syms = syms; \n\ + \n\ + ", + obj_name, sym_sz + ); + + /* walk through each symbol and emit the runtime representation + */ + bpf_object__for_each_map(map, obj) { + if (!bpf_map__is_internal(map)) + continue; + + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) + continue; + + if (!get_map_ident(map, ident, sizeof(ident))) + continue; + + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC); + if (map_type_id < 0) { + err = map_type_id; + goto out; + } + map_type = btf__type_by_id(btf, map_type_id); + + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type); + i < len; + i++, var++, sym_idx++) { + var_type = btf__type_by_id(btf, var->type); + var_name = btf__name_by_offset(btf, var_type->name_off); + + var_ident[0] = '\0'; + strncat(var_ident, var_name, sizeof(var_ident) - 1); + sanitize_identifier(var_ident); + + codegen("\ + \n\ + syms[%4$d].name = \"%1$s\"; \n\ + syms[%4$d].section = \"%3$s\"; \n\ + syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\ + ", var_ident, ident, bpf_map__section_name(map), sym_idx); + } + } + + codegen("\ + \n\ + \n\ + err = bpf_object__open_subskeleton(subskel); \n\ + if (err) { \n\ + %1$s__destroy(obj); \n\ + errno = err; \n\ + return NULL; \n\ + } \n\ + \n\ + return obj; \n\ + } \n\ + ", + obj_name); + + codegen("\ + \n\ + \n\ + #ifdef __cplusplus \n\ + struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\ + void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\ + #endif /* __cplusplus */ \n\ + \n\ + #endif /* %2$s */ \n\ + ", + obj_name, header_guard); + err = 0; +out: + bpf_object__close(obj); + if (obj_data) + munmap(obj_data, mmap_sz); + close(fd); + return err; +} + static int do_object(int argc, char **argv) { struct bpf_linker *linker; @@ -1192,6 +1494,7 @@ static int do_help(int argc, char **argv) fprintf(stderr, "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n" " %1$s %2$s skeleton FILE [name OBJECT_NAME]\n" + " %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n" " %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n" " %1$s %2$s help\n" "\n" @@ -1788,6 +2091,7 @@ static int do_min_core_btf(int argc, char **argv) static const struct cmd cmds[] = { { "object", do_object }, { "skeleton", do_skeleton }, + { "subskeleton", do_subskeleton }, { "min_core_btf", do_min_core_btf}, { "help", do_help }, { 0 }
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, only globals are supported, though non-owning references to links, programs, and other objects may be added as the needs arise. Signed-off-by: Delyan Kratunov <delyank@fb.com> --- tools/bpf/bpftool/gen.c | 322 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 313 insertions(+), 9 deletions(-)