| Message ID | 20210414200146.2663044-13-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Apr 14, 2021 at 04:48:25PM -0700, Andrii Nakryiko wrote: > On Wed, Apr 14, 2021 at 3:00 PM Alexei Starovoitov <ast@fb.com> wrote: > > > > On 4/14/21 1:01 PM, Andrii Nakryiko wrote: > > > Add extra logic to handle map externs (only BTF-defined maps are supported for > > > linking). Re-use the map parsing logic used during bpf_object__open(). Map > > > externs are currently restricted to always and only specify map type, key > > > type and/or size, and value type and/or size. Nothing extra is allowed. If any > > > of those attributes are mismatched between extern and actual map definition, > > > linker will report an error. > > > > I don't get the motivation for this. > > It seems cumbersome to force users to do: > > +extern struct { > > + __uint(type, BPF_MAP_TYPE_HASH); > > + __type(key, key_type); > > + __type(value, value_type); > > + /* no max_entries on extern map definitions */ > > +} map1 SEC(".maps"); > > The intent was to simulate what you'd have in a language with > generics. E.g., if you were declaring extern for a map in C++: > > extern std::map<key_type, value_type> my_map; right, because C++ will mangle types into names. When llvm bpf backend will support C++ front-end it will do the mangling too. I think BPF is ready for C++, but it's a separate discussion, of course. > > but there is only one such full map definition. > > Can all externs to be: > > extern struct {} map1 SEC(".maps"); > > I can certainly modify logic to allow this. But for variables and > funcs we want to enforce type information, right? So I'm not sure why > you think it's bad for maps. I'm not saying it's bad. Traditional linker only deals with names, since we're in C domain, so far, I figured it's an option, but more below. C++ is good analogy too. > So if it's just a multi-file application and you don't care which file > declares that map, you can do a single __weak definition in a header > and forget about it. > > But imagine a BPF library, maintained separately from some BPF > application that is using it. And imagine that for some reason that > BPF library wants/needs to "export" its map directly. In such case, > I'd imagine BPF library author to provide a header with pre-defined > correct extern definition of that map. I'm mainly looking at patch 17 and thinking how that copy paste can be avoided. In C and C++ world the user would do: defs.h: struct S { ... }; extern struct S s; file.c: #include "defs.h" struct S s; and it would work, but afaics it won't work for BPF C in patch 17. If the user does: defs.h: struct my_map { __uint(type, BPF_MAP_TYPE_HASH); __type(key, struct my_key); __type(value, struct my_value); __uint(max_entries, 16); }; extern struct my_map map1 SEC(".maps"); file.c: #include "defs.h" struct my_map map1; // do we need SEC here too? probably not? It won't work for another_filer.c since max_entries are not allowed? Why, btw? So how the user suppose to do this? With __weak in .h ? But if that's the only reasonable choice whe bother supporting extern in the linker? > I originally wanted to let users define which attributes matter and > enforce them (as I mention in the commit description), but that > requires some more work on merging BTF. Now that I'm done with all the > rest logic, I guess I can go and address that as well. I think that would be overkill. It won't match neither C style nor C++. Let's pick one. > So see above about __weak. As for the BPF library providers, that felt > unavoidable (and actually desirable), because that's what they would > do with extern func and extern vars anyways. As far as supporting __weak for map defs, I think __weak in one file.c should be weak for all attributes. Another_file.c should be able to define the same map name without __weak and different types, value/type sizes. Because why not? Sort-of C++ style of override. > so forcing to type+key+value is to make sure that currently all > externs (if there are many) are exactly the same. Because as soon as I > allow some to specify max_entries and some don't, I don't get why max_entries is special. They can be overridden in typical skeleton usage. After open and before load. So max_entries is a default value in map init. Whether it's part of extern or not why should that matter? > Maybe nothing, just there is no single right answer (except the > aspirational implementation I explained above). I'm open to > discussion, btw, not claiming my way is the best way. I'm not suggesting that extern struct {} my_map; is the right answer either. Mainly looking into how user code will look like and trying to make it look the most similar to how C, C++ code traditionally looks. BPF C is reduced and extended C at the same time. BPF C++ will be similar. Certain features will be supported right away, some others will take time. I'm looking at BTF as a language independent concept. Both C and C++ will rely on it. To summarize if max_entries can be supported and ingored in extern when the definition has a different value then it's probably good to enforce that the rest of map fields are the same. Then my .h/.c example above will work. In case of __weak probably all map fields can change. It can be seen as a weak definition of the whole map. Not just weak of the variable. It's a default for everything that can be overridden. While non-weak can override max_entries only. btw for signed progs I'm thinking to allow override of max_entries only, since this attribute doesn't affect safety, correctness, behavior. Meaning max_entries will and will not be part of a signature at the same time. In other words it's necessary to support existing bcc/libbpf-tools. If we go with 'allow max_entries in extern' that would match that behavior.
On Wed, Apr 14, 2021 at 7:01 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Apr 14, 2021 at 04:48:25PM -0700, Andrii Nakryiko wrote: > > On Wed, Apr 14, 2021 at 3:00 PM Alexei Starovoitov <ast@fb.com> wrote: > > > > > > On 4/14/21 1:01 PM, Andrii Nakryiko wrote: > > > > Add extra logic to handle map externs (only BTF-defined maps are supported for > > > > linking). Re-use the map parsing logic used during bpf_object__open(). Map > > > > externs are currently restricted to always and only specify map type, key > > > > type and/or size, and value type and/or size. Nothing extra is allowed. If any > > > > of those attributes are mismatched between extern and actual map definition, > > > > linker will report an error. > > > > > > I don't get the motivation for this. > > > It seems cumbersome to force users to do: > > > +extern struct { > > > + __uint(type, BPF_MAP_TYPE_HASH); > > > + __type(key, key_type); > > > + __type(value, value_type); > > > + /* no max_entries on extern map definitions */ > > > +} map1 SEC(".maps"); > > > > The intent was to simulate what you'd have in a language with > > generics. E.g., if you were declaring extern for a map in C++: > > > > extern std::map<key_type, value_type> my_map; > > right, because C++ will mangle types into names. > When llvm bpf backend will support C++ front-end it will do the mangling too. > I think BPF is ready for C++, but it's a separate discussion, of course. > > > > but there is only one such full map definition. > > > Can all externs to be: > > > extern struct {} map1 SEC(".maps"); > > > > I can certainly modify logic to allow this. But for variables and > > funcs we want to enforce type information, right? So I'm not sure why > > you think it's bad for maps. > > I'm not saying it's bad. > Traditional linker only deals with names, since we're in C domain, so far, > I figured it's an option, but more below. > C++ is good analogy too. > > > So if it's just a multi-file application and you don't care which file > > declares that map, you can do a single __weak definition in a header > > and forget about it. > > > > But imagine a BPF library, maintained separately from some BPF > > application that is using it. And imagine that for some reason that > > BPF library wants/needs to "export" its map directly. In such case, > > I'd imagine BPF library author to provide a header with pre-defined > > correct extern definition of that map. > > I'm mainly looking at patch 17 and thinking how that copy paste can be avoided. > In C and C++ world the user would do: > defs.h: > struct S { > ... > }; > extern struct S s; > file.c: > #include "defs.h" > struct S s; > and it would work, but afaics it won't work for BPF C in patch 17. Yes, you are right, there is no clean way to avoid defining extern and full map definition. Which is the case for functions and variables, except those type signatures tend to be shorter. E.g., if you had void my_func(int arg) { ... } you'd still have to duplicate it as at least: extern void my_func(int); I don't think you can use typedef for this either. > If the user does: > defs.h: > struct my_map { > __uint(type, BPF_MAP_TYPE_HASH); > __type(key, struct my_key); > __type(value, struct my_value); > __uint(max_entries, 16); > }; > extern struct my_map map1 SEC(".maps"); > file.c: > #include "defs.h" > struct my_map map1; // do we need SEC here too? probably not? yeah, we do, all map "variables" are designated with .maps section, otherwise they'll be treated as just a normal global variable (there is no way to distinguish two, generally speaking). > > It won't work for another_filer.c since max_entries are not allowed? > Why, btw? So the idea was that for consumers of extern map definition map type and key/value info was the only thing they should care about and linker should enforce (at least that's how I thought about this and what I think the typical use case would be). E.g., caring about exact max_entries, or numa_node, or pinning, or map_flags, etc, shouldn't be the concern of the consumer of the map. > > So how the user suppose to do this? With __weak in .h ? > But if that's the only reasonable choice whe bother supporting extern in the linker? > > > I originally wanted to let users define which attributes matter and > > enforce them (as I mention in the commit description), but that > > requires some more work on merging BTF. Now that I'm done with all the > > rest logic, I guess I can go and address that as well. > > I think that would be overkill. It won't match neither C style nor C++. > Let's pick one. So I still think we might want to implement it down the road, but let's stick to something simpler for now. See below. > > > So see above about __weak. As for the BPF library providers, that felt > > unavoidable (and actually desirable), because that's what they would > > do with extern func and extern vars anyways. > > As far as supporting __weak for map defs, I think __weak in one file.c > should be weak for all attributes. Another_file.c should be able > to define the same map name without __weak and different types, value/type > sizes. Because why not? Sort-of C++ style of override. That's a significant deviation from semantics of weak variables and functions, but I can see how it's useful. E.g., we can have some potentially compiled out big map definition, but a __weak fallback to a small, but compatible one. > > > so forcing to type+key+value is to make sure that currently all > > externs (if there are many) are exactly the same. Because as soon as I > > allow some to specify max_entries and some don't, > > I don't get why max_entries is special. > They can be overridden in typical skeleton usage. After open and before load. > So max_entries is a default value in map init. Whether it's part of > extern or not why should that matter? This is just a source code-level contract. You can override any attribute of the map at runtime from user-space code. You can even replace one map with an entirely different map (using bpf_map__reuse_fd()). You can change type, etc, etc. The goal here is to not prevent the abuse (I don't think it's possible at linking stage, but at least BPF verifier will stop from something totally stupid), rather for a typical case to make sure that there is no accidental mismatch at the C code level. > > > Maybe nothing, just there is no single right answer (except the > > aspirational implementation I explained above). I'm open to > > discussion, btw, not claiming my way is the best way. > > I'm not suggesting that extern struct {} my_map; is the right answer either. > Mainly looking into how user code will look like and trying to > make it look the most similar to how C, C++ code traditionally looks. > BPF C is reduced and extended C at the same time. > BPF C++ will be similar. Certain features will be supported right away, > some others will take time. > I'm looking at BTF as a language independent concept. > Both C and C++ will rely on it. > > To summarize if max_entries can be supported and ingored in extern > when the definition has a different value then it's probably good to enforce > that the rest of map fields are the same. Then my .h/.c example above will work. > In case of __weak probably all map fields can change. > It can be seen as a weak definition of the whole map. Not just weak of the variable. > It's a default for everything that can be overridden. > While non-weak can override max_entries only. How about we start in the most restrictive way first. Each extern would need to specify all the attributes that should match the map definition. That includes max_entries. That way the typedef struct { ... } my_map_t re-use will work right out of the box. Later, if we see this is not sufficient, we can start relaxing the rules. Ultimately what I proposed above with selective extern attributes enforcement would allow to cover any scenario. Granted it's special compared to C linking style, but given the __weak map definition proposal above also deviates significantly, we can just treat maps specially, but in a "makes sense" way. > > btw for signed progs I'm thinking to allow override of max_entries only, > since this attribute doesn't affect safety, correctness, behavior. > Meaning max_entries will and will not be part of a signature at the same time. > In other words it's necessary to support existing bcc/libbpf-tools. > If we go with 'allow max_entries in extern' that would match that behavior. Ok, unless I misunderstood, allowing and checking all map attributes as a starting point should work, right?
On 4/15/21 1:35 PM, Andrii Nakryiko wrote: > > How about we start in the most restrictive way first. Each extern > would need to specify all the attributes that should match the map > definition. That includes max_entries. That way the typedef struct { > ... } my_map_t re-use will work right out of the box. Later, if we see > this is not sufficient, we can start relaxing the rules. +1 >> >> btw for signed progs I'm thinking to allow override of max_entries only, >> since this attribute doesn't affect safety, correctness, behavior. >> Meaning max_entries will and will not be part of a signature at the same time. >> In other words it's necessary to support existing bcc/libbpf-tools. >> If we go with 'allow max_entries in extern' that would match that behavior. > > Ok, unless I misunderstood, allowing and checking all map attributes > as a starting point should work, right? yes. thanks!
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c index 7f9b91760462..9432c125fa43 100644 --- a/tools/lib/bpf/linker.c +++ b/tools/lib/bpf/linker.c @@ -1467,6 +1467,169 @@ static bool glob_sym_btf_matches(const char *sym_name, bool exact, } } +static bool map_defs_match(const char *sym_name, bool full_match, + const struct btf *main_btf, + const struct btf_map_def *main_def, + const struct btf_map_def *main_inner_def, + const struct btf *extra_btf, + const struct btf_map_def *extra_def, + const struct btf_map_def *extra_inner_def) +{ + const char *reason; + + if (main_def->map_type != extra_def->map_type) { + reason = "type"; + goto mismatch; + } + + /* check key type/size match */ + if (main_def->key_size != extra_def->key_size) { + reason = "key_size"; + goto mismatch; + } + if (!!main_def->key_type_id != !!extra_def->key_type_id) { + reason = "key type"; + goto mismatch; + } + if ((main_def->parts & MAP_DEF_KEY_TYPE) + && !glob_sym_btf_matches(sym_name, true /*exact*/, + main_btf, main_def->key_type_id, + extra_btf, extra_def->key_type_id)) { + reason = "key type"; + goto mismatch; + } + + /* validate value type/size match */ + if (main_def->value_size != extra_def->value_size) { + reason = "value_size"; + goto mismatch; + } + if (!!main_def->value_type_id != !!extra_def->value_type_id) { + reason = "value type"; + goto mismatch; + } + if ((main_def->parts & MAP_DEF_VALUE_TYPE) + && !glob_sym_btf_matches(sym_name, true /*exact*/, + main_btf, main_def->value_type_id, + extra_btf, extra_def->value_type_id)) { + reason = "key type"; + goto mismatch; + } + + /* when having non-extern and extern, we don't compare the rest, + * because externs are currently enforced to only specify map type, + * key, and value info + */ + if (!full_match) + return true; + + if (main_def->max_entries != extra_def->max_entries) { + reason = "max_entries"; + goto mismatch; + } + if (main_def->map_flags != extra_def->map_flags) { + reason = "map_flags"; + goto mismatch; + } + if (main_def->numa_node != extra_def->numa_node) { + reason = "numa_node"; + goto mismatch; + } + if (main_def->pinning != extra_def->pinning) { + reason = "pinning"; + goto mismatch; + } + + if ((main_def->parts & MAP_DEF_INNER_MAP) != (extra_def->parts & MAP_DEF_INNER_MAP)) { + reason = "inner map"; + goto mismatch; + } + + if (main_def->parts & MAP_DEF_INNER_MAP) { + char inner_map_name[128]; + + snprintf(inner_map_name, sizeof(inner_map_name), "%s.inner", sym_name); + + return map_defs_match(inner_map_name, true /*full_match*/, + main_btf, main_inner_def, NULL, + extra_btf, extra_inner_def, NULL); + } + + return true; + +mismatch: + pr_warn("global '%s': map %s mismatch\n", sym_name, reason); + return false; +} + +#define MAP_DEF_EXTERN_PARTS (MAP_DEF_MAP_TYPE \ + | MAP_DEF_KEY_SIZE | MAP_DEF_KEY_TYPE \ + | MAP_DEF_VALUE_SIZE | MAP_DEF_VALUE_TYPE) + +static bool glob_map_defs_match(const char *sym_name, + struct bpf_linker *linker, struct glob_sym *glob_sym, + struct src_obj *obj, Elf64_Sym *sym, int btf_id) +{ + bool sym_is_extern = sym->st_shndx == SHN_UNDEF; + struct btf_map_def dst_def = {}, dst_inner_def = {}; + struct btf_map_def src_def = {}, src_inner_def = {}; + const struct btf_type *t; + int err; + + t = btf__type_by_id(obj->btf, btf_id); + if (!btf_is_var(t)) { + pr_warn("global '%s': invalid map definition type [%d]\n", sym_name, btf_id); + return false; + } + t = skip_mods_and_typedefs(obj->btf, t->type, NULL); + + err = parse_btf_map_def(sym_name, obj->btf, t, true /*strict*/, &src_def, &src_inner_def); + if (err) { + pr_warn("global '%s': invalid map definition\n", sym_name); + return false; + } + + /* We restict extern map defs to only specify map type and key/value + * type or size. Inner map definitions are prohibited for now as well. + */ + if (sym_is_extern && (src_def.parts & ~MAP_DEF_EXTERN_PARTS)) { + pr_warn("global '%s': extern map can specify only map type and key/value info\n", + sym_name); + return false; + } + + /* re-parse existing map definition */ + t = btf__type_by_id(linker->btf, glob_sym->btf_id); + t = skip_mods_and_typedefs(linker->btf, t->type, NULL); + err = parse_btf_map_def(sym_name, linker->btf, t, true /*strict*/, &dst_def, &dst_inner_def); + if (err) { + /* this should not happen, because we already validated it */ + pr_warn("global '%s': invalid dst map definition\n", sym_name); + return false; + } + + if (glob_sym->is_extern != sym_is_extern) { + /* extern map def should be a subset of non-extern one */ + if (sym_is_extern) + /* existing map def is the main one */ + return map_defs_match(sym_name, false /*full_match*/, + linker->btf, &dst_def, &dst_inner_def, + obj->btf, &src_def, &src_inner_def); + else + /* new map def is the main one */ + return map_defs_match(sym_name, false /*full_match*/, + obj->btf, &src_def, &src_inner_def, + linker->btf, &dst_def, &dst_inner_def); + } else { + /* map defs should match exactly regardless of extern/extern + * or non-extern/non-extern case + */ + return map_defs_match(sym_name, true /*full_match*/, + linker->btf, &dst_def, &dst_inner_def, + obj->btf, &src_def, &src_inner_def); + } +} + static bool glob_syms_match(const char *sym_name, struct bpf_linker *linker, struct glob_sym *glob_sym, struct src_obj *obj, Elf64_Sym *sym, size_t sym_idx, int btf_id) @@ -1488,6 +1651,10 @@ static bool glob_syms_match(const char *sym_name, return false; } + /* deal with .maps definitions specially */ + if (glob_sym->sec_id && strcmp(linker->secs[glob_sym->sec_id].sec_name, MAPS_ELF_SEC) == 0) + return glob_map_defs_match(sym_name, linker, glob_sym, obj, sym, btf_id); + if (!glob_sym_btf_matches(sym_name, true /*exact*/, linker->btf, glob_sym->btf_id, obj->btf, btf_id)) return false;
Add extra logic to handle map externs (only BTF-defined maps are supported for linking). Re-use the map parsing logic used during bpf_object__open(). Map externs are currently restricted to always and only specify map type, key type and/or size, and value type and/or size. Nothing extra is allowed. If any of those attributes are mismatched between extern and actual map definition, linker will report an error. The original intent was to allow for extern to specify attributes that matters (to user) to enforce. E.g., if you specify just key information and omit value, then any value fits. Similarly, it should have been possible to enforce map_flags, pinning, and any other possible map attribute. Unfortunately, that means that multiple externs can be only partially overlapping with each other, which means linker would need to combine their type definitions to end up with the most restrictive and fullest map definition. This requires an extra amount of BTF manipulation which at this time was deemed unnecessary and would require further extending generic BTF writer APIs. So that is left for future follow ups, if there will be demand for that. But the idea seems intresting and useful, so I want to document it here. Otherwise extern maps behave intuitively, just like extern vars and funcs. Weak definitions are also supported. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/linker.c | 167 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+)