Re: [PATCH v11 2/5] modpost: Produce extended MODVERSIONS information
From: Masahiro Yamada <masahiroy@kernel.org>
Date: 2024-12-28 16:04:58
Also in:
linux-doc, linux-kbuild, linux-modules, lkml, rust-for-linux
On Tue, Dec 24, 2024 at 5:13 AM Matthew Maurer [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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" helpdiff --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 (local) 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 (local)
+ }
+ 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
quoted hunk ↗ jump to hunk
+ 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