| Message ID | 20241223-extended-modversions-v11-2-221d184ee9a7@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Extended MODVERSIONS Support | expand |
On Tue, Dec 24, 2024 at 5:13 AM Matthew Maurer <mmaurer@google.com> wrote: > > Generate both the existing modversions format and the new extended one > when running modpost. Presence of this metadata in the final .ko is > guarded by CONFIG_EXTENDED_MODVERSIONS. > > We no longer generate an error on long symbols in modpost if > CONFIG_EXTENDED_MODVERSIONS is set, as they can now be appropriately > encoded in the extended section. These symbols will be skipped in the > previous encoding. An error will still be generated if > CONFIG_EXTENDED_MODVERSIONS is not set. > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com> > Signed-off-by: Matthew Maurer <mmaurer@google.com> > --- > kernel/module/Kconfig | 10 ++++++++ > scripts/Makefile.modpost | 1 + > scripts/mod/modpost.c | 65 +++++++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig > index d443fc504ffca0d1001f880ec496ab1f21fe979e..9568b629a03ce8289d3f3597eefc66fc96445720 100644 > --- a/kernel/module/Kconfig > +++ b/kernel/module/Kconfig > @@ -207,6 +207,16 @@ config ASM_MODVERSIONS > assembly. This can be enabled only when the target architecture > supports it. > > +config EXTENDED_MODVERSIONS > + bool "Extended Module Versioning Support" > + depends on MODVERSIONS > + help > + This enables extended MODVERSIONs support, allowing long symbol > + names to be versioned. > + > + The most likely reason you would enable this is to enable Rust > + support. If unsure, say N. > + > config MODULE_SRCVERSION_ALL > bool "Source checksum for all modules" > help > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > index ab0e94ea62496e11dbaa3ffc289ce546862795ca..40426fc6350985780c0092beb49c6cc29b9eff62 100644 > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -43,6 +43,7 @@ MODPOST = $(objtree)/scripts/mod/modpost > modpost-args = \ > $(if $(CONFIG_MODULES),-M) \ > $(if $(CONFIG_MODVERSIONS),-m) \ > + $(if $(CONFIG_EXTENDED_MODVERSIONS),-x) \ > $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a) \ > $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E) \ > $(if $(KBUILD_MODPOST_WARN),-w) \ > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index fb787a5715f5e8b37713162979ca45355770dc73..c58f08681fc1e37929333ba3a3e0989c4db5a79d 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -33,6 +33,8 @@ static bool module_enabled; > static bool modversions; > /* Is CONFIG_MODULE_SRCVERSION_ALL set? */ > static bool all_versions; > +/* Is CONFIG_EXTENDED_MODVERSIONS set? */ > +static bool extended_modversions; > /* If we are modposting external module set to 1 */ > static bool external_module; > /* Only warn about unresolved symbols */ > @@ -1804,6 +1806,52 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod) > } > } > > +/** > + * Record CRCs for unresolved symbols, supporting long names > + */ > +static void add_extended_versions(struct buffer *b, struct module *mod) > +{ > + struct symbol *s; > + > + if (!extended_modversions) > + return; > + > + buf_printf(b, "\n"); > + buf_printf(b, "static const s32 ____version_ext_crcs[]\n"); In general, 's32' (i.e. signed) and the '%x' printf-format (i.e. unsigned) are an odd combination. I know why you are mixing up these. Previously, we had a reason to use 's32' because the 'crc' field may contain a negative address offset. After commit 7b4537199a4a, there is no more reason for keeping this weirdness. I will apply the following first. https://lore.kernel.org/linux-kbuild/20241228154603.2234284-2-masahiroy@kernel.org/T/#u Then, you can change 's32' to 'u32' in this series. > + buf_printf(b, "__used __section(\"__version_ext_crcs\") = {\n"); > + list_for_each_entry(s, &mod->unresolved_symbols, list) { > + if (!s->module) > + continue; > + if (!s->crc_valid) { > + /* > + * We already warned on this when producing the legacy > + * modversions table. > + */ > + continue; Personally, I tend to avoid a noise change because you will change this comment to warn() in the next commit. It is up to you. > + } > + buf_printf(b, "\t%#8x,\n", s->crc); This is also a copy-paste of the existing code. I like to have zero-padding for better alignment. Please follow this format: https://lore.kernel.org/linux-kbuild/20241228154603.2234284-2-masahiroy@kernel.org/T/#mc46376f41c0313c660990e92ed37689432f325c7 > + } > + buf_printf(b, "};\n"); > + > + buf_printf(b, "static const char ____version_ext_names[]\n"); > + buf_printf(b, "__used __section(\"__version_ext_names\") =\n"); > + list_for_each_entry(s, &mod->unresolved_symbols, list) { > + if (!s->module) > + continue; > + if (!s->crc_valid) { > + /* > + * We already warned on this when producing the legacy > + * modversions table. > + * We need to skip its name too, as the indexes in > + * both tables need to align. > + */ > + continue; > + } Unnecessary braces. Reason: https://github.com/torvalds/linux/blob/v6.12/Documentation/process/coding-style.rst?plain=1#L197 > + buf_printf(b, "\t\"%s\\0\"\n", s->name); > + } > + buf_printf(b, ";\n"); > +} > + > /** > * Record CRCs for unresolved symbols > **/ > @@ -1827,9 +1875,14 @@ static void add_versions(struct buffer *b, struct module *mod) > continue; > } > if (strlen(s->name) >= MODULE_NAME_LEN) { > - error("too long symbol \"%s\" [%s.ko]\n", > - s->name, mod->name); > - break; > + if (extended_modversions) > + /* this symbol will only be in the extended info */ > + continue; You need braces here. Reason: https://github.com/torvalds/linux/blob/v6.12/Documentation/process/coding-style.rst?plain=1#L213 -- Best Regards Masahiro Yamada
diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig index d443fc504ffca0d1001f880ec496ab1f21fe979e..9568b629a03ce8289d3f3597eefc66fc96445720 100644 --- a/kernel/module/Kconfig +++ b/kernel/module/Kconfig @@ -207,6 +207,16 @@ config ASM_MODVERSIONS assembly. This can be enabled only when the target architecture supports it. +config EXTENDED_MODVERSIONS + bool "Extended Module Versioning Support" + depends on MODVERSIONS + help + This enables extended MODVERSIONs support, allowing long symbol + names to be versioned. + + The most likely reason you would enable this is to enable Rust + support. If unsure, say N. + config MODULE_SRCVERSION_ALL bool "Source checksum for all modules" help diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index ab0e94ea62496e11dbaa3ffc289ce546862795ca..40426fc6350985780c0092beb49c6cc29b9eff62 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -43,6 +43,7 @@ MODPOST = $(objtree)/scripts/mod/modpost modpost-args = \ $(if $(CONFIG_MODULES),-M) \ $(if $(CONFIG_MODVERSIONS),-m) \ + $(if $(CONFIG_EXTENDED_MODVERSIONS),-x) \ $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a) \ $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E) \ $(if $(KBUILD_MODPOST_WARN),-w) \ diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index fb787a5715f5e8b37713162979ca45355770dc73..c58f08681fc1e37929333ba3a3e0989c4db5a79d 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -33,6 +33,8 @@ static bool module_enabled; static bool modversions; /* Is CONFIG_MODULE_SRCVERSION_ALL set? */ static bool all_versions; +/* Is CONFIG_EXTENDED_MODVERSIONS set? */ +static bool extended_modversions; /* If we are modposting external module set to 1 */ static bool external_module; /* Only warn about unresolved symbols */ @@ -1804,6 +1806,52 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod) } } +/** + * Record CRCs for unresolved symbols, supporting long names + */ +static void add_extended_versions(struct buffer *b, struct module *mod) +{ + struct symbol *s; + + if (!extended_modversions) + return; + + buf_printf(b, "\n"); + buf_printf(b, "static const s32 ____version_ext_crcs[]\n"); + buf_printf(b, "__used __section(\"__version_ext_crcs\") = {\n"); + list_for_each_entry(s, &mod->unresolved_symbols, list) { + if (!s->module) + continue; + if (!s->crc_valid) { + /* + * We already warned on this when producing the legacy + * modversions table. + */ + continue; + } + buf_printf(b, "\t%#8x,\n", s->crc); + } + buf_printf(b, "};\n"); + + buf_printf(b, "static const char ____version_ext_names[]\n"); + buf_printf(b, "__used __section(\"__version_ext_names\") =\n"); + list_for_each_entry(s, &mod->unresolved_symbols, list) { + if (!s->module) + continue; + if (!s->crc_valid) { + /* + * We already warned on this when producing the legacy + * modversions table. + * We need to skip its name too, as the indexes in + * both tables need to align. + */ + continue; + } + buf_printf(b, "\t\"%s\\0\"\n", s->name); + } + buf_printf(b, ";\n"); +} + /** * Record CRCs for unresolved symbols **/ @@ -1827,9 +1875,14 @@ static void add_versions(struct buffer *b, struct module *mod) continue; } if (strlen(s->name) >= MODULE_NAME_LEN) { - error("too long symbol \"%s\" [%s.ko]\n", - s->name, mod->name); - break; + if (extended_modversions) + /* this symbol will only be in the extended info */ + continue; + else { + error("too long symbol \"%s\" [%s.ko]\n", + s->name, mod->name); + break; + } } buf_printf(b, "\t{ %#8x, \"%s\" },\n", s->crc, s->name); @@ -1960,6 +2013,7 @@ static void write_mod_c_file(struct module *mod) add_header(&buf, mod); add_exported_symbols(&buf, mod); add_versions(&buf, mod); + add_extended_versions(&buf, mod); add_depends(&buf, mod); buf_printf(&buf, "\n"); @@ -2125,7 +2179,7 @@ int main(int argc, char **argv) LIST_HEAD(dump_lists); struct dump_list *dl, *dl2; - while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:")) != -1) { + while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:x")) != -1) { switch (opt) { case 'e': external_module = true; @@ -2174,6 +2228,9 @@ int main(int argc, char **argv) case 'd': missing_namespace_deps = optarg; break; + case 'x': + extended_modversions = true; + break; default: exit(1); }