On 10/8/24 20:38, Sami Tolvanen wrote:
> Distributions that want to maintain a stable kABI need the ability
> to make ABI compatible changes to kernel without affecting symbol
> versions, either because of LTS updates or backports.
>
> With genksyms, developers would typically hide these changes from
> version calculation with #ifndef __GENKSYMS__, which would result
> in the symbol version not changing even though the actual type has
> changed. When we process precompiled object files, this isn't an
> option.
>
> To support this use case, add a --stable command line flag that
> gates kABI stability features that are not needed in mainline
> kernels, but can be useful for distributions, and add support for
> kABI rules, which can be used to restrict gendwarfksyms output.
>
> The rules are specified as a set of null-terminated strings stored
> in the .discard.gendwarfksyms.kabi_rules section. Each rule consists
> of four strings as follows:
>
> "version\0type\0target\0value"
>
> The version string ensures the structure can be changed in a
> backwards compatible way. The type string indicates the type of the
> rule, and target and value strings contain rule-specific data.
>
> Initially support two simple rules:
>
> 1. Declaration-only structures
>
> A structure declaration can change into a full definition when
> additional includes are pulled in to the TU, which changes the
> versions of any symbol that references the struct. Add support
> for defining declaration-only structs whose definition is not
> expanded during versioning.
>
> 2. Ignored enum fields
>
> It's possible to add new enum fields without changing the ABI,
> but as the fields are included in symbol versioning, this would
> change the versions. Add support for ignoring specific fields.
>
> Add examples for using the rules under the examples/ directory.
Thanks for coming up with this approach. It makes sense to me.
>
> Signed-off-by: Sami Tolvanen <samitolva...@google.com>
> Acked-by: Neal Gompa <n...@gompa.dev>
> ---
> scripts/gendwarfksyms/Makefile | 1 +
> scripts/gendwarfksyms/dwarf.c | 19 +-
> scripts/gendwarfksyms/examples/kabi.h | 61 ++++++
> scripts/gendwarfksyms/examples/kabi_rules.c | 56 +++++
> scripts/gendwarfksyms/gendwarfksyms.c | 11 +-
> scripts/gendwarfksyms/gendwarfksyms.h | 57 ++++++
> scripts/gendwarfksyms/kabi.c | 214 ++++++++++++++++++++
> 7 files changed, 415 insertions(+), 4 deletions(-)
> create mode 100644 scripts/gendwarfksyms/examples/kabi.h
> create mode 100644 scripts/gendwarfksyms/examples/kabi_rules.c
> create mode 100644 scripts/gendwarfksyms/kabi.c
>
> diff --git a/scripts/gendwarfksyms/Makefile b/scripts/gendwarfksyms/Makefile
> index 6540282dc746..27258c31e839 100644
> --- a/scripts/gendwarfksyms/Makefile
> +++ b/scripts/gendwarfksyms/Makefile
> @@ -5,6 +5,7 @@ gendwarfksyms-objs += gendwarfksyms.o
> gendwarfksyms-objs += cache.o
> gendwarfksyms-objs += die.o
> gendwarfksyms-objs += dwarf.o
> +gendwarfksyms-objs += kabi.o
> gendwarfksyms-objs += symbols.o
> gendwarfksyms-objs += types.o
>
> diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
> index a47a3a0f7a69..b15f1a5db452 100644
> --- a/scripts/gendwarfksyms/dwarf.c
> +++ b/scripts/gendwarfksyms/dwarf.c
> @@ -80,11 +80,12 @@ static bool match_export_symbol(struct state *state,
> Dwarf_Die *die)
> return !!state->sym;
> }
>
> -static bool is_declaration(Dwarf_Die *die)
> +static bool is_declaration(struct die *cache, Dwarf_Die *die)
> {
> bool value;
>
> - return get_flag_attr(die, DW_AT_declaration, &value) && value;
> + return (get_flag_attr(die, DW_AT_declaration, &value) && value) ||
> + kabi_is_struct_declonly(cache->fqn);
> }
>
> /*
> @@ -472,10 +473,11 @@ static void __process_structure_type(struct state
> *state, struct die *cache,
> process(cache, " {");
> process_linebreak(cache, 1);
>
> - is_decl = is_declaration(die);
> + is_decl = is_declaration(cache, die);
>
> if (!is_decl && state->expand.expand) {
> cache_mark_expanded(&state->expansion_cache, die->addr);
> + state->expand.current_fqn = cache->fqn;
> check(process_die_container(state, cache, die, process_func,
> match_func));
> }
> @@ -508,6 +510,15 @@ static void process_enumerator_type(struct state *state,
> struct die *cache,
> {
> Dwarf_Word value;
>
> + if (stable) {
> + /* Get the fqn before we process anything */
> + update_fqn(cache, die);
> +
> + if (kabi_is_enumerator_ignored(state->expand.current_fqn,
> + cache->fqn))
> + return;
> + }
> +
> process_list_comma(state, cache);
> process(cache, "enumerator");
> process_fqn(cache, die);
> @@ -580,6 +591,7 @@ static void state_init(struct state *state)
> state->expand.expand = true;
> state->expand.ptr_depth = 0;
> state->expand.ptr_expansion_depth = 0;
> + state->expand.current_fqn = NULL;
> hash_init(state->expansion_cache.cache);
> }
>
> @@ -589,6 +601,7 @@ static void expansion_state_restore(struct
> expansion_state *state,
> state->expand = saved->expand;
> state->ptr_depth = saved->ptr_depth;
> state->ptr_expansion_depth = saved->ptr_expansion_depth;
> + state->current_fqn = saved->current_fqn;
> }
>
> static void expansion_state_save(struct expansion_state *state,
> diff --git a/scripts/gendwarfksyms/examples/kabi.h
> b/scripts/gendwarfksyms/examples/kabi.h
> new file mode 100644
> index 000000000000..c53e8d4a7d2e
> --- /dev/null
> +++ b/scripts/gendwarfksyms/examples/kabi.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Google LLC
> + *
> + * Example macros for maintaining kABI stability.
> + *
> + * This file is based on android_kabi.h, which has the following notice:
> + *
> + * Heavily influenced by rh_kabi.h which came from the RHEL/CENTOS kernel
> + * and was:
> + * Copyright (c) 2014 Don Zickus
> + * Copyright (c) 2015-2018 Jiri Benc
> + * Copyright (c) 2015 Sabrina Dubroca, Hannes Frederic Sowa
> + * Copyright (c) 2016-2018 Prarit Bhargava
> + * Copyright (c) 2017 Paolo Abeni, Larry Woodman
> + */
> +
> +#ifndef __KABI_H__
> +#define __KABI_H__
> +
> +/* Kernel macros for userspace testing. */
> +#ifndef __aligned
> +#define __aligned(x) __attribute__((__aligned__(x)))
> +#endif
> +#ifndef __used
> +#define __used __attribute__((__used__))
> +#endif
> +#ifndef __section
> +#define __section(section) __attribute__((__section__(section)))
> +#endif
> +#ifndef __PASTE
> +#define ___PASTE(a, b) a##b
> +#define __PASTE(a, b) ___PASTE(a, b)
> +#endif
> +#ifndef __stringify
> +#define __stringify_1(x...) #x
> +#define __stringify(x...) __stringify_1(x)
> +#endif
> +
> +#define __KABI_RULE(hint, target, value) \
> + static const char __PASTE(__gendwarfksyms_rule_, \
> + __COUNTER__)[] __used __aligned(1) \
> + __section(".discard.gendwarfksyms.kabi_rules") = \
> + "1\0" #hint "\0" #target "\0" #value
> +
> +/*
> + * KABI_USE_ARRAY(fqn)
> + * Treat the struct fqn as a declaration, i.e. even if a definition
> + * is available, don't expand the contents.
> + */
> +#define KABI_STRUCT_DECLONLY(fqn) __KABI_RULE(struct_declonly, fqn, ;)
Nit: s/KABI_USE_ARRAY/KABI_STRUCT_DECLONLY/ in the preceding comment.
> +
> +/*
> + * KABI_ENUMERATOR_IGNORE(fqn, field)
> + * When expanding enum fqn, skip the provided field. This makes it
> + * possible to hide added enum fields from versioning.
> + */
> +#define KABI_ENUMERATOR_IGNORE(fqn, field) \
> + __KABI_RULE(enumerator_ignore, fqn, field)
> +
> +#endif /* __KABI_H__ */
> diff --git a/scripts/gendwarfksyms/examples/kabi_rules.c
> b/scripts/gendwarfksyms/examples/kabi_rules.c
> new file mode 100644
> index 000000000000..446818e67d80
> --- /dev/null
> +++ b/scripts/gendwarfksyms/examples/kabi_rules.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Google LLC
> + *
> + * Examples for kABI rules with --stable.
> + */
> +
> +/*
> + * The comments below each example contain the expected gendwarfksyms
> + * output which can be verified using LLVM's FileCheck tool:
> + *
> + * https://llvm.org/docs/CommandGuide/FileCheck.html
> + *
> + * RUN: gcc -g -c examples/kabi_rules.c -o examples/kabi_rules.o
> + *
> + * Verify --stable output:
> + *
> + * RUN: echo -e "ex0\nex1" | \
> + * RUN: ./gendwarfksyms --stable --dump-dies \
> + * RUN: examples/kabi_rules.o 2>&1 >/dev/null | \
> + * RUN: FileCheck examples/kabi_rules.c --check-prefix=STABLE
> + */
It would be useful to make this test automated. Overall, I believe
gendwarfksyms should have a set of automated tests to verify its
functionality. At a minimum, I think we would want to work out some
blueprint how to write them. Should they be added to kselftests, or
would something like kconfig/tests be more appropriate? How to write
tests with stable DWARF data that ideally work across all platforms?
More tests can be then added incrementally.
> +
> +#include "kabi.h"
> +
> +struct s {
> + int a;
> +};
> +
> +KABI_STRUCT_DECLONLY(s);
> +
> +struct s e0;
> +
> +/*
> + * STABLE: variable structure_type s {
> + * STABLE-NEXT: }
> + */
> +
> +enum e {
> + A,
> + B,
> + C,
> + D,
> +};
> +
> +KABI_ENUMERATOR_IGNORE(e, B);
> +KABI_ENUMERATOR_IGNORE(e, C);
> +
> +enum e e1;
> +
> +/*
> + * STABLE: variable enumeration_type e {
> + * STABLE-NEXT: enumerator A = 0 ,
> + * STABLE-NEXT: enumerator D = 3
> + * STABLE-NEXT: } byte_size(4)
> + */
> diff --git a/scripts/gendwarfksyms/gendwarfksyms.c
> b/scripts/gendwarfksyms/gendwarfksyms.c
> index e90d909d259b..21abf1c98366 100644
> --- a/scripts/gendwarfksyms/gendwarfksyms.c
> +++ b/scripts/gendwarfksyms/gendwarfksyms.c
> @@ -25,6 +25,8 @@ int dump_die_map;
> int dump_types;
> /* Print out expanded type strings used for symbol versions */
> int dump_versions;
> +/* Support kABI stability features */
> +int stable;
> /* Write a symtypes file */
> int symtypes;
> static const char *symtypes_file;
> @@ -38,6 +40,7 @@ static void usage(void)
> " --dump-die-map Print debugging information about die_map
> changes\n"
> " --dump-types Dump type strings\n"
> " --dump-versions Dump expanded type strings used for
> symbol versions\n"
> + " -s, --stable Support kABI stability features\n"
> " -T, --symtypes file Write a symtypes file\n"
> " -h, --help Print this message\n"
> "\n",
> @@ -97,17 +100,21 @@ int main(int argc, char **argv)
> { "dump-die-map", 0, &dump_die_map, 1 },
> { "dump-types", 0, &dump_types, 1 },
> { "dump-versions", 0, &dump_versions, 1 },
> + { "stable", 0, NULL, 's' },
> { "symtypes", 1, NULL, 'T' },
> { "help", 0, NULL, 'h' },
> { 0, 0, NULL, 0 } };
>
> - while ((opt = getopt_long(argc, argv, "dT:h", opts, NULL)) != EOF) {
> + while ((opt = getopt_long(argc, argv, "dsT:h", opts, NULL)) != EOF) {
> switch (opt) {
> case 0:
> break;
> case 'd':
> debug = 1;
> break;
> + case 's':
> + stable = 1;
> + break;
> case 'T':
> symtypes = 1;
> symtypes_file = optarg;
> @@ -151,6 +158,7 @@ int main(int argc, char **argv)
> strerror(errno));
>
> symbol_read_symtab(fd);
> + kabi_read_rules(fd);
>
> dwfl = dwfl_begin(&callbacks);
> if (!dwfl)
> @@ -167,6 +175,7 @@ int main(int argc, char **argv)
> error("dwfl_getmodules failed for '%s'", argv[n]);
>
> dwfl_end(dwfl);
> + kabi_free();
> }
>
> if (symfile)
> diff --git a/scripts/gendwarfksyms/gendwarfksyms.h
> b/scripts/gendwarfksyms/gendwarfksyms.h
> index 814f53ef799e..f32ad4389b58 100644
> --- a/scripts/gendwarfksyms/gendwarfksyms.h
> +++ b/scripts/gendwarfksyms/gendwarfksyms.h
> @@ -27,6 +27,7 @@ extern int dump_dies;
> extern int dump_die_map;
> extern int dump_types;
> extern int dump_versions;
> +extern int stable;
> extern int symtypes;
>
> /*
> @@ -225,6 +226,7 @@ struct expansion_state {
> bool expand;
> unsigned int ptr_depth;
> unsigned int ptr_expansion_depth;
> + const char *current_fqn;
> };
>
> struct state {
> @@ -256,4 +258,59 @@ void process_cu(Dwarf_Die *cudie);
>
> void generate_symtypes_and_versions(FILE *file);
>
> +/*
> + * kabi.c
> + */
> +
> +#define KABI_RULE_SECTION ".discard.gendwarfksyms.kabi_rules"
> +#define KABI_RULE_VERSION "1"
> +
> +/*
> + * The rule section consists of four null-terminated strings per
> + * entry:
> + *
> + * 1. version
> + * Entry format version. Must match KABI_RULE_VERSION.
> + *
> + * 2. type
> + * Type of the kABI rule. Must be one of the tags defined below.
> + *
> + * 3. target
> + * Rule-dependent target, typically the fully qualified name of
> + * the target DIE.
> + *
> + * 4. value
> + * Rule-dependent value.
> + */
> +#define KABI_RULE_MIN_ENTRY_SIZE \
> + (/* version\0 */ 2 + /* type\0 */ 2 + /* target\0" */ 2 + \
> + /* value\0 */ 2)
> +#define KABI_RULE_EMPTY_VALUE ";"
Hmm, is there a reason why an empty value is ";" instead of just ""?
> +
> +/*
> + * Rule: struct_declonly
> + * - For the struct in the target field, treat it as a declaration
> + * only even if a definition is available.
> + */
> +#define KABI_RULE_TAG_STRUCT_DECLONLY "struct_declonly"
> +
> +/*
> + * Rule: enumerator_ignore
> + * - For the enum in the target field, ignore the named enumerator
> + * in the value field.
> + */
> +#define KABI_RULE_TAG_ENUMERATOR_IGNORE "enumerator_ignore"
> +
> +enum kabi_rule_type {
> + KABI_RULE_TYPE_UNKNOWN,
> + KABI_RULE_TYPE_STRUCT_DECLONLY,
> + KABI_RULE_TYPE_ENUMERATOR_IGNORE,
> +};
Nit: All new KABI_* defines and the enum kabi_rule_type added in
gendwarfksyms.h are used only locally from kabi.c, so they could be
moved in that file.
> +
> +bool kabi_is_enumerator_ignored(const char *fqn, const char *field);
> +bool kabi_is_struct_declonly(const char *fqn);
> +
> +void kabi_read_rules(int fd);
> +void kabi_free(void);
> +
> #endif /* __GENDWARFKSYMS_H */
> diff --git a/scripts/gendwarfksyms/kabi.c b/scripts/gendwarfksyms/kabi.c
> new file mode 100644
> index 000000000000..a5414382782c
> --- /dev/null
> +++ b/scripts/gendwarfksyms/kabi.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Google LLC
> + */
> +
> +#include "gendwarfksyms.h"
> +
> +#define RULE_HASH_BITS 10
> +
> +struct rule {
> + enum kabi_rule_type type;
> + const char *target;
> + const char *value;
> + struct hlist_node hash;
> +};
What is the idea behind using 'const char *' instead of 'char *' for
owned strings in structures?
> +
> +/* { type, target, value } -> struct rule */
> +static HASHTABLE_DEFINE(rules, 1 << RULE_HASH_BITS);
> +
> +static inline unsigned int rule_hash(enum kabi_rule_type type,
> + const char *target, const char *value)
> +{
> + return hash_32(type) ^ hash_str(target) ^ hash_str(value);
> +}
> +
> +static inline unsigned int __rule_hash(const struct rule *rule)
> +{
> + return rule_hash(rule->type, rule->target, rule->value);
> +}
Nit: Perhaps call the two hash functions rule_values_hash() and
rule_hash() to avoid the "__" prefix?
As a general comment, I believe the gendwarfksyms code overuses the "__"
prefix. Similarly, I find harder to navigate its code when, in a few
instances, there is a function named <verb>_<object>() and another as
<object>_<verb>(). An example of both would be the functions
expand_type(), type_expand() and __type_expand().
> +
> +static inline const char *get_rule_field(const char **pos, ssize_t *left)
> +{
> + const char *start = *pos;
> + size_t len;
> +
> + if (*left <= 1)
> + error("unexpected end of kABI rules");
> +
> + len = strnlen(start, *left);
> + if (!len)
> + error("empty kABI rule field");
> +
> + len += 1;
> + *pos += len;
> + *left -= len;
> +
> + return start;
> +}
> +
> +void kabi_read_rules(int fd)
> +{
> + GElf_Shdr shdr_mem;
> + GElf_Shdr *shdr;
> + Elf_Data *rule_data = NULL;
> + Elf_Scn *scn;
> + Elf *elf;
> + size_t shstrndx;
> + const char *rule_str;
> + ssize_t left;
> + int i;
> +
> + const struct {
> + enum kabi_rule_type type;
> + const char *tag;
> + } rule_types[] = {
> + {
> + .type = KABI_RULE_TYPE_STRUCT_DECLONLY,
> + .tag = KABI_RULE_TAG_STRUCT_DECLONLY,
> + },
> + {
> + .type = KABI_RULE_TYPE_ENUMERATOR_IGNORE,
> + .tag = KABI_RULE_TAG_ENUMERATOR_IGNORE,
> + },
> + };
> +
> + if (!stable)
> + return;
> +
> + if (elf_version(EV_CURRENT) != EV_CURRENT)
> + error("elf_version failed: %s", elf_errmsg(-1));
> +
> + elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> + if (!elf)
> + error("elf_begin failed: %s", elf_errmsg(-1));
> +
> + if (elf_getshdrstrndx(elf, &shstrndx) < 0)
> + error("elf_getshdrstrndx failed: %s", elf_errmsg(-1));
> +
> + scn = elf_nextscn(elf, NULL);
> +
> + while (scn) {
> + shdr = gelf_getshdr(scn, &shdr_mem);
> + if (shdr) {
Isn't it an error when gelf_getshdr() returns NULL and as such it should
be reported with error()? If this makes sense then the same handling
should be implemented in symbols.c:elf_for_each_global().
> + const char *sname =
> + elf_strptr(elf, shstrndx, shdr->sh_name);
> +
> + if (sname && !strcmp(sname, KABI_RULE_SECTION)) {
> + rule_data = elf_getdata(scn, NULL);
Similarly here for elf_strptr() and elf_getdata().
> + break;
> + }
> + }
> +
> + scn = elf_nextscn(elf, scn);
> + }
> +
> + if (!rule_data) {
> + debug("kABI rules not found");
> + return;
> + }
> +
> + rule_str = rule_data->d_buf;
> + left = shdr->sh_size;
> +
> + if (left < KABI_RULE_MIN_ENTRY_SIZE)
> + error("kABI rule section too small: %zd bytes", left);
> +
> + if (rule_str[left - 1] != '\0')
> + error("kABI rules are not null-terminated");
> +
> + while (left > KABI_RULE_MIN_ENTRY_SIZE) {
> + enum kabi_rule_type type = KABI_RULE_TYPE_UNKNOWN;
> + const char *field;
> + struct rule *rule;
> +
> + /* version */
> + field = get_rule_field(&rule_str, &left);
> +
> + if (strcmp(field, KABI_RULE_VERSION))
> + error("unsupported kABI rule version: '%s'", field);
> +
> + /* type */
> + field = get_rule_field(&rule_str, &left);
> +
> + for (i = 0; i < ARRAY_SIZE(rule_types); i++) {
> + if (!strcmp(field, rule_types[i].tag)) {
> + type = rule_types[i].type;
> + break;
> + }
> + }
> +
> + if (type == KABI_RULE_TYPE_UNKNOWN)
> + error("unsupported kABI rule type: '%s'", field);
> +
> + rule = xmalloc(sizeof(struct rule));
> +
> + rule->type = type;
> + rule->target = xstrdup(get_rule_field(&rule_str, &left));
> + rule->value = xstrdup(get_rule_field(&rule_str, &left));
> +
> + hash_add(rules, &rule->hash, __rule_hash(rule));
> +
> + debug("kABI rule: type: '%s', target: '%s', value: '%s'", field,
> + rule->target, rule->value);
> + }
> +
> + if (left > 0)
> + warn("unexpected data at the end of the kABI rules section");
> +
> + check(elf_end(elf));
> +}
> +
> +bool kabi_is_struct_declonly(const char *fqn)
> +{
> + struct rule *rule;
> +
> + if (!stable)
> + return false;
> + if (!fqn || !*fqn)
> + return false;
> +
> + hash_for_each_possible(rules, rule, hash,
> + rule_hash(KABI_RULE_TYPE_STRUCT_DECLONLY, fqn,
> + KABI_RULE_EMPTY_VALUE)) {
> + if (rule->type == KABI_RULE_TYPE_STRUCT_DECLONLY &&
> + !strcmp(fqn, rule->target))
> + return true;
> + }
> +
> + return false;
> +}
> +
> +bool kabi_is_enumerator_ignored(const char *fqn, const char *field)
> +{
> + struct rule *rule;
> +
> + if (!stable)
> + return false;
> + if (!fqn || !*fqn || !field || !*field)
> + return false;
> +
> + hash_for_each_possible(rules, rule, hash,
> + rule_hash(KABI_RULE_TYPE_ENUMERATOR_IGNORE, fqn,
> + field)) {
> + if (rule->type == KABI_RULE_TYPE_ENUMERATOR_IGNORE &&
> + !strcmp(fqn, rule->target) && !strcmp(field, rule->value))
> + return true;
> + }
> +
> + return false;
> +}
> +
> +void kabi_free(void)
> +{
> + struct hlist_node *tmp;
> + struct rule *rule;
> +
> + hash_for_each_safe(rules, rule, tmp, hash) {
> + free((void *)rule->target);
> + free((void *)rule->value);
> + free(rule);
> + }
> +
> + hash_init(rules);
> +}
--
Thanks,
Petr
