Re: [PATCH 2/3] modpost: Extended modversion support
On Wed, Nov 15, 2023 at 06:50:10PM +, 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
> ---
> 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, _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(_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
[PATCH 2/3] modpost: Extended modversion support
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
---
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(_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
+
