* [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-30 23:05 [PATCH v8 0/3] Extended MODVERSIONS Support Matthew Maurer
@ 2024-10-30 23:05 ` Matthew Maurer
2024-10-31 1:22 ` Michael Ellerman
2024-10-30 23:05 ` [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information Matthew Maurer
2024-10-30 23:05 ` [PATCH v8 3/3] rust: Use gendwarfksyms + extended modversions for CONFIG_MODVERSIONS Matthew Maurer
2 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-10-30 23:05 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-30 23:05 ` [PATCH v8 1/3] modules: Support extended MODVERSIONS info Matthew Maurer
@ 2024-10-31 1:22 ` Michael Ellerman
2024-10-31 4:36 ` Luis Chamberlain
0 siblings, 1 reply; 22+ messages in thread
From: Michael Ellerman @ 2024-10-31 1:22 UTC (permalink / raw)
To: Matthew Maurer, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux, Matthew Maurer
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-31 1:22 ` Michael Ellerman
@ 2024-10-31 4:36 ` Luis Chamberlain
2024-10-31 5:06 ` Matthew Maurer
0 siblings, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-10-31 4:36 UTC (permalink / raw)
To: Michael Ellerman
Cc: Matthew Maurer, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-31 4:36 ` Luis Chamberlain
@ 2024-10-31 5:06 ` Matthew Maurer
2024-10-31 7:49 ` Luis Chamberlain
0 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-10-31 5:06 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-31 5:06 ` Matthew Maurer
@ 2024-10-31 7:49 ` Luis Chamberlain
2024-11-07 19:40 ` Matthew Maurer
0 siblings, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-10-31 7:49 UTC (permalink / raw)
To: Matthew Maurer
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 1/3] modules: Support extended MODVERSIONS info
2024-10-31 7:49 ` Luis Chamberlain
@ 2024-11-07 19:40 ` Matthew Maurer
0 siblings, 0 replies; 22+ messages in thread
From: Matthew Maurer @ 2024-11-07 19:40 UTC (permalink / raw)
To: Luis Chamberlain, Lucas De Marchi, Lucas De Marchi
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-10-30 23:05 [PATCH v8 0/3] Extended MODVERSIONS Support Matthew Maurer
2024-10-30 23:05 ` [PATCH v8 1/3] modules: Support extended MODVERSIONS info Matthew Maurer
@ 2024-10-30 23:05 ` Matthew Maurer
2024-10-31 11:37 ` Luis Chamberlain
2024-10-30 23:05 ` [PATCH v8 3/3] rust: Use gendwarfksyms + extended modversions for CONFIG_MODVERSIONS Matthew Maurer
2 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-10-30 23:05 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-10-30 23:05 ` [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information Matthew Maurer
@ 2024-10-31 11:37 ` Luis Chamberlain
2024-10-31 20:00 ` Matthew Maurer
0 siblings, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-10-31 11:37 UTC (permalink / raw)
To: Matthew Maurer
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-10-31 11:37 ` Luis Chamberlain
@ 2024-10-31 20:00 ` Matthew Maurer
2024-11-01 21:10 ` Luis Chamberlain
0 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-10-31 20:00 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
> 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?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-10-31 20:00 ` Matthew Maurer
@ 2024-11-01 21:10 ` Luis Chamberlain
2024-11-06 0:26 ` Matthew Maurer
0 siblings, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-11-01 21:10 UTC (permalink / raw)
To: Matthew Maurer, Lucas De Marchi, Lucas De Marchi
Cc: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-01 21:10 ` Luis Chamberlain
@ 2024-11-06 0:26 ` Matthew Maurer
2024-11-06 2:16 ` Luis Chamberlain
0 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-11-06 0:26 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-06 0:26 ` Matthew Maurer
@ 2024-11-06 2:16 ` Luis Chamberlain
2024-11-06 22:19 ` Matthew Maurer
0 siblings, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-11-06 2:16 UTC (permalink / raw)
To: Matthew Maurer
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-06 2:16 ` Luis Chamberlain
@ 2024-11-06 22:19 ` Matthew Maurer
2024-11-07 6:27 ` Lucas De Marchi
2024-11-07 22:38 ` Luis Chamberlain
0 siblings, 2 replies; 22+ messages in thread
From: Matthew Maurer @ 2024-11-06 22:19 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
>
> > 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-06 22:19 ` Matthew Maurer
@ 2024-11-07 6:27 ` Lucas De Marchi
2024-11-07 19:37 ` Matthew Maurer
2024-11-07 22:38 ` Luis Chamberlain
1 sibling, 1 reply; 22+ messages in thread
From: Lucas De Marchi @ 2024-11-07 6:27 UTC (permalink / raw)
To: Matthew Maurer
Cc: Luis Chamberlain, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-07 6:27 ` Lucas De Marchi
@ 2024-11-07 19:37 ` Matthew Maurer
0 siblings, 0 replies; 22+ messages in thread
From: Matthew Maurer @ 2024-11-07 19:37 UTC (permalink / raw)
To: Lucas De Marchi
Cc: Luis Chamberlain, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-06 22:19 ` Matthew Maurer
2024-11-07 6:27 ` Lucas De Marchi
@ 2024-11-07 22:38 ` Luis Chamberlain
2024-11-18 22:25 ` Luis Chamberlain
2024-11-21 21:51 ` Matthew Maurer
1 sibling, 2 replies; 22+ messages in thread
From: Luis Chamberlain @ 2024-11-07 22:38 UTC (permalink / raw)
To: Matthew Maurer
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-07 22:38 ` Luis Chamberlain
@ 2024-11-18 22:25 ` Luis Chamberlain
2024-11-19 0:09 ` Matthew Maurer
2024-11-21 21:51 ` Matthew Maurer
1 sibling, 1 reply; 22+ messages in thread
From: Luis Chamberlain @ 2024-11-18 22:25 UTC (permalink / raw)
To: Matthew Maurer
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-18 22:25 ` Luis Chamberlain
@ 2024-11-19 0:09 ` Matthew Maurer
2024-11-19 1:33 ` Luis Chamberlain
0 siblings, 1 reply; 22+ messages in thread
From: Matthew Maurer @ 2024-11-19 0:09 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
> 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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-19 0:09 ` Matthew Maurer
@ 2024-11-19 1:33 ` Luis Chamberlain
0 siblings, 0 replies; 22+ messages in thread
From: Luis Chamberlain @ 2024-11-19 1:33 UTC (permalink / raw)
To: Matthew Maurer
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information
2024-11-07 22:38 ` Luis Chamberlain
2024-11-18 22:25 ` Luis Chamberlain
@ 2024-11-21 21:51 ` Matthew Maurer
1 sibling, 0 replies; 22+ messages in thread
From: Matthew Maurer @ 2024-11-21 21:51 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Lucas De Marchi, Lucas De Marchi, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux
[-- 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 #
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v8 3/3] rust: Use gendwarfksyms + extended modversions for CONFIG_MODVERSIONS
2024-10-30 23:05 [PATCH v8 0/3] Extended MODVERSIONS Support Matthew Maurer
2024-10-30 23:05 ` [PATCH v8 1/3] modules: Support extended MODVERSIONS info Matthew Maurer
2024-10-30 23:05 ` [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information Matthew Maurer
@ 2024-10-30 23:05 ` Matthew Maurer
2 siblings, 0 replies; 22+ messages in thread
From: Matthew Maurer @ 2024-10-30 23:05 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: linuxppc-dev, linux-kernel, linux-modules, linux-kbuild,
rust-for-linux, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread