Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Sat, Oct 19, 2024 at 01:45:35PM -0700, Luis Chamberlain wrote: > On Thu, Oct 17, 2024 at 02:08:19PM +0200, Helge Deller wrote: > > Hi Luis, > > > > On 10/17/24 01:21, Luis Chamberlain wrote: > > > That sounds great. Yeah, the above would be great to test. A while ago > > > I wrote a new modules selftests in order to test possible improvements > > > on find_symbol() but I also did this due to push the limits of the > > > numbers of symbols we could support. I wrote all this to also test the > > > possible 64-bit alignment benefits of __ksymtab_ sections on > > > architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (e.g. ppc64, > > > ppc64le, parisc, s390x,...). [] > > > > > > I forget what we concluded on Helge Deller's alignement patches, I think > > > there was an idea on how to address the alignment through other means. > > > > > > [0] > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20241016-modules-symtab > > > > I stumbled upon the unaligned-memory-access.rst document [1]. > > Please read it, as it is a really good document, and the section > > "Why unaligned access is bad" states: > > It should be obvious from the above that if your code causes unaligned > > memory accesses to happen, your code will not work correctly on certain > > platforms and will cause performance problems on others. > > > > With this in mind, you really should apply both of my alignment > > patches which you currently carry in [0]. > > > > For parisc I partly solved the issue by fixing the arch-specific kernel > > unalignment > > handler, but every time module sections are stored unaligned, it triggers > > performance degregation on parisc (and other sensitive platforms). > > > > I suggest you apply them unconditionally. > > > > Helge > > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/unaligned-memory-access.rst > > You're right, I've just referred to that doc and pushed to the new > linux modules [2] modules-next branch. This is also great timing so > that the work that is ongoing for Rust will take this into > consideration as well. I'll just post the test I wrote as separate > thing but it surely can be used to help test some of this later. > > [2] git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git Helge, I went down memory lane and noted Masahiro asked for this to be done in asm so I droped your patches. Feel free to post a new iteration. Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Thu, Oct 17, 2024 at 02:08:19PM +0200, Helge Deller wrote: > Hi Luis, > > On 10/17/24 01:21, Luis Chamberlain wrote: > > That sounds great. Yeah, the above would be great to test. A while ago > > I wrote a new modules selftests in order to test possible improvements > > on find_symbol() but I also did this due to push the limits of the > > numbers of symbols we could support. I wrote all this to also test the > > possible 64-bit alignment benefits of __ksymtab_ sections on > > architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (e.g. ppc64, > > ppc64le, parisc, s390x,...). [] > > > > I forget what we concluded on Helge Deller's alignement patches, I think > > there was an idea on how to address the alignment through other means. > > > > [0] > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20241016-modules-symtab > > I stumbled upon the unaligned-memory-access.rst document [1]. > Please read it, as it is a really good document, and the section > "Why unaligned access is bad" states: > It should be obvious from the above that if your code causes unaligned > memory accesses to happen, your code will not work correctly on certain > platforms and will cause performance problems on others. > > With this in mind, you really should apply both of my alignment > patches which you currently carry in [0]. > > For parisc I partly solved the issue by fixing the arch-specific kernel > unalignment > handler, but every time module sections are stored unaligned, it triggers > performance degregation on parisc (and other sensitive platforms). > > I suggest you apply them unconditionally. > > Helge > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/unaligned-memory-access.rst You're right, I've just referred to that doc and pushed to the new linux modules [2] modules-next branch. This is also great timing so that the work that is ongoing for Rust will take this into consideration as well. I'll just post the test I wrote as separate thing but it surely can be used to help test some of this later. [2] git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
Hi Luis, On 10/17/24 01:21, Luis Chamberlain wrote: That sounds great. Yeah, the above would be great to test. A while ago I wrote a new modules selftests in order to test possible improvements on find_symbol() but I also did this due to push the limits of the numbers of symbols we could support. I wrote all this to also test the possible 64-bit alignment benefits of __ksymtab_ sections on architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (e.g. ppc64, ppc64le, parisc, s390x,...). [] I forget what we concluded on Helge Deller's alignement patches, I think there was an idea on how to address the alignment through other means. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20241016-modules-symtab I stumbled upon the unaligned-memory-access.rst document [1]. Please read it, as it is a really good document, and the section "Why unaligned access is bad" states: It should be obvious from the above that if your code causes unaligned memory accesses to happen, your code will not work correctly on certain platforms and will cause performance problems on others. With this in mind, you really should apply both of my alignment patches which you currently carry in [0]. For parisc I partly solved the issue by fixing the arch-specific kernel unalignment handler, but every time module sections are stored unaligned, it triggers performance degregation on parisc (and other sensitive platforms). I suggest you apply them unconditionally. Helge [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/unaligned-memory-access.rst
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
Le 17/10/2024 à 01:21, Luis Chamberlain a écrit :
On Tue, Oct 15, 2024 at 04:22:22PM -0700, Matthew Maurer wrote:
So, the basic things I can think of to test here are:
1. The kernel can still load the previous MODVERSIONS format
2. The kernel can load the new MODVERSIONS format
3. If we artificially tweak a CRC in the previous format, it will fail to load.
4. If we artificially tweak a CRC in the new format, it will fail to load.
5. With CONFIG_EXTENDED_MODVERSIONS enabled, the kernel will build and
load modules with long symbol names, with MODVERSIONS enabled.
Is there anything else you were thinking of here, or are those the
kinds of checks you were envisioning?
That sounds great. Yeah, the above would be great to test. A while ago
I wrote a new modules selftests in order to test possible improvements
on find_symbol() but I also did this due to push the limits of the
numbers of symbols we could support. I wrote all this to also test the
possible 64-bit alignment benefits of __ksymtab_ sections on
architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (e.g. ppc64,
ppc64le, parisc, s390x,...). But come to think of it, you might be
able to easily leverage this to also just test long symbols by self
generated symbols as another test case. In case its useful to you I've
put this in a rebased branch 20241016-modules-symtab branch. Feel free
to use as you see fit.
By reading this, I discovered that was initially added to powerpc by
commit 271ca788774a ("arch: enable relative relocations for arm64, power
and x86") and then removed due to problem with modules, see commit
ff69279a44e9 ("powerpc: disable support for relative ksymtab references")
Wouldn't it be better to try and fix modules and activate again
CONFIG_HAVE_ARCH_PREL32_RELOCATIONS on powerpc ?
Christophe
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Tue, Oct 15, 2024 at 04:22:22PM -0700, Matthew Maurer wrote: > So, the basic things I can think of to test here are: > > 1. The kernel can still load the previous MODVERSIONS format > 2. The kernel can load the new MODVERSIONS format > 3. If we artificially tweak a CRC in the previous format, it will fail to > load. > 4. If we artificially tweak a CRC in the new format, it will fail to load. > 5. With CONFIG_EXTENDED_MODVERSIONS enabled, the kernel will build and > load modules with long symbol names, with MODVERSIONS enabled. > > Is there anything else you were thinking of here, or are those the > kinds of checks you were envisioning? That sounds great. Yeah, the above would be great to test. A while ago I wrote a new modules selftests in order to test possible improvements on find_symbol() but I also did this due to push the limits of the numbers of symbols we could support. I wrote all this to also test the possible 64-bit alignment benefits of __ksymtab_ sections on architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS (e.g. ppc64, ppc64le, parisc, s390x,...). But come to think of it, you might be able to easily leverage this to also just test long symbols by self generated symbols as another test case. In case its useful to you I've put this in a rebased branch 20241016-modules-symtab branch. Feel free to use as you see fit. I forget what we concluded on Helge Deller's alignement patches, I think there was an idea on how to address the alignment through other means. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20241016-modules-symtab Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
So, the basic things I can think of to test here are: 1. The kernel can still load the previous MODVERSIONS format 2. The kernel can load the new MODVERSIONS format 3. If we artificially tweak a CRC in the previous format, it will fail to load. 4. If we artificially tweak a CRC in the new format, it will fail to load. 5. With CONFIG_EXTENDED_MODVERSIONS enabled, the kernel will build and load modules with long symbol names, with MODVERSIONS enabled. Is there anything else you were thinking of here, or are those the kinds of checks you were envisioning? On Fri, Oct 11, 2024 at 4:46 PM Luis Chamberlain wrote: > > On Fri, Oct 11, 2024 at 04:45:25PM -0700, Luis Chamberlain wrote: > > > > Also, just as I asked Sami, coould you split this up into patch sets? > > One with all the cleanups and elf validation code shifts. And then the > > other code. That will let me pick up quickly the first patch set. > > Oh and if you can think of ways to enhance our test covereage on all > this as I noted to Sami, it would be greatly appreciated. > > Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Fri, Oct 11, 2024 at 04:45:25PM -0700, Luis Chamberlain wrote: > > Also, just as I asked Sami, coould you split this up into patch sets? > One with all the cleanups and elf validation code shifts. And then the > other code. That will let me pick up quickly the first patch set. Oh and if you can think of ways to enhance our test covereage on all this as I noted to Sami, it would be greatly appreciated. Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
Also, just as I asked Sami, coould you split this up into patch sets? One with all the cleanups and elf validation code shifts. And then the other code. That will let me pick up quickly the first patch set. Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Fri, Oct 11, 2024 at 03:27:30PM -0700, Matthew Maurer wrote: > On Fri, Oct 11, 2024 at 3:22 PM Luis Chamberlain wrote: > > > > On Wed, Sep 25, 2024 at 11:38:29PM +, Matthew Maurer wrote: > > > 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. > > > > > > Signed-off-by: Matthew Maurer > > > > I'm all for the ELF validation work so far, all that was nice, thanks > > for all that tidying up. This however is not considering when we really > > need all this at all, and not making it specific to the build times when > > such things are needed. That is, yes I'd like to see the need for this > > clearly explicitly defined through Kconfig, a *select FOO_FEATURE* for > > when this is needed. No need to extend a module with bloat if we don't > > need it, likewise if a kernel was built without needing those things, > > why bloat the modules with the extra information? > > To make sure I understand what you're asking for, are you suggesting: > 1. A config flag for supporting parsing the extended format > 2. A config flag for supporting parsing the existing format > 3. A config flag for putting the extended format into produced modules > 4. A config flag for putting the existing format into produced modules > or some combination of the above? > > I'm currently reading this as #3, but figured I'd ask to be certain. 3), but if your kernel build does not require these extra things, then a simple if !(IS_ENABLED) sanity check could be put in place to avoid processing the information if the kernel didn't need it. It's a one line change. So at run time, we build the same kernel with all that code in, but it makes no sense to be processing modules with that stuff if kernels did not need it. Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Fri, Oct 11, 2024 at 3:22 PM Luis Chamberlain wrote: > > On Wed, Sep 25, 2024 at 11:38:29PM +, Matthew Maurer wrote: > > 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. > > > > Signed-off-by: Matthew Maurer > > I'm all for the ELF validation work so far, all that was nice, thanks > for all that tidying up. This however is not considering when we really > need all this at all, and not making it specific to the build times when > such things are needed. That is, yes I'd like to see the need for this > clearly explicitly defined through Kconfig, a *select FOO_FEATURE* for > when this is needed. No need to extend a module with bloat if we don't > need it, likewise if a kernel was built without needing those things, > why bloat the modules with the extra information? To make sure I understand what you're asking for, are you suggesting: 1. A config flag for supporting parsing the extended format 2. A config flag for supporting parsing the existing format 3. A config flag for putting the extended format into produced modules 4. A config flag for putting the existing format into produced modules or some combination of the above? I'm currently reading this as #3, but figured I'd ask to be certain. > > Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Wed, Sep 25, 2024 at 11:38:29PM +, Matthew Maurer wrote: > 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. > > Signed-off-by: Matthew Maurer I'm all for the ELF validation work so far, all that was nice, thanks for all that tidying up. This however is not considering when we really need all this at all, and not making it specific to the build times when such things are needed. That is, yes I'd like to see the need for this clearly explicitly defined through Kconfig, a *select FOO_FEATURE* for when this is needed. No need to extend a module with bloat if we don't need it, likewise if a kernel was built without needing those things, why bloat the modules with the extra information? Luis
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
On Thu, Sep 26, 2024 at 5:22 AM Christophe Leroy
wrote:
>
>
>
> Le 26/09/2024 à 01:38, Matthew Maurer a écrit :
> > 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.
> >
> > Signed-off-by: Matthew Maurer
> > ---
> > arch/powerpc/kernel/module_64.c | 23 -
> > kernel/module/internal.h| 11
> > kernel/module/main.c| 92 ++---
> > kernel/module/version.c | 45
> > 4 files changed, 161 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/module_64.c
> > b/arch/powerpc/kernel/module_64.c
> > index e9bab599d0c2..4e7b156dd8b2 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -355,6 +355,23 @@ 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++) {
> > + /* Skip one leading dot */
> > + if (last == '\0' && str_seq[in] == '.')
> > + in++;
> > + last = str_seq[in];
> > + str_seq[out++] = last;
> > + }
>
> Why do you need a loop here ?
>
> Can't you just do something like:
>
> if (str_seq[0] == '.')
> memmove(str_seq, str_seq + 1, size);
I initially had the same thought, but it's because this is is a
sequence of multiple null-terminated strings, and we need to dedotify
all of them, not just the first one. Here's an example:
https://godbolt.org/z/avMGnd48M
> > + /* Zero the trailing portion of the names table for robustness */
> > + memset(&str_seq[out], 0, size - out);
>
> This seems unneeded.
Strictly speaking it shouldn't be needed, but I think it's still good
hygiene to not leave another null-terminated fragment at the end.
Sami
Re: [PATCH v5 14/16] modules: Support extended MODVERSIONS info
Le 26/09/2024 à 01:38, Matthew Maurer a écrit :
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.
Signed-off-by: Matthew Maurer
---
arch/powerpc/kernel/module_64.c | 23 -
kernel/module/internal.h| 11
kernel/module/main.c| 92 ++---
kernel/module/version.c | 45
4 files changed, 161 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index e9bab599d0c2..4e7b156dd8b2 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -355,6 +355,23 @@ 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++) {
+ /* Skip one leading dot */
+ if (last == '\0' && str_seq[in] == '.')
+ in++;
+ last = str_seq[in];
+ str_seq[out++] = last;
+ }
Why do you need a loop here ?
Can't you just do something like:
if (str_seq[0] == '.')
memmove(str_seq, str_seq + 1, size);
+ /* Zero the trailing portion of the names table for robustness */
+ memset(&str_seq[out], 0, size - out);
This seems unneeded.
+}
+
/*
* Undefined symbols which refer to .funcname, hack to funcname. Make .TOC.
* seem to be defined (value set later).
Christophe
