[PATCH v8 0/3] Extended MODVERSIONS Support

[PATCH v8 0/3] Extended MODVERSIONS Support

[Matthew Maurer]

This patch series is intended for use alongside the Implement DWARF
modversions series [1] to enable RUST and MODVERSIONS at the same
time.

Elsewhere, we've seen a desire for long symbol name support for LTO
symbol names [2], and the previous series came up [3] as a possible
solution rather than hashing, which some have objected [4] to.

This series adds a MODVERSIONS format which uses a section per column.
This avoids userspace tools breaking if we need to make a similar change
to the format in the future - we would do so by adding a new section,
rather than editing the struct definition. In the new format, the name
section is formatted as a concatenated sequence of NUL-terminated
strings, which allows for arbitrary length names.

Emitting the extended format is guarded by CONFIG_EXTENDED_MODVERSIONS,
but the kernel always knows how to validate both the original and
extended formats.

Selecting RUST and MODVERSIONS is now possible if GENDWARFKSYMS is
selected, and will implicitly select EXTENDED_MODVERSIONS.

This series depends upon the module verification refactor patches [5]
that were split off of v5, and DWARF-based versions [1].

[1] https://lore.kernel.org/lkml/20241030170106.1501763-21-samitolvanen@google.com/
[2] https://lore.kernel.org/lkml/20240605032120.3179157-1-song@kernel.org/
[3] https://lore.kernel.org/lkml/ZoxbEEsK40ASi1cY@bombadil.infradead.org/
[4] https://lore.kernel.org/lkml/0b2697fd-7ab4-469f-83a6-ec9ebc701ba0@suse.com/
[5] https://lore.kernel.org/linux-modules/20241015231651.3851138-1-mmaurer@google.com/T/#t

Changes in v8:
- Rebased onto latest version of Sami's series, on top of v6.12-rc5
- Pass --stable when KBUILD_GENDWARFKSYMS_STABLE is set.
- Flipped MODVERSIONS/GENDWARFKSYMS order in deps for CONFIG_RUST
- Picked up trailers

v7: https://lore.kernel.org/r/20241023-extended-modversions-v7-0-339787b43373@google.com
- Fix modpost to detect EXTENDED_MODVERSIONS based on a flag
- Drop patches to fix export_report.pl
- Switch from conditional compilation in .mod.c to conditional emission
  in modpost
- Factored extended modversion emission into its own function
- Allow RUST + MODVERSIONS if GENDWARFKSYMS is enabled by selecting
  EXTENDED_MODVERSIONS

v6: https://lore.kernel.org/lkml/20241015231925.3854230-1-mmaurer@google.com/
- Splits verification refactor Luis requested out to a separate change
- Clarifies commits around export_report.pl repairs
- Add CONFIG_EXTENDED_MODVERSIONS to control whether extended
  information is included in the module, per Luis's request.

v5: https://lore.kernel.org/all/20240925233854.90072-1-mmaurer@google.com/
- Addresses Sami's comments from v3 that I missed in v4 (missing early
  return, extra parens)

v4: https://lore.kernel.org/asahi/20240924212024.540574-1-mmaurer@google.com/
- Fix incorrect dot munging in PPC

v3: https://lore.kernel.org/lkml/87le0w2hop.fsf@mail.lhotse/T/
- Split up the module verification refactor into smaller patches, per
  Greg K-H's suggestion.

v2: https://lore.kernel.org/all/20231118025748.2778044-1-mmaurer@google.com/
- Add loading/verification refactor before modifying, per Luis's request

v1: https://lore.kernel.org/rust-for-linux/20231115185858.2110875-1-mmaurer@google.com/

--
2.47.0.rc1.288.g06298d1525-goog

---
Matthew Maurer (2):
      modules: Support extended MODVERSIONS info
      modpost: Produce extended MODVERSIONS information

Sami Tolvanen (1):
      rust: Use gendwarfksyms + extended modversions for CONFIG_MODVERSIONS

 arch/powerpc/kernel/module_64.c | 24 ++++++++++-
 init/Kconfig                    |  3 +-
 kernel/module/Kconfig           | 10 +++++
 kernel/module/internal.h        | 11 +++++
 kernel/module/main.c            | 92 +++++++++++++++++++++++++++++++++++++----
 kernel/module/version.c         | 45 ++++++++++++++++++++
 rust/Makefile                   | 33 ++++++++++++++-
 scripts/Makefile.modpost        |  1 +
 scripts/mod/modpost.c           | 65 +++++++++++++++++++++++++++--
 9 files changed, 267 insertions(+), 17 deletions(-)
---
base-commit: ac746e6156c4d6d7b46ba2102acf644ea2aa4aac
change-id: 20241022-extended-modversions-a7b44dfbfff1
prerequisite-message-id: <20241015231651.3851138-1-mmaurer@google.com>
prerequisite-patch-id: 7b7bf0c0c0f484703e29a452dc99dc99711c051b
prerequisite-patch-id: 8cc51bc35ddd4c268b5ccba4c3a74af3dbee8bee
prerequisite-patch-id: 0c4fded10660440fc59e256d6456ac865b70f04b
prerequisite-patch-id: 121f9313b4bde4e374ba37132fbf36e435f7ada5
prerequisite-patch-id: bbd158ee717130fd5d5fc4b7c0613d89c2adcc45
prerequisite-patch-id: af83141b7e527e3d1936326e3c9996bddfa45642
prerequisite-patch-id: 61a51b5c2ab3dc55031fcb2a2b56b4b44b9fabd3
prerequisite-patch-id: 63b4bdc24ff078bd48b8dcec28a334042450796e
prerequisite-patch-id: 429739b875bf7400ece44ec2529f43051b43dd45
prerequisite-patch-id: 55a19e6365f3d60ac5dbea13e320ece71538de25
prerequisite-patch-id: d5ab8e10e837e8193c265dc8548b97655a56db27
prerequisite-patch-id: e2f5364a0c5f3c9341aaa183f97fb7544b1c9dba
prerequisite-message-id: <20241030170106.1501763-21-samitolvanen@google.com>
prerequisite-patch-id: 08b46e0d1e37c262c08da6db4a87728d7b3047cc
prerequisite-patch-id: 0a1e1ac99f325f4df27bd35f00bd4914f5386cb9
prerequisite-patch-id: 32a05b89083cfed15e5b877664b0c8138c40d09b
prerequisite-patch-id: e192e2a692c40d96cba919e3baae68c441ab25e4
prerequisite-patch-id: 50e884d28c720e90f201aae7801590d19736541b
prerequisite-patch-id: 4d6a826429c519b581d01215e1d9c7373fdfd8c6
prerequisite-patch-id: 0dcd84187b222adf52696dbcab303d683d087dd2
prerequisite-patch-id: 0abe8634eb844a85e8dc51c1cd3970cf96cc494a
prerequisite-patch-id: 5fabb630792f9304f200b5996314f3c2ae4c83ae
prerequisite-patch-id: 4859bef5bb0f6b2142bd7a0e89973f7a79009624
prerequisite-patch-id: a5cf20d27871bf63be64ac79cc81e5eb9d117b89
prerequisite-patch-id: f9cacaf82d1f2a93ade313c44269fb871e7b9ce2
prerequisite-patch-id: 9fcea62d87a577d69ec262fe76b81c889c1bdf92
prerequisite-patch-id: 310f411df60af62002a3898eafe60c1687c0e9b8
prerequisite-patch-id: c21f85ffe5c7684c1ffc87af716e2e50498d5c92
prerequisite-patch-id: a372f88626c3dda51eab6c6af132a76141ff20cc
prerequisite-patch-id: 57d2fe708769154a6494fb1fece56911dea00687
prerequisite-patch-id: e5fb35555f6a95bc9953bddebba0612f422146c4
prerequisite-patch-id: 624e6794e5003cff734873894c2343595b45244b

Best regards,
-- 
Matthew Maurer <mmaurer@google.com>

[PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Matthew Maurer]

Adds a new format for MODVERSIONS which stores each field in a separate
ELF section. This initially adds support for variable length names, but
could later be used to add additional fields to MODVERSIONS in a
backwards compatible way if needed. Any new fields will be ignored by
old user tooling, unlike the current format where user tooling cannot
tolerate adjustments to the format (for example making the name field
longer).

Since PPC munges its version records to strip leading dots, we reproduce
the munging for the new format. Other architectures do not appear to
have architecture-specific usage of this information.

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 arch/powerpc/kernel/module_64.c | 24 ++++++++++-
 kernel/module/internal.h        | 11 +++++
 kernel/module/main.c            | 92 +++++++++++++++++++++++++++++++++++++----
 kernel/module/version.c         | 45 ++++++++++++++++++++
 4 files changed, 162 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index e9bab599d0c2745e4d2b5cae04f2c56395c24654..02ada0b057cef6b2f29fa7519a5d52acac740ee5 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -355,6 +355,24 @@ static void dedotify_versions(struct modversion_info *vers,
 		}
 }
 
+/* Same as normal versions, remove a leading dot if present. */
+static void dedotify_ext_version_names(char *str_seq, unsigned long size)
+{
+	unsigned long out = 0;
+	unsigned long in;
+	char last = '\0';
+
+	for (in = 0; in < size; in++) {
+		/* Skip one leading dot */
+		if (last == '\0' && str_seq[in] == '.')
+			in++;
+		last = str_seq[in];
+		str_seq[out++] = last;
+	}
+	/* Zero the trailing portion of the names table for robustness */
+	memset(&str_seq[out], 0, size - out);
+}
+
 /*
  * Undefined symbols which refer to .funcname, hack to funcname. Make .TOC.
  * seem to be defined (value set later).
@@ -424,10 +442,12 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 			me->arch.toc_section = i;
 			if (sechdrs[i].sh_addralign < 8)
 				sechdrs[i].sh_addralign = 8;
-		}
-		else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
+		} else if (strcmp(secstrings + sechdrs[i].sh_name, "__versions") == 0)
 			dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
 					  sechdrs[i].sh_size);
+		else if (strcmp(secstrings + sechdrs[i].sh_name, "__version_ext_names") == 0)
+			dedotify_ext_version_names((void *)hdr + sechdrs[i].sh_offset,
+						   sechdrs[i].sh_size);
 
 		if (sechdrs[i].sh_type == SHT_SYMTAB)
 			dedotify((void *)hdr + sechdrs[i].sh_offset,
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index daef2be8390222c22220e2f168baa8d35ad531b9..59959c21b205bf91c0073260885743098c4022cf 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -86,6 +86,8 @@ struct load_info {
 		unsigned int vers;
 		unsigned int info;
 		unsigned int pcpu;
+		unsigned int vers_ext_crc;
+		unsigned int vers_ext_name;
 	} index;
 };
 
@@ -389,6 +391,15 @@ void module_layout(struct module *mod, struct modversion_info *ver, struct kerne
 		   struct kernel_symbol *ks, struct tracepoint * const *tp);
 int check_modstruct_version(const struct load_info *info, struct module *mod);
 int same_magic(const char *amagic, const char *bmagic, bool has_crcs);
+struct modversion_info_ext {
+	size_t remaining;
+	const s32 *crc;
+	const char *name;
+};
+void modversion_ext_start(const struct load_info *info, struct modversion_info_ext *ver);
+void modversion_ext_advance(struct modversion_info_ext *ver);
+#define for_each_modversion_info_ext(ver, info) \
+	for (modversion_ext_start(info, &ver); ver.remaining > 0; modversion_ext_advance(&ver))
 #else /* !CONFIG_MODVERSIONS */
 static inline int check_version(const struct load_info *info,
 				const char *symname,
diff --git a/kernel/module/main.c b/kernel/module/main.c
index b40b632f00a65e66ed73c4b386ef1f323a5b790c..9a9feca344f8bb06408d350e13f759bb909962cd 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2039,6 +2039,82 @@ static int elf_validity_cache_index_str(struct load_info *info)
 	return 0;
 }
 
+/**
+ * elf_validity_cache_index_versions() - Validate and cache version indices
+ * @info:  Load info to cache version indices in.
+ *         Must have &load_info->sechdrs and &load_info->secstrings populated.
+ * @flags: Load flags, relevant to suppress version loading, see
+ *         uapi/linux/module.h
+ *
+ * If we're ignoring modversions based on @flags, zero all version indices
+ * and return validity. Othewrise check:
+ *
+ * * If "__version_ext_crcs" is present, "__version_ext_names" is present
+ * * There is a name present for every crc
+ *
+ * Then populate:
+ *
+ * * &load_info->index.vers
+ * * &load_info->index.vers_ext_crc
+ * * &load_info->index.vers_ext_names
+ *
+ * if present.
+ *
+ * Return: %0 if valid, %-ENOEXEC on failure.
+ */
+static int elf_validity_cache_index_versions(struct load_info *info, int flags)
+{
+	unsigned int vers_ext_crc;
+	unsigned int vers_ext_name;
+	size_t crc_count;
+	size_t remaining_len;
+	size_t name_size;
+	char *name;
+
+	/* If modversions were suppressed, pretend we didn't find any */
+	if (flags & MODULE_INIT_IGNORE_MODVERSIONS) {
+		info->index.vers = 0;
+		info->index.vers_ext_crc = 0;
+		info->index.vers_ext_name = 0;
+		return 0;
+	}
+
+	vers_ext_crc = find_sec(info, "__version_ext_crcs");
+	vers_ext_name = find_sec(info, "__version_ext_names");
+
+	/* If we have one field, we must have the other */
+	if (!!vers_ext_crc != !!vers_ext_name) {
+		pr_err("extended version crc+name presence does not match");
+		return -ENOEXEC;
+	}
+
+	/*
+	 * If we have extended version information, we should have the same
+	 * number of entries in every section.
+	 */
+	if (vers_ext_crc) {
+		crc_count = info->sechdrs[vers_ext_crc].sh_size / sizeof(s32);
+		name = (void *)info->hdr +
+			info->sechdrs[vers_ext_name].sh_offset;
+		remaining_len = info->sechdrs[vers_ext_name].sh_size;
+
+		while (crc_count--) {
+			name_size = strnlen(name, remaining_len) + 1;
+			if (name_size > remaining_len) {
+				pr_err("more extended version crcs than names");
+				return -ENOEXEC;
+			}
+			remaining_len -= name_size;
+			name += name_size;
+		}
+	}
+
+	info->index.vers = find_sec(info, "__versions");
+	info->index.vers_ext_crc = vers_ext_crc;
+	info->index.vers_ext_name = vers_ext_name;
+	return 0;
+}
+
 /**
  * elf_validity_cache_index() - Resolve, validate, cache section indices
  * @info:  Load info to read from and update.
@@ -2053,9 +2129,7 @@ static int elf_validity_cache_index_str(struct load_info *info)
  * * elf_validity_cache_index_mod()
  * * elf_validity_cache_index_sym()
  * * elf_validity_cache_index_str()
- *
- * If versioning is not suppressed via flags, load the version index from
- * a section called "__versions" with no validation.
+ * * elf_validity_cache_index_versions()
  *
  * If CONFIG_SMP is enabled, load the percpu section by name with no
  * validation.
@@ -2078,11 +2152,9 @@ static int elf_validity_cache_index(struct load_info *info, int flags)
 	err = elf_validity_cache_index_str(info);
 	if (err < 0)
 		return err;
-
-	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
-		info->index.vers = 0; /* Pretend no __versions section! */
-	else
-		info->index.vers = find_sec(info, "__versions");
+	err = elf_validity_cache_index_versions(info, flags);
+	if (err < 0)
+		return err;
 
 	info->index.pcpu = find_pcpusec(info);
 
@@ -2293,6 +2365,10 @@ static int rewrite_section_headers(struct load_info *info, int flags)
 
 	/* Track but don't keep modinfo and version sections. */
 	info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
+	info->sechdrs[info->index.vers_ext_crc].sh_flags &=
+		~(unsigned long)SHF_ALLOC;
+	info->sechdrs[info->index.vers_ext_name].sh_flags &=
+		~(unsigned long)SHF_ALLOC;
 	info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
 	return 0;
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 53f43ac5a73e9d537a9e95ff97728a51fad0e797..c246d40879706d4f413fa7ea9bbe2264ea1b2aa8 100644
--- a/kernel/module/version.c
+++ b/kernel/module/version.c
@@ -19,11 +19,28 @@ int check_version(const struct load_info *info,
 	unsigned int versindex = info->index.vers;
 	unsigned int i, num_versions;
 	struct modversion_info *versions;
+	struct modversion_info_ext version_ext;
 
 	/* Exporting module didn't supply crcs?  OK, we're already tainted. */
 	if (!crc)
 		return 1;
 
+	/* If we have extended version info, rely on it */
+	if (info->index.vers_ext_crc) {
+		for_each_modversion_info_ext(version_ext, info) {
+			if (strcmp(version_ext.name, symname) != 0)
+				continue;
+			if (*version_ext.crc == *crc)
+				return 1;
+			pr_debug("Found checksum %X vs module %X\n",
+				 *crc, *version_ext.crc);
+			goto bad_version;
+		}
+		pr_warn_once("%s: no extended symbol version for %s\n",
+			     info->name, symname);
+		return 1;
+	}
+
 	/* No versions at all?  modprobe --force does this. */
 	if (versindex == 0)
 		return try_to_force_load(mod, symname) == 0;
@@ -87,6 +104,34 @@ int same_magic(const char *amagic, const char *bmagic,
 	return strcmp(amagic, bmagic) == 0;
 }
 
+void modversion_ext_start(const struct load_info *info,
+			  struct modversion_info_ext *start)
+{
+	unsigned int crc_idx = info->index.vers_ext_crc;
+	unsigned int name_idx = info->index.vers_ext_name;
+	Elf_Shdr *sechdrs = info->sechdrs;
+
+	/*
+	 * Both of these fields are needed for this to be useful
+	 * Any future fields should be initialized to NULL if absent.
+	 */
+	if (crc_idx == 0 || name_idx == 0) {
+		start->remaining = 0;
+		return;
+	}
+
+	start->crc = (const s32 *)sechdrs[crc_idx].sh_addr;
+	start->name = (const char *)sechdrs[name_idx].sh_addr;
+	start->remaining = sechdrs[crc_idx].sh_size / sizeof(*start->crc);
+}
+
+void modversion_ext_advance(struct modversion_info_ext *vers)
+{
+	vers->remaining--;
+	vers->crc++;
+	vers->name += strlen(vers->name) + 1;
+}
+
 /*
  * Generate the signature for all relevant module structures here.
  * If these change, we don't want to try to parse the module.

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

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 e6b2427e5c190aacf7b9c5c1bb57fca39d311564..a31c617cd67d3d66b24d2fba34cbd5cc9c53ab78 100644
--- a/kernel/module/Kconfig
+++ b/kernel/module/Kconfig
@@ -208,6 +208,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 44936ebad161e914cbcc40ac74a2d651596d7b07..765da63d592be56fe93c0f4a35f1bfbcb924541a 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -43,6 +43,7 @@ MODPOST = 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 107393a8c48a5993dbe456702fec0652a967ee86..bd38f33fd41fbd98bce34f8924b2fb0ac04297ee 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -32,6 +32,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 */
@@ -1817,6 +1819,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
  **/
@@ -1840,9 +1888,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);
@@ -1972,6 +2025,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);
 	add_moddevtable(&buf, mod);
 	add_srcversion(&buf, mod);
@@ -2130,7 +2184,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;
@@ -2179,6 +2233,9 @@ int main(int argc, char **argv)
 		case 'd':
 			missing_namespace_deps = optarg;
 			break;
+		case 'x':
+			extended_modversions = true;
+			break;
 		default:
 			exit(1);
 		}

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v8 3/3] rust: Use gendwarfksyms + extended modversions for CONFIG_MODVERSIONS

[Matthew Maurer]

From: Sami Tolvanen <samitolvanen@google.com>

Previously, two things stopped Rust from using MODVERSIONS:
1. Rust symbols are occasionally too long to be represented in the
   original versions table
2. Rust types cannot be properly hashed by the existing genksyms
   approach because:
	* Looking up type definitions in Rust is more complex than C
	* Type layout is potentially dependent on the compiler in Rust,
	  not just the source type declaration.

CONFIG_EXTENDED_MODVERSIONS addresses the first point, and
CONFIG_GENDWARFKSYMS the second. If Rust wants to use MODVERSIONS, allow
it to do so by selecting both features.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Co-developed-by: Matthew Maurer <mmaurer@google.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 init/Kconfig  |  3 ++-
 rust/Makefile | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index c521e1421ad4abd80080bce8cf1c68389cb65c69..5e6d9868705ece2b2f6b90d7ce197a4d66240b6b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1946,7 +1946,8 @@ config RUST
 	bool "Rust support"
 	depends on HAVE_RUST
 	depends on RUST_IS_AVAILABLE
-	depends on !MODVERSIONS
+	select EXTENDED_MODVERSIONS if MODVERSIONS
+	depends on !MODVERSIONS || GENDWARFKSYMS
 	depends on !GCC_PLUGIN_RANDSTRUCT
 	depends on !RANDSTRUCT
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
diff --git a/rust/Makefile b/rust/Makefile
index b5e0a73b78f3e58fc8fb8c9fab8fb5792406c6d8..0e98590082a1ac88e6ee29c28ce1b1d19982ac10 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -303,10 +303,11 @@ $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_extra = ;
 $(obj)/bindings/bindings_helpers_generated.rs: $(src)/helpers/helpers.c FORCE
 	$(call if_changed_dep,bindgen)
 
+rust_exports = $(NM) -p --defined-only $(1) | awk '$$2~/(T|R|D|B)/ && $$3!~/__cfi/ { printf $(2),$(3) }'
+
 quiet_cmd_exports = EXPORTS $@
       cmd_exports = \
-	$(NM) -p --defined-only $< \
-		| awk '$$2~/(T|R|D|B)/ && $$3!~/__cfi/ {printf "EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3}' > $@
+	$(call rust_exports,$<,"EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3) > $@
 
 $(obj)/exports_core_generated.h: $(obj)/core.o FORCE
 	$(call if_changed,exports)
@@ -378,11 +379,36 @@ ifneq ($(or $(CONFIG_ARM64),$(and $(CONFIG_RISCV),$(CONFIG_64BIT))),)
 		__ashlti3 __lshrti3
 endif
 
+ifdef CONFIG_MODVERSIONS
+cmd_gendwarfksyms = $(if $(skip_gendwarfksyms),, \
+	$(call rust_exports,$@,"%s\n",$$3) | \
+	scripts/gendwarfksyms/gendwarfksyms \
+		$(if $(KBUILD_GENDWARFKSYMS_STABLE), --stable) \
+		$(if $(KBUILD_SYMTYPES), --symtypes $(@:.o=.symtypes),) \
+		$@ >> $(dot-target).cmd)
+endif
+
 define rule_rustc_library
 	$(call cmd_and_fixdep,rustc_library)
 	$(call cmd,gen_objtooldep)
+	$(call cmd,gendwarfksyms)
 endef
 
+define rule_rust_cc_library
+	$(call if_changed_rule,cc_o_c)
+	$(call cmd,force_checksrc)
+	$(call cmd,gendwarfksyms)
+endef
+
+# helpers.o uses the same export mechanism as Rust libraries, so ensure symbol
+# versions are calculated for the helpers too.
+$(obj)/helpers/helpers.o: $(src)/helpers/helpers.c $(recordmcount_source) FORCE
+	+$(call if_changed_rule,rust_cc_library)
+
+# Disable symbol versioning for exports.o to avoid conflicts with the actual
+# symbol versions generated from Rust objects.
+$(obj)/exports.o: private skip_gendwarfksyms = 1
+
 $(obj)/core.o: private skip_clippy = 1
 $(obj)/core.o: private skip_flags = -Wunreachable_pub
 $(obj)/core.o: private rustc_objcopy = $(foreach sym,$(redirect-intrinsics),--redefine-sym $(sym)=__rust$(sym))
@@ -394,6 +420,7 @@ ifneq ($(or $(CONFIG_X86_64),$(CONFIG_X86_32)),)
 $(obj)/core.o: scripts/target.json
 endif
 
+$(obj)/compiler_builtins.o: private skip_gendwarfksyms = 1
 $(obj)/compiler_builtins.o: private rustc_objcopy = -w -W '__*'
 $(obj)/compiler_builtins.o: $(src)/compiler_builtins.rs $(obj)/core.o FORCE
 	+$(call if_changed_rule,rustc_library)
@@ -404,6 +431,7 @@ $(obj)/alloc.o: private rustc_target_flags = $(alloc-cfgs)
 $(obj)/alloc.o: $(RUST_LIB_SRC)/alloc/src/lib.rs $(obj)/compiler_builtins.o FORCE
 	+$(call if_changed_rule,rustc_library)
 
+$(obj)/build_error.o: private skip_gendwarfksyms = 1
 $(obj)/build_error.o: $(src)/build_error.rs $(obj)/compiler_builtins.o FORCE
 	+$(call if_changed_rule,rustc_library)
 
@@ -413,6 +441,7 @@ $(obj)/bindings.o: $(src)/bindings/lib.rs \
     $(obj)/bindings/bindings_helpers_generated.rs FORCE
 	+$(call if_changed_rule,rustc_library)
 
+$(obj)/uapi.o: private skip_gendwarfksyms = 1
 $(obj)/uapi.o: $(src)/uapi/lib.rs \
     $(obj)/compiler_builtins.o \
     $(obj)/uapi/uapi_generated.rs FORCE

-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Michael Ellerman]

Matthew Maurer <mmaurer@google.com> writes:
> Adds a new format for MODVERSIONS which stores each field in a separate
> ELF section. This initially adds support for variable length names, but
> could later be used to add additional fields to MODVERSIONS in a
> backwards compatible way if needed. Any new fields will be ignored by
> old user tooling, unlike the current format where user tooling cannot
> tolerate adjustments to the format (for example making the name field
> longer).
>
> Since PPC munges its version records to strip leading dots, we reproduce
> the munging for the new format. Other architectures do not appear to
> have architecture-specific usage of this information.
>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  arch/powerpc/kernel/module_64.c | 24 ++++++++++-
  
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Luis Chamberlain]

On Thu, Oct 31, 2024 at 12:22:36PM +1100, Michael Ellerman wrote:
> Matthew Maurer <mmaurer@google.com> writes:
> > Adds a new format for MODVERSIONS which stores each field in a separate
> > ELF section. This initially adds support for variable length names, but
> > could later be used to add additional fields to MODVERSIONS in a
> > backwards compatible way if needed. Any new fields will be ignored by
> > old user tooling, unlike the current format where user tooling cannot
> > tolerate adjustments to the format (for example making the name field
> > longer).
> >
> > Since PPC munges its version records to strip leading dots, we reproduce
> > the munging for the new format. Other architectures do not appear to
> > have architecture-specific usage of this information.
> >
> > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > ---
> >  arch/powerpc/kernel/module_64.c | 24 ++++++++++-
>   
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

Michael, Matthew, why make everyone deal with this instead of just
making this an arch thing and ppc would be the only one doing it?

  Luis

Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Matthew Maurer]

On Wed, Oct 30, 2024 at 9:37 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, Oct 31, 2024 at 12:22:36PM +1100, Michael Ellerman wrote:
> > Matthew Maurer <mmaurer@google.com> writes:
> > > Adds a new format for MODVERSIONS which stores each field in a separate
> > > ELF section. This initially adds support for variable length names, but
> > > could later be used to add additional fields to MODVERSIONS in a
> > > backwards compatible way if needed. Any new fields will be ignored by
> > > old user tooling, unlike the current format where user tooling cannot
> > > tolerate adjustments to the format (for example making the name field
> > > longer).
> > >
> > > Since PPC munges its version records to strip leading dots, we reproduce
> > > the munging for the new format. Other architectures do not appear to
> > > have architecture-specific usage of this information.
> > >
> > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > > ---
> > >  arch/powerpc/kernel/module_64.c | 24 ++++++++++-
> >
> > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
>
> Michael, Matthew, why make everyone deal with this instead of just
> making this an arch thing and ppc would be the only one doing it?
>
>   Luis
>

I'm not sure I understand - the PPC changes are in an arch-specific
directory, and triggered through the arch-implemented callback
mod_frob_arch_sections. What would you like done to make it more of an
arch-thing?

Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Luis Chamberlain]

On Wed, Oct 30, 2024 at 10:06:12PM -0700, Matthew Maurer wrote:
> On Wed, Oct 30, 2024 at 9:37 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Thu, Oct 31, 2024 at 12:22:36PM +1100, Michael Ellerman wrote:
> > > Matthew Maurer <mmaurer@google.com> writes:
> > > > Adds a new format for MODVERSIONS which stores each field in a separate
> > > > ELF section. This initially adds support for variable length names, but
> > > > could later be used to add additional fields to MODVERSIONS in a
> > > > backwards compatible way if needed. Any new fields will be ignored by
> > > > old user tooling, unlike the current format where user tooling cannot
> > > > tolerate adjustments to the format (for example making the name field
> > > > longer).
> > > >
> > > > Since PPC munges its version records to strip leading dots, we reproduce
> > > > the munging for the new format. Other architectures do not appear to
> > > > have architecture-specific usage of this information.
> > > >
> > > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > > > ---
> > > >  arch/powerpc/kernel/module_64.c | 24 ++++++++++-
> > >
> > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> >
> > Michael, Matthew, why make everyone deal with this instead of just
> > making this an arch thing and ppc would be the only one doing it?
> >
> >   Luis
> >
> 
> I'm not sure I understand - the PPC changes are in an arch-specific
> directory, and triggered through the arch-implemented callback
> mod_frob_arch_sections. What would you like done to make it more of an
> arch-thing?

Sorry, yes, I see that now, that's what I get for late night patch
review. Nevermidn, this all looks good to me now.

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Wed, Oct 30, 2024 at 11:05:03PM +0000, Matthew Maurer wrote:
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
> index e6b2427e5c190aacf7b9c5c1bb57fca39d311564..a31c617cd67d3d66b24d2fba34cbd5cc9c53ab78 100644
> --- a/kernel/module/Kconfig
> +++ b/kernel/module/Kconfig
> @@ -208,6 +208,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.
> +

The question is, if only extended moversions are used, what new tooling
requirements are there? Can you test using only extended modversions?

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

> The question is, if only extended moversions are used, what new tooling
> requirements are there? Can you test using only extended modversions?
>
>   Luis

I'm not sure precisely what you're asking for. Do you want:
1. A kconfig that suppresses the emission of today's MODVERSIONS
format? This would be fairly easy to do, but I was leaving it enabled
for compatibility's sake, at least until extended modversions become
more common. This way existing `kmod` tools and kernels would continue
to be able to load new-style modules.
2. libkmod support for parsing the new format? I can do that fairly
easily too, but wanted the format actually decided on and accepted
before I started modifying things that read modversions.
3. Something else? Maybe I'm not understanding your comment?

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote:
> > The question is, if only extended moversions are used, what new tooling
> > requirements are there? Can you test using only extended modversions?
> >
> >   Luis
> 
> I'm not sure precisely what you're asking for. Do you want:
> 1. A kconfig that suppresses the emission of today's MODVERSIONS
> format? 

Yes that's right, a brave new world, and with the warning of that.

> This would be fairly easy to do, but I was leaving it enabled
> for compatibility's sake, at least until extended modversions become
> more common. This way existing `kmod` tools and kernels would continue
> to be able to load new-style modules.

Sure, understood why we'd have both.

> 2. libkmod support for parsing the new format? I can do that fairly
> easily too, but wanted the format actually decided on and accepted
> before I started modifying things that read modversions.

This is implied, what I'd like is for an A vs B comparison to be able to
be done on even without rust modules, so that we can see if really
libkmod changes are all that's needed. Does boot fail without a new
libkmod for this? If so the Kconfig should specificy that for this new
brave new world.

If a distribution can leverage just one format, why would they not
consider it if they can ensure the proper tooling is in place. We
haven't itemized the differences in practice and this could help
with this. One clear difference so far is the kabi stuff, but that's
just evaluating one way of doing things so far, I suspect we'll get
more review on that from Petr soon.

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote:
> > > The question is, if only extended moversions are used, what new tooling
> > > requirements are there? Can you test using only extended modversions?
> > >
> > >   Luis
> >
> > I'm not sure precisely what you're asking for. Do you want:
> > 1. A kconfig that suppresses the emission of today's MODVERSIONS
> > format?
>
> Yes that's right, a brave new world, and with the warning of that.

OK, I can send another revision with a suppression config, perhaps
CONFIG_NO_BASIC_MODVERSIONS

>
>
> > This would be fairly easy to do, but I was leaving it enabled
> > for compatibility's sake, at least until extended modversions become
> > more common. This way existing `kmod` tools and kernels would continue
> > to be able to load new-style modules.
>
> Sure, understood why we'd have both.
>
> > 2. libkmod support for parsing the new format? I can do that fairly
> > easily too, but wanted the format actually decided on and accepted
> > before I started modifying things that read modversions.
>
> This is implied, what I'd like is for an A vs B comparison to be able to
> be done on even without rust modules, so that we can see if really
> libkmod changes are all that's needed. Does boot fail without a new
> libkmod for this? If so the Kconfig should specificy that for this new
> brave new world.

libkmod changes are not needed for boot - the userspace tools do not
examine this data for anything inline with boot at the moment, libkmod
only looks at it for kmod_module_get_versions, and modprobe only looks
at that with --show-modversions or --dump-modversions, which are not
normally part of boot.

With the code as is, the only change will be that if a module with
EXTENDED_MODVERSIONS set contains an over-length symbol (which
wouldn't have been possible before), the overlong symbol's modversion
data will not appear in --show-modversions. After patching `libkmod`
in a follow-up patch, long symbols would appear as well. If booted
against an old kernel, long symbols will not have their CRCs in the
list to be checked. However, the old kernel could not export these
symbols, so it will fail to resolve the symbol and fail the load
regardless.

If we add and enable NO_BASIC_MODVERSIONS like you suggested above,
today's --show-modversions will claim there is no modversions data.
Applying a libkmod patch will result in modversions info being
displayed by that command again. If booted against a new kernel,
everything will be fine. If booted against an old kernel, it will
behave as though there is no modversions information.

>
>
> If a distribution can leverage just one format, why would they not
> consider it if they can ensure the proper tooling is in place. We
> haven't itemized the differences in practice and this could help
> with this. One clear difference so far is the kabi stuff, but that's

The kabi stuff is at least partially decoupled - you can (and it
sounds like from the responses to Sami's change, occasionally might
want to) enable debug symbol based modversions even without extended
modversions. You can also enable extended modversions without the
debug symbol based modversions, though there are less clear use-cases
for that.

> just evaluating one way of doing things so far, I suspect we'll get
> more review on that from Petr soon.
>
>   Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Tue, Nov 05, 2024 at 04:26:51PM -0800, Matthew Maurer wrote:
> On Fri, Nov 1, 2024 at 2:10 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Thu, Oct 31, 2024 at 01:00:28PM -0700, Matthew Maurer wrote:
> > > > The question is, if only extended moversions are used, what new tooling
> > > > requirements are there? Can you test using only extended modversions?
> > > >
> > > >   Luis
> > >
> > > I'm not sure precisely what you're asking for. Do you want:
> > > 1. A kconfig that suppresses the emission of today's MODVERSIONS
> > > format?
> >
> > Yes that's right, a brave new world, and with the warning of that.
> 
> OK, I can send another revision with a suppression config, perhaps
> CONFIG_NO_BASIC_MODVERSIONS

Great.

> > > This would be fairly easy to do, but I was leaving it enabled
> > > for compatibility's sake, at least until extended modversions become
> > > more common. This way existing `kmod` tools and kernels would continue
> > > to be able to load new-style modules.
> >
> > Sure, understood why we'd have both.
> >
> > > 2. libkmod support for parsing the new format? I can do that fairly
> > > easily too, but wanted the format actually decided on and accepted
> > > before I started modifying things that read modversions.
> >
> > This is implied, what I'd like is for an A vs B comparison to be able to
> > be done on even without rust modules, so that we can see if really
> > libkmod changes are all that's needed. Does boot fail without a new
> > libkmod for this? If so the Kconfig should specificy that for this new
> > brave new world.
> 
> libkmod changes are not needed for boot - the userspace tools do not
> examine this data for anything inline with boot at the moment, libkmod
> only looks at it for kmod_module_get_versions, and modprobe only looks
> at that with --show-modversions or --dump-modversions, which are not
> normally part of boot.
> 
> With the code as is, the only change will be that if a module with
> EXTENDED_MODVERSIONS set contains an over-length symbol (which
> wouldn't have been possible before), the overlong symbol's modversion
> data will not appear in --show-modversions. After patching `libkmod`
> in a follow-up patch, long symbols would appear as well. If booted
> against an old kernel, long symbols will not have their CRCs in the
> list to be checked. However, the old kernel could not export these
> symbols, so it will fail to resolve the symbol and fail the load
> regardless.

Thanks for checking all this. It is exactly what I was looking for.
All this should be part of the cover letter and Kconfig documentation.

> If we add and enable NO_BASIC_MODVERSIONS like you suggested above,
> today's --show-modversions will claim there is no modversions data.
> Applying a libkmod patch will result in modversions info being
> displayed by that command again. If booted against a new kernel,
> everything will be fine.

*This* is is the sort of information I was also looking for and I think
it would be good to make it clear for the upcoming NO_BASIC_MODVERSIONS.

> If booted against an old kernel, it will
> behave as though there is no modversions information.

Huh? This I don't get. If you have the new libkmod and boot
an old kernel, that should just not break becauase well, long
symbols were not ever supported properly anyway, so no regression.

I'm not quite sure I understood your last comment here though,
can you clarify what you meant?

Anyway, so now that this is all cleared up, the next question I have
is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
userspace requirements aren't large at all, what actual benefits does
using this new extended mod versions have? Why wouldn't a distro end
up preferring this for say a future release for all modules?

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

>
> > If booted against an old kernel, it will
> > behave as though there is no modversions information.
>
> Huh? This I don't get. If you have the new libkmod and boot
> an old kernel, that should just not break becauase well, long
> symbols were not ever supported properly anyway, so no regression.

Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and
then load said module with a kernel *before* EXTENDED_MODVERSIONS
existed, it will see no modversion info on the module to check. This
will be true regardless of symbol length.

>
> I'm not quite sure I understood your last comment here though,
> can you clarify what you meant?
>
> Anyway, so now that this is all cleared up, the next question I have
> is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
> userspace requirements aren't large at all, what actual benefits does
> using this new extended mod versions have? Why wouldn't a distro end
> up preferring this for say a future release for all modules?

I think a distro will end up preferring using this for all modules,
but was intending to put both in for a transitional period until the
new format was more accepted.

>
>   Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Lucas De Marchi]

On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote:
>>
>> > If booted against an old kernel, it will
>> > behave as though there is no modversions information.
>>
>> Huh? This I don't get. If you have the new libkmod and boot
>> an old kernel, that should just not break becauase well, long
>> symbols were not ever supported properly anyway, so no regression.
>
>Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and

how are you setting NO_BASIC_MODVERSIONS and loading it in a kernel
that still doesn't have that, i.e. before EXTENDED_MODVERSIONS?

Please Cc me on the format change and if possible submit the libkmod
support.

thanks
Lucas De Marchi

>then load said module with a kernel *before* EXTENDED_MODVERSIONS
>existed, it will see no modversion info on the module to check. This
>will be true regardless of symbol length.
>
>>
>> I'm not quite sure I understood your last comment here though,
>> can you clarify what you meant?
>>
>> Anyway, so now that this is all cleared up, the next question I have
>> is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
>> userspace requirements aren't large at all, what actual benefits does
>> using this new extended mod versions have? Why wouldn't a distro end
>> up preferring this for say a future release for all modules?
>
>I think a distro will end up preferring using this for all modules,
>but was intending to put both in for a transitional period until the
>new format was more accepted.
>
>>
>>   Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

On Wed, Nov 6, 2024 at 10:27 PM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote:
> >>
> >> > If booted against an old kernel, it will
> >> > behave as though there is no modversions information.
> >>
> >> Huh? This I don't get. If you have the new libkmod and boot
> >> an old kernel, that should just not break becauase well, long
> >> symbols were not ever supported properly anyway, so no regression.
> >
> >Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and
>
> how are you setting NO_BASIC_MODVERSIONS and loading it in a kernel
> that still doesn't have that, i.e. before EXTENDED_MODVERSIONS?

That action would involve e.g. building a module against a 6.13 series
kernel with NO_BASIC_MODVERSIONS and trying insmod it on a 6.12 series
kernel. I know it's not supported, I was just trying to describe the
full matrix of what would happen differently with the proposed
additional config flag.

>
> Please Cc me on the format change and if possible submit the libkmod
> support.

It seems awkward to adjust kmod to support a format that still hasn't
been accepted to the kernel. I can send kmod patches to support it,
but since this patch series hasn't been accepted yet, it seemed a bit
premature.

I'll explicitly add you to the format change (patch before this in the
series) and add you to the whole series in v9

>
> thanks
> Lucas De Marchi
>
> >then load said module with a kernel *before* EXTENDED_MODVERSIONS
> >existed, it will see no modversion info on the module to check. This
> >will be true regardless of symbol length.
> >
> >>
> >> I'm not quite sure I understood your last comment here though,
> >> can you clarify what you meant?
> >>
> >> Anyway, so now that this is all cleared up, the next question I have
> >> is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
> >> userspace requirements aren't large at all, what actual benefits does
> >> using this new extended mod versions have? Why wouldn't a distro end
> >> up preferring this for say a future release for all modules?
> >
> >I think a distro will end up preferring using this for all modules,
> >but was intending to put both in for a transitional period until the
> >new format was more accepted.
> >
> >>
> >>   Luis

Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info

[Matthew Maurer]

Adding Lucas DeMarchi to the thread after voicing an interest in the
modpost patch.

On Thu, Oct 31, 2024 at 12:49 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Oct 30, 2024 at 10:06:12PM -0700, Matthew Maurer wrote:
> > On Wed, Oct 30, 2024 at 9:37 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > On Thu, Oct 31, 2024 at 12:22:36PM +1100, Michael Ellerman wrote:
> > > > Matthew Maurer <mmaurer@google.com> writes:
> > > > > Adds a new format for MODVERSIONS which stores each field in a separate
> > > > > ELF section. This initially adds support for variable length names, but
> > > > > could later be used to add additional fields to MODVERSIONS in a
> > > > > backwards compatible way if needed. Any new fields will be ignored by
> > > > > old user tooling, unlike the current format where user tooling cannot
> > > > > tolerate adjustments to the format (for example making the name field
> > > > > longer).
> > > > >
> > > > > Since PPC munges its version records to strip leading dots, we reproduce
> > > > > the munging for the new format. Other architectures do not appear to
> > > > > have architecture-specific usage of this information.
> > > > >
> > > > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > > > > ---
> > > > >  arch/powerpc/kernel/module_64.c | 24 ++++++++++-
> > > >
> > > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> > >
> > > Michael, Matthew, why make everyone deal with this instead of just
> > > making this an arch thing and ppc would be the only one doing it?
> > >
> > >   Luis
> > >
> >
> > I'm not sure I understand - the PPC changes are in an arch-specific
> > directory, and triggered through the arch-implemented callback
> > mod_frob_arch_sections. What would you like done to make it more of an
> > arch-thing?
>
> Sorry, yes, I see that now, that's what I get for late night patch
> review. Nevermidn, this all looks good to me now.
>
>   Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote:
> >
> > > If booted against an old kernel, it will
> > > behave as though there is no modversions information.
> >
> > Huh? This I don't get. If you have the new libkmod and boot
> > an old kernel, that should just not break becauase well, long
> > symbols were not ever supported properly anyway, so no regression.
> 
> Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and
> then load said module with a kernel *before* EXTENDED_MODVERSIONS
> existed, it will see no modversion info on the module to check. This
> will be true regardless of symbol length.

Isn't that just the same as disabling modverisons?

If you select modversions, you get the options to choose:

  - old modversions
  - old modversions + extended modversions
  - extended modversions only

> > I'm not quite sure I understood your last comment here though,
> > can you clarify what you meant?
> >
> > Anyway, so now that this is all cleared up, the next question I have
> > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
> > userspace requirements aren't large at all, what actual benefits does
> > using this new extended mod versions have? Why wouldn't a distro end
> > up preferring this for say a future release for all modules?
> 
> I think a distro will end up preferring using this for all modules,
> but was intending to put both in for a transitional period until the
> new format was more accepted.

The only thing left I think to test is the impact at runtime, and the
only thing I can think of is first we use find_symbol() on resolve_symbol() 
which it took me a while to review and realize that this just uses a
completely different ELF section, the the ksymtab sections which are split up
between the old and the gpl section. But after that we use check_version().
I suspect the major overhead here is in find_symbol() and that's in no way shape
or form affected by your changes, and I also suspect that since the
way you implemented for_each_modversion_info_ext() is just *one* search
there shouldn't be any penalty here at all. Given it took *me* a while
to review all this, I think it would be good for you to also expand your
cover letter to be crystal clear on these expectations to users and
developers and if anything expand on the Kconfig / and add documentation
if we don't document any of this.

I'd still like to see you guys test all this with the new TEST_KALLSYMS.

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Thu, Nov 07, 2024 at 02:38:13PM -0800, Luis Chamberlain wrote:
> The only thing left I think to test is the impact at runtime, and the
> only thing I can think of is first we use find_symbol() on resolve_symbol() 
> which it took me a while to review and realize that this just uses a
> completely different ELF section, the the ksymtab sections which are split up
> between the old and the gpl section.

Thinking about this some more, if we're going down enabling a new
option, it seems to beg the question if the old *two* ksymtab sections
could just be folded into the a new one where the "gpl only" thing
becomes just one "column" as you call it. Reasons I ask, it seems like
we're duplicating symbol names on ksymtab and for modeversions. Could
you review this a bit?

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

> Thinking about this some more, if we're going down enabling a new
> option, it seems to beg the question if the old *two* ksymtab sections
> could just be folded into the a new one where the "gpl only" thing
> becomes just one "column" as you call it. Reasons I ask, it seems like
> we're duplicating symbol names on ksymtab and for modeversions. Could
> you review this a bit?

 Short answer: We could do this, but I don't necessarily think it's a good idea.

ksymtab and modversions aren't duplicating names even with this patch
series - We have two different formats, one for importing symbols, and
one for exporting them. `__ksymtab`, `__ksymtab_gpl`, and
`__ksymtab_strings` are used to export symbols. `__versions` or the
new `__version_ext_names` and `__version_ext_crcs` are used to import
them. For this reason, in any given compilation unit, a string should
only appear either in the ksymtab (providing it), or in versions
(consuming it).

There also isn't as much immediate technical need for that kind of
rework of the ksymtab format - ksymtab uses a string table for their
names, so the "long name support" that extended modversions provides
to modversions is already present in ksymtab.

Combined, this means that there would be few technical benefits to
this - the primary potential benefit I could see to something like
this would be code complexity reduction, which is a bit of a matter of
personal taste, and mine might not match others'.

However, we could do some things similar to what's going on here:
A. We could try to unify versions and ksymtab (this seems most viable,
but the change in meaning of this data structure has me wary)
B. We could make ksymtab use columnar storage for more things - it
already does so for CRCs, we could theoretically make any or all of
licensing, namespaces, or symbol values columnar.

With the caveat that I am not convinced this restructuring is worth
the churn, the way I would do A would be:

1. Add a field to the `kernel_symbol` that indicates whether the
symbol is import/export (or possibly re-use `value` with a 0 value
after linker resolution to mean "import" instead of export).
2. Generate `kernel_symbol` entries for imported symbols, not just
exported ones.
3. Read `kcrctab` for import symbols to figure out what the expected
crc value is when importing, rather than using versions.
4. Stop generating/reading any of `__versions`, `__version_ext_names`,
`__versions_ext_crcs`, etc.

There are two downsides I can see to this:
1. You cannot make this backwards compatible with existing `kmod`.
(This was the argument given against just enlarging MODVERSIONS symbol
names.)
2. It's hard to be certain that we know about all users of `ksymtab`
in order to ensure they all know the new convention around imported vs
exported symbols.

I think that B would actually make things worse because symbols always
today always have a value, a namespace, a name, and a license. The
only thing that's optional is the CRC, and that's already columnar.
Making the other ones columnar would hurt locality. We'd still need
the strtab sections, or we'd end up with many copies of each
namespace, where today that should get deduped down by the linker.
Columns are good for things that are extensions, optional, or variable
length.

If there are other reasons *for* doing this that I'm not aware of,
what I'd do would be:
1. Use the name as the primary index, same as modversions.
2. Split each other piece into its own column, with a joint iterator.
3. Convert license into a column, with an enum value (currently only
fully exported or GPL).
4. Replace places in the coe where a `struct kernel_symbol *` is used
today with an iterator over the joint columns.

Again, to reiterate, I *do not* think that B is a good idea. A might
be, but the improvement seems sufficiently marginal to me that I don't
know if it's worth the churn.

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Luis Chamberlain]

On Mon, Nov 18, 2024 at 04:09:34PM -0800, Matthew Maurer wrote:
> > Thinking about this some more, if we're going down enabling a new
> > option, it seems to beg the question if the old *two* ksymtab sections
> > could just be folded into the a new one where the "gpl only" thing
> > becomes just one "column" as you call it. Reasons I ask, it seems like
> > we're duplicating symbol names on ksymtab and for modeversions. Could
> > you review this a bit?
> 
>  Short answer: We could do this, but I don't necessarily think it's a good idea.

Thanks for your review on this. I agree the complexities you outline
don't yet justify the churn.

  Luis

Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Thu, Nov 7, 2024 at 2:38 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote:
> > >
> > > > If booted against an old kernel, it will
> > > > behave as though there is no modversions information.
> > >
> > > Huh? This I don't get. If you have the new libkmod and boot
> > > an old kernel, that should just not break becauase well, long
> > > symbols were not ever supported properly anyway, so no regression.
> >
> > Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and
> > then load said module with a kernel *before* EXTENDED_MODVERSIONS
> > existed, it will see no modversion info on the module to check. This
> > will be true regardless of symbol length.
>
> Isn't that just the same as disabling modverisons?
>
> If you select modversions, you get the options to choose:
>
>   - old modversions
>   - old modversions + extended modversions
>   - extended modversions only

Yes, what I'm pointing out is that kernels before the introduction of
extended modversions will not know how to read extended modversions,
and so they will treat modules with *only* extended modversions as
though they have no modversions.

>
> > > I'm not quite sure I understood your last comment here though,
> > > can you clarify what you meant?
> > >
> > > Anyway, so now that this is all cleared up, the next question I have
> > > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
> > > userspace requirements aren't large at all, what actual benefits does
> > > using this new extended mod versions have? Why wouldn't a distro end
> > > up preferring this for say a future release for all modules?
> >
> > I think a distro will end up preferring using this for all modules,
> > but was intending to put both in for a transitional period until the
> > new format was more accepted.
>
> The only thing left I think to test is the impact at runtime, and the
> only thing I can think of is first we use find_symbol() on resolve_symbol()
> which it took me a while to review and realize that this just uses a
> completely different ELF section, the the ksymtab sections which are split up
> between the old and the gpl section. But after that we use check_version().
> I suspect the major overhead here is in find_symbol() and that's in no way shape
> or form affected by your changes, and I also suspect that since the
> way you implemented for_each_modversion_info_ext() is just *one* search
> there shouldn't be any penalty here at all. Given it took *me* a while
> to review all this, I think it would be good for you to also expand your
> cover letter to be crystal clear on these expectations to users and
> developers and if anything expand on the Kconfig / and add documentation
> if we don't document any of this.

I can add a commit extending modules.rst, but it's not clear to me
what piece was surprising here - the existing MODVERSIONS format is
*also* in a separate section. Nothing written in the "Module
Versioning" section has been invalidated that I can see.

Things I could think to add:

* Summary of the internal data format (seems odd, since the previous
one isn't here, and I'd think that an implementation detail anyways)
* A warning about the effects of NO_BASIC_MODVERSIONS (probably better
in Kconfig, isn't in the current changeset because the flag isn't
there)

>
> I'd still like to see you guys test all this with the new TEST_KALLSYMS.

I've attached the results of running TEST_KALLSYMS - it appears to be
irrelevant to performance, as you expected.

>
>   Luis

[-- Attachment #2: extended-log --]
[-- Type: application/octet-stream, Size: 1306 bytes --]

TAP version 13
1..1
# timeout set to 45
# selftests: module: find_symbol.sh
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           28898032 ns   duration_time
#            2366000 ns   user_time
#                207      page-faults
#
#        0.020085991 seconds time elapsed
#
#        0.002366000 seconds user
#        0.000000000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25333640 ns   duration_time
#              32000 ns   user_time
#            2357000 ns   system_time
#                207      page-faults
#
#        0.024083957 seconds time elapsed
#
#        0.000032000 seconds user
#        0.002357000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25398600 ns   duration_time
#              30000 ns   user_time
#            2269000 ns   system_time
#                208      page-faults
#
#        0.024095204 seconds time elapsed
#
#        0.000030000 seconds user
#        0.002269000 seconds sys
#
#
ok 1 selftests: module: find_symbol.sh
/selftests # zcat /proc/config.gz | grep EXTENDED
# CONFIG_X86_EXTENDED_PLATFORM is not set
CONFIG_EXTENDED_MODVERSIONS=y
# CONFIG_NETCONSOLE_EXTENDED_LOG is not set
CONFIG_SERIAL_8250_EXTENDED=y
/selftests #

[-- Attachment #3: noextend-log --]
[-- Type: application/octet-stream, Size: 1316 bytes --]

TAP version 13
1..1
# timeout set to 45
# selftests: module: find_symbol.sh
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           30477348 ns   duration_time
#            2494000 ns   user_time
#                206      page-faults
#
#        0.024078527 seconds time elapsed
#
#        0.002494000 seconds user
#        0.000000000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25264385 ns   duration_time
#              34000 ns   user_time
#            2351000 ns   system_time
#                207      page-faults
#
#        0.024081980 seconds time elapsed
#
#        0.000034000 seconds user
#        0.002351000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25300644 ns   duration_time
#              33000 ns   user_time
#            2307000 ns   system_time
#                207      page-faults
#
#        0.024109409 seconds time elapsed
#
#        0.000033000 seconds user
#        0.002307000 seconds sys
#
#
ok 1 selftests: module: find_symbol.sh
/selftests # zcat /proc/config.gz | grep EXTEND
# CONFIG_X86_EXTENDED_PLATFORM is not set
# CONFIG_EXTENDED_MODVERSIONS is not set
# CONFIG_NETCONSOLE_EXTENDED_LOG is not set
CONFIG_SERIAL_8250_EXTENDED=y
/selftests #