[PATCH 0/3] MODVERSIONS + RUST Redux

[PATCH 0/3] MODVERSIONS + RUST Redux

[Matthew Maurer]

Support both MODVERSIONS and RUST by making symbol version information extensible. This works by having a separate section per field, and allowing iteration to work differently per field. The old module information remains available to allow existing kmod tools to continue to work on new modules if they are only looking for C information.

This also gives us the ability to attach additional information to symbol versioning (e.g. namespacing) should it become necessary in the future without breaking backwards compatibility - new sections will be added, but existing tools will still find all their information and keep the existing format.

PPC has a hack around removing leading dots in symvers. I have
replicated this hack for the new format, but without context for why
this was being done, I can't test that it works as expected.

This was inspired by Masahiro Yamada's suggestion[1] when discussing the previous attempt.
Link: https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=E7bTf+MvgJ+NXT3aZNwg@mail.gmail.com/

Matthew Maurer (3):
  export_report.pl: Rehabilitate script
  modpost: Extended modversion support
  export_report: Use new version info format

 arch/powerpc/kernel/module_64.c | 24 +++++++++-
 init/Kconfig                    |  1 -
 kernel/module/internal.h        | 16 ++++++-
 kernel/module/main.c            |  9 +++-
 kernel/module/version.c         | 77 +++++++++++++++++++++++++++++++++
 scripts/export_report.pl        | 17 +++++---
 scripts/mod/modpost.c           | 33 ++++++++++++--
 7 files changed, 161 insertions(+), 16 deletions(-)

-- 
2.43.0.rc0.421.g78406f8d94-goog

[PATCH 2/3] modpost: Extended modversion support

[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.

Adding support for variable length names makes it possible to enable
MODVERSIONS and RUST at the same time.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 arch/powerpc/kernel/module_64.c | 24 +++++++++-
 init/Kconfig                    |  1 -
 kernel/module/internal.h        | 16 ++++++-
 kernel/module/main.c            |  9 +++-
 kernel/module/version.c         | 77 +++++++++++++++++++++++++++++++++
 scripts/mod/modpost.c           | 33 ++++++++++++--
 6 files changed, 151 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 7112adc597a8..2582353a2048 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,
 		}
 }
 
+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++) {
+		if (last == '\0')
+			/* Skip all leading dots */
+			if (str_seq[in] == '.')
+				continue;
+		last = str_seq[in];
+		str_seq[out++] = last;
+	}
+	/* Zero the trailing portion of the names table for robustness */
+	bzero(&str_seq[out], 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/init/Kconfig b/init/Kconfig
index 9ffb103fc927..6cac5b4db8f6 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1885,7 +1885,6 @@ config RUST
 	bool "Rust support"
 	depends on HAVE_RUST
 	depends on RUST_IS_AVAILABLE
-	depends on !MODVERSIONS
 	depends on !GCC_PLUGINS
 	depends on !RANDSTRUCT
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c8b7b4dcf782..0c188c96a045 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -80,7 +80,7 @@ struct load_info {
 	unsigned int used_pages;
 #endif
 	struct {
-		unsigned int sym, str, mod, vers, info, pcpu;
+		unsigned int sym, str, mod, vers, info, pcpu, vers_ext_crc, vers_ext_name;
 	} index;
 };
 
@@ -384,6 +384,20 @@ 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_s32 {
+	const s32 *value;
+	const s32 *end;
+};
+struct modversion_info_ext_string {
+	const char *value;
+	const char *end;
+};
+struct modversion_info_ext {
+	struct modversion_info_ext_s32 crc;
+	struct modversion_info_ext_string name;
+};
+ssize_t modversion_ext_start(const struct load_info *info, struct modversion_info_ext *ver);
+int modversion_ext_advance(struct modversion_info_ext *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 98fedfdb8db5..e69b2ae46161 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1886,10 +1886,15 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
 	if (!info->name)
 		info->name = info->mod->name;
 
-	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
+	if (flags & MODULE_INIT_IGNORE_MODVERSIONS) {
 		info->index.vers = 0; /* Pretend no __versions section! */
-	else
+		info->index.vers_ext_crc = 0;
+		info->index.vers_ext_name = 0;
+	} else {
 		info->index.vers = find_sec(info, "__versions");
+		info->index.vers_ext_crc = find_sec(info, "__version_ext_crcs");
+		info->index.vers_ext_name = find_sec(info, "__version_ext_names");
+	}
 
 	info->index.pcpu = find_pcpusec(info);
 
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 53f43ac5a73e..93d97dad8c77 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 (modversion_ext_start(info, &version_ext) >= 0) {
+		do {
+			if (strncmp(version_ext.name.value, symname,
+				    version_ext.name.end - version_ext.name.value) != 0)
+				continue;
+
+			if (*version_ext.crc.value == *crc)
+				return 1;
+			pr_debug("Found checksum %X vs module %X\n",
+				 *crc, *version_ext.crc.value);
+			goto bad_version;
+		} while (modversion_ext_advance(&version_ext) == 0);
+		goto broken_toolchain;
+	}
+
 	/* No versions at all?  modprobe --force does this. */
 	if (versindex == 0)
 		return try_to_force_load(mod, symname) == 0;
@@ -46,6 +63,7 @@ int check_version(const struct load_info *info,
 		goto bad_version;
 	}
 
+broken_toolchain:
 	/* Broken toolchain. Warn once, then let it go.. */
 	pr_warn_once("%s: no symbol version for %s\n", info->name, symname);
 	return 1;
@@ -87,6 +105,65 @@ int same_magic(const char *amagic, const char *bmagic,
 	return strcmp(amagic, bmagic) == 0;
 }
 
+#define MODVERSION_FIELD_START(sec, field) \
+	field.value = (typeof(field.value))sec.sh_addr; \
+	field.end = field.value + sec.sh_size
+
+ssize_t 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))
+		return -EINVAL;
+
+	MODVERSION_FIELD_START(sechdrs[crc_idx], start->crc);
+	MODVERSION_FIELD_START(sechdrs[name_idx], start->name);
+
+	return (start->crc.end - start->crc.value) / sizeof(*start->crc.value);
+}
+
+static int modversion_ext_s32_advance(struct modversion_info_ext_s32 *field)
+{
+	if (!field->value)
+		return 0;
+	if (field->value >= field->end)
+		return -EINVAL;
+	field->value++;
+	return 0;
+}
+
+static int modversion_ext_string_advance(struct modversion_info_ext_string *s)
+{
+	if (!s->value)
+		return 0;
+	if (s->value >= s->end)
+		return -EINVAL;
+	s->value += strnlen(s->value, s->end - s->value - 1) + 1;
+	if (s->value >= s->end)
+		return -EINVAL;
+	return 0;
+}
+
+int modversion_ext_advance(struct modversion_info_ext *start)
+{
+	int ret;
+
+	ret = modversion_ext_s32_advance(&start->crc);
+	if (ret < 0)
+		return ret;
+
+	ret = modversion_ext_string_advance(&start->name);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 /*
  * Generate the signature for all relevant module structures here.
  * If these change, we don't want to try to parse the module.
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 973b5e5ae2dd..884860c2e833 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1910,15 +1910,42 @@ 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;
+			/* this symbol will only be in the extended info */
+			continue;
 		}
 		buf_printf(b, "\t{ %#8x, \"%s\" },\n",
 			   s->crc, s->name);
 	}
 
 	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.
+			continue;
+		}
+		buf_printf(b, "\t\"%s\\0\"\n", s->name);
+	}
+	buf_printf(b, ";\n");
 }
 
 static void add_depends(struct buffer *b, struct module *mod)
-- 
2.43.0.rc0.421.g78406f8d94-goog

Re: [PATCH 0/3] MODVERSIONS + RUST Redux

[Trevor Gross]

On Wed, Nov 15, 2023 at 1:59 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> Support both MODVERSIONS and RUST by making symbol version information extensible. This works by having a separate section per field, and allowing iteration to work differently per field. The old module information remains available to allow existing kmod tools to continue to work on new modules if they are only looking for C information.
>
> This also gives us the ability to attach additional information to symbol versioning (e.g. namespacing) should it become necessary in the future without breaking backwards compatibility - new sections will be added, but existing tools will still find all their information and keep the existing format.
>
> PPC has a hack around removing leading dots in symvers. I have
> replicated this hack for the new format, but without context for why
> this was being done, I can't test that it works as expected.
>
> This was inspired by Masahiro Yamada's suggestion[1] when discussing the previous attempt.
> Link: https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=E7bTf+MvgJ+NXT3aZNwg@mail.gmail.com/
>
> Matthew Maurer (3):
>   export_report.pl: Rehabilitate script
>   modpost: Extended modversion support
>   export_report: Use new version info format
>
>  arch/powerpc/kernel/module_64.c | 24 +++++++++-
>  init/Kconfig                    |  1 -
>  kernel/module/internal.h        | 16 ++++++-
>  kernel/module/main.c            |  9 +++-
>  kernel/module/version.c         | 77 +++++++++++++++++++++++++++++++++
>  scripts/export_report.pl        | 17 +++++---
>  scripts/mod/modpost.c           | 33 ++++++++++++--
>  7 files changed, 161 insertions(+), 16 deletions(-)
>
> --
> 2.43.0.rc0.421.g78406f8d94-goog

Hi Matthew,

You may want to resend this patch. For whatever reason, the RFL list
only received 0/3 and 2/3 [1], and it looks like the other relevant
lists only received 2/3 [2].

Also seems like text wrapping is off for 0/3 :)

- Trevor

[1]: https://lore.kernel.org/rust-for-linux/20231115185858.2110875-1-mmaurer@google.com/T/#t
[2]: https://lore.kernel.org/linux-modules/20231115185858.2110875-3-mmaurer@google.com/T/#u

Re: [PATCH 0/3] MODVERSIONS + RUST Redux

[Matthew Maurer]

Should I be sending all these patches to all the lists? I was trying
to follow the recommendations of get_maintainers.pl, which thought
modules were the only relevant folks for the export_modules patches.
(I also added Gary + Masahiro explicitly due to their involvement with
the previous approach.)

If I should be sending all the patches to all the lists, should I also
be adding all the suggested To/Cc lines for all the patches, or? I
didn't see any suggestions on how to approach this in
https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch

Sorry for messing up the line wrapping in the cover letter, and if I
re-send I'll fix that, but it doesn't seem worth resending until I
know how I should be setting the To/Cc lines for each patch.


On Wed, Nov 15, 2023 at 1:23 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> Should I be sending all these patches to all the lists? I was trying to follow the recommendations of get_maintainers.pl, which thought modules were the only relevant folks for the export_modules patches. (I also added Gary + Masahiro explicitly due to their involvement with the previous approach.)
>
> If I should be sending all the patches to all the lists, should I also be adding all the suggested To/Cc lines for all the patches, or? I didn't see any suggestions on how to approach this in https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch
>
> Sorry for messing up the line wrapping in the cover letter, and if I re-send I'll fix that, but it doesn't seem worth resending until I know how I should be setting the To/Cc lines for each patch.
>
> On Wed, Nov 15, 2023 at 1:05 PM Trevor Gross <tmgross@umich.edu> wrote:
>>
>> On Wed, Nov 15, 2023 at 1:59 PM Matthew Maurer <mmaurer@google.com> wrote:
>> >
>> > Support both MODVERSIONS and RUST by making symbol version information extensible. This works by having a separate section per field, and allowing iteration to work differently per field. The old module information remains available to allow existing kmod tools to continue to work on new modules if they are only looking for C information.
>> >
>> > This also gives us the ability to attach additional information to symbol versioning (e.g. namespacing) should it become necessary in the future without breaking backwards compatibility - new sections will be added, but existing tools will still find all their information and keep the existing format.
>> >
>> > PPC has a hack around removing leading dots in symvers. I have
>> > replicated this hack for the new format, but without context for why
>> > this was being done, I can't test that it works as expected.
>> >
>> > This was inspired by Masahiro Yamada's suggestion[1] when discussing the previous attempt.
>> > Link: https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=E7bTf+MvgJ+NXT3aZNwg@mail.gmail.com/
>> >
>> > Matthew Maurer (3):
>> >   export_report.pl: Rehabilitate script
>> >   modpost: Extended modversion support
>> >   export_report: Use new version info format
>> >
>> >  arch/powerpc/kernel/module_64.c | 24 +++++++++-
>> >  init/Kconfig                    |  1 -
>> >  kernel/module/internal.h        | 16 ++++++-
>> >  kernel/module/main.c            |  9 +++-
>> >  kernel/module/version.c         | 77 +++++++++++++++++++++++++++++++++
>> >  scripts/export_report.pl        | 17 +++++---
>> >  scripts/mod/modpost.c           | 33 ++++++++++++--
>> >  7 files changed, 161 insertions(+), 16 deletions(-)
>> >
>> > --
>> > 2.43.0.rc0.421.g78406f8d94-goog
>>
>> Hi Matthew,
>>
>> You may want to resend this patch. For whatever reason, the RFL list
>> only received 0/3 and 2/3 [1], and it looks like the other relevant
>> lists only received 2/3 [2].
>>
>> Also seems like text wrapping is off for 0/3 :)
>>
>> - Trevor
>>
>> [1]: https://lore.kernel.org/rust-for-linux/20231115185858.2110875-1-mmaurer@google.com/T/#t
>> [2]: https://lore.kernel.org/linux-modules/20231115185858.2110875-3-mmaurer@google.com/T/#u

Re: [PATCH 0/3] MODVERSIONS + RUST Redux

[Trevor Gross]

On Wed, Nov 15, 2023 at 4:24 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> Should I be sending all these patches to all the lists? I was trying
> to follow the recommendations of get_maintainers.pl, which thought
> modules were the only relevant folks for the export_modules patches.
> (I also added Gary + Masahiro explicitly due to their involvement with
> the previous approach.)
>
> If I should be sending all the patches to all the lists, should I also
> be adding all the suggested To/Cc lines for all the patches, or? I
> didn't see any suggestions on how to approach this in
> https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch

I am not an expert here but I think you generally want all patches to
go to the same group of people / lists. Maybe this isn't a hard and
fast rule but you lose context if you don't see the cover letter or
the change as a set. Also patchsets are picked up as a group, so the
whole thing needs to hit at least one list.

Probably somebody else could add some more context here.

> Sorry for messing up the line wrapping in the cover letter
> [...]

That was just an FYI, we can still read it :)


- Trevor

Re: [PATCH 0/3] MODVERSIONS + RUST Redux

[Ariel Miculas]

On 23/11/15 04:54PM, Trevor Gross wrote:
> On Wed, Nov 15, 2023 at 4:24 PM Matthew Maurer <mmaurer@google.com> wrote:
> >
> > Should I be sending all these patches to all the lists? I was trying
> > to follow the recommendations of get_maintainers.pl, which thought
> > modules were the only relevant folks for the export_modules patches.
> > (I also added Gary + Masahiro explicitly due to their involvement with
> > the previous approach.)
> >
> > If I should be sending all the patches to all the lists, should I also
> > be adding all the suggested To/Cc lines for all the patches, or? I
> > didn't see any suggestions on how to approach this in
> > https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch
> 
> I am not an expert here but I think you generally want all patches to
> go to the same group of people / lists. Maybe this isn't a hard and
> fast rule but you lose context if you don't see the cover letter or
> the change as a set. Also patchsets are picked up as a group, so the
> whole thing needs to hit at least one list.
> 
> Probably somebody else could add some more context here.
> 
I'm using lei [1] with --threads option, so I got all the patches. I
doubt that using get_maintainers.pl is the wrong way to go here, and if
it is, the script should be fixed.

Regards,
Ariel

[1] https://people.kernel.org/monsieuricon/lore-lei-part-1-getting-started

Re: [PATCH 0/3] MODVERSIONS + RUST Redux

[Laura Abbott]

On 11/15/23 16:24, Matthew Maurer wrote:
> Should I be sending all these patches to all the lists? I was trying
> to follow the recommendations of get_maintainers.pl, which thought
> modules were the only relevant folks for the export_modules patches.
> (I also added Gary + Masahiro explicitly due to their involvement with
> the previous approach.)
> 
> If I should be sending all the patches to all the lists, should I also
> be adding all the suggested To/Cc lines for all the patches, or? I
> didn't see any suggestions on how to approach this in
> https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch
> 


 From my experience doing work that touched a large number of areas
across the kernel, I would usually cc everyone on the cover letter
so they have context and then only add the relevant people/lists
to the specific patches. I would generate the patches then
manually go through and adjust the To/Cc lists from get_maintainers
before sending them out with git-send-email. Here, I would
definitely make sure the cover letter got sent to the powerpc
list for example.

That said, a three patch series is pretty short so you could
probably get away with ccing everyone on all the patches unless
you've gotten complaints. There are probably other opinions here
too.

Thanks,
Laura

Re: [PATCH 2/3] modpost: Extended modversion support

[Luis Chamberlain]

On Wed, Nov 15, 2023 at 06:50:10PM +0000, Matthew Maurer wrote:
> Adds a new format for modversions which stores each field in a separate
> elf section.

The "why" is critical and not mentioned. And I'd like to also see
documented this with foresight, if Rust needed could this be used
in the future for other things?

Also please include folks CC'd in *one* patch to *all* patches as
otherwise we have no context.

> 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.
> 
> Adding support for variable length names makes it possible to enable
> MODVERSIONS and RUST at the same time.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  arch/powerpc/kernel/module_64.c | 24 +++++++++-

Why was only powerpc modified? If the commit log explained this it would
make it easier for review.

> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index c8b7b4dcf782..0c188c96a045 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -80,7 +80,7 @@ struct load_info {
>  	unsigned int used_pages;
>  #endif
>  	struct {
> -		unsigned int sym, str, mod, vers, info, pcpu;
> +		unsigned int sym, str, mod, vers, info, pcpu, vers_ext_crc, vers_ext_name;

We might as well modify this in a preliminary patch to add each new
unsinged int in a new line, so that it is easier to blame when each new
entry gets added. It should not grow the size of the struct at all but
it would make futur extensions easier to review what is new and git
blame easier to spot when something was added.

Although we don't use this extensively today this can easily grow for
convenience and making code easier to read.

> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 53f43ac5a73e..93d97dad8c77 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 (modversion_ext_start(info, &version_ext) >= 0) {

There are two things we need to do to make processing modules easier:

  1) ELF validation
  2) Once checked then process the information

We used to have this split up but also had a few places which did both
1) and 2) together. This was wrong and so I want to keep things tidy
and ensure we do things which validate the ELF separate. To that
end please put the checks to validate the ELF first so that we report
to users with a proper error/debug check in case the ELF is wrong,
this enables futher debug checks for that to be done instead of
confusing users who end up scratching their heads why something
failed.

So please split up the ELF validation check and put that into
elf_validity_cache_copy() which runs *earlier* than this.

Then *if* if has this, you just process it. Please take care to be
very pedantic in the elf_validity_cache_copy() and extend the checks
you have for validation in modversion_ext_start() and bring them to
elf_validity_cache_copy() with perhaps *more* stuff which does any
insane checks to verify it is 100% correct.

> +		do {
> +			if (strncmp(version_ext.name.value, symname,
> +				    version_ext.name.end - version_ext.name.value) != 0)
> +				continue;
> +
> +			if (*version_ext.crc.value == *crc)
> +				return 1;
> +			pr_debug("Found checksum %X vs module %X\n",
> +				 *crc, *version_ext.crc.value);
> +			goto bad_version;
> +		} while (modversion_ext_advance(&version_ext) == 0);

Can you do a for_each_foo()) type loop here instead after validation?
Because the validation would ensure your loop is bounded then. Look at
for_each_mod_mem_type() for inspiration.

> +		goto broken_toolchain;

The broken toolchain thing would then be an issue reported in the
ELF validation.

> @@ -87,6 +105,65 @@ int same_magic(const char *amagic, const char *bmagic,
>  	return strcmp(amagic, bmagic) == 0;
>  }
>  
> +#define MODVERSION_FIELD_START(sec, field) \
> +	field.value = (typeof(field.value))sec.sh_addr; \
> +	field.end = field.value + sec.sh_size
> +
> +ssize_t 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.

Curious, what gave you the impression // type style comments are
welcomed, please replace that with either a one line 

/* foo comment */

Or a multi-line:

/*
 * stuff and go into great deatils
 * more elaaborate explanation
 */

Of even better, since you are moving this to ELF Validation please add
undertand what elf_validity_cache_copy() does, and add kdoc style
comments for it and then extend it with why Rust needs these magical things.

> +	if ((crc_idx == 0) || (name_idx == 0))
> +		return -EINVAL;
> +
> +	MODVERSION_FIELD_START(sechdrs[crc_idx], start->crc);
> +	MODVERSION_FIELD_START(sechdrs[name_idx], start->name);
> +
> +	return (start->crc.end - start->crc.value) / sizeof(*start->crc.value);
> +}
> +
> +static int modversion_ext_s32_advance(struct modversion_info_ext_s32 *field)
> +{
> +	if (!field->value)
> +		return 0;
> +	if (field->value >= field->end)
> +		return -EINVAL;
> +	field->value++;
> +	return 0;
> +}
> +
> +static int modversion_ext_string_advance(struct modversion_info_ext_string *s)
> +{
> +	if (!s->value)
> +		return 0;
> +	if (s->value >= s->end)
> +		return -EINVAL;
> +	s->value += strnlen(s->value, s->end - s->value - 1) + 1;
> +	if (s->value >= s->end)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +int modversion_ext_advance(struct modversion_info_ext *start)
> +{
> +	int ret;
> +
> +	ret = modversion_ext_s32_advance(&start->crc);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = modversion_ext_string_advance(&start->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

Please add all the validation as part of the ELF validation sanity checks
and make sure you rant so toolchains get easily debugged and fixed.
That would make the processing of data a secodnary step and it is
easier to read and simpler code. The validation then becomes the part
which kicks issues out early.

>  /*
>   * Generate the signature for all relevant module structures here.
>   * If these change, we don't want to try to parse the module.
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 973b5e5ae2dd..884860c2e833 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1910,15 +1910,42 @@ 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;
> +			/* this symbol will only be in the extended info */
> +			continue;

I cannot grok why this is being done, but hopefully in the next patch
series this will be easier to understand.

 Luis