Re: [PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
Hello,
On Thu, Feb 03, 2022 at 11:51:05AM -0800, Luis Chamberlain wrote:
> On Thu, Feb 03, 2022 at 07:05:13AM +, Christophe Leroy wrote:
> > Le 03/02/2022 à 01:01, Luis Chamberlain a écrit :
> > > On Sat, Jan 29, 2022 at 05:02:09PM +, Christophe Leroy wrote:
> > >> diff --git a/kernel/module.c b/kernel/module.c
> > >> index 11f51e17fb9f..f3758115ebaa 100644
> > >> --- a/kernel/module.c
> > >> +++ b/kernel/module.c
> > >> @@ -81,7 +81,9 @@
> > >> /* If this is set, the section belongs in the init part of the module
> > >> */
> > >> #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> > >>
> > >> +#ifndef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> > >> #definedata_layout core_layout
> > >> +#endif
> > >>
> > >> /*
> > >>* Mutex protects:
> > >> @@ -111,6 +113,12 @@ static struct mod_tree_root {
> > >> #define module_addr_min mod_tree.addr_min
> > >> #define module_addr_max mod_tree.addr_max
> > >>
> > >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> > >> +static struct mod_tree_root mod_data_tree __cacheline_aligned = {
> > >> +.addr_min = -1UL,
> > >> +};
> > >> +#endif
> > >> +
> > >> #ifdef CONFIG_MODULES_TREE_LOOKUP
> > >>
> > >> /*
> > >> @@ -186,6 +194,11 @@ static void mod_tree_insert(struct module *mod)
> > >> __mod_tree_insert(>core_layout.mtn, _tree);
> > >> if (mod->init_layout.size)
> > >> __mod_tree_insert(>init_layout.mtn, _tree);
> > >> +
> > >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> > >> +mod->data_layout.mtn.mod = mod;
> > >> +__mod_tree_insert(>data_layout.mtn, _data_tree);
> > >> +#endif
> > >
> > >
> > > kernel/ directory has quite a few files, module.c is the second to
> > > largest file, and it has tons of stuff. Aaron is doing work to
> > > split things out to make code easier to read and so that its easier
> > > to review changes. See:
> > >
> > > https://lkml.kernel.org/r/20220130213214.1042497-1-atom...@redhat.com
> > >
> > > I think this is a good patch example which could benefit from that work.
> > > So I'd much prefer to see that work go in first than this, so to see if
> > > we can make the below changes more compartamentalized.
> > >
> > > Curious, how much testing has been put into this series?
> >
> >
> > I tested the change up to (including) patch 4 to verify it doesn't
> > introduce regression when not using
> > CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC,
>
> > Then I tested with patch 5. I first tried with the 'hello world' test
> > module. After that I loaded several important modules and checked I
> > didn't get any regression, both with and without STRICT_MODULES_RWX and
> > I checked the consistency in /proc/vmallocinfo
> > /proc/modules /sys/class/modules/*
>
> I wonder if we have a test for STRICT_MODULES_RWX.
>
> > I also tested with a hacked module_alloc() to force branch trampolines.
>
> So to verify that reducing these trampolines actually helps on an
> architecture? I wonder if we can generalize this somehow to let archs
> verify such strategies can help.
>
> I was hoping for a bit more wider testing, like actually users, etc.
> It does not seem like so. So we can get to that by merging this soon
> into modules-next and having this bleed out issues with linux-next.
> We are in good time to do this now.
>
> The kmod tree has tons of tests:
>
> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
>
> Can you use that to verify there are no regressions?
openSUSE has the testsuite packaged so it's easy to run on arbitrary
kernel but only on ppc64(le) because there is no ppc there anymore.
So yes, it does not regress Book3S/64 as far as kmod testsuite is
conderned and building s390x non-modular kernel also still worka but
that's not saying much.
Thanks
Michal
Re: [PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
On Thu, Feb 03, 2022 at 07:05:13AM +, Christophe Leroy wrote:
>
>
> Le 03/02/2022 à 01:01, Luis Chamberlain a écrit :
> > On Sat, Jan 29, 2022 at 05:02:09PM +, Christophe Leroy wrote:
> >> diff --git a/kernel/module.c b/kernel/module.c
> >> index 11f51e17fb9f..f3758115ebaa 100644
> >> --- a/kernel/module.c
> >> +++ b/kernel/module.c
> >> @@ -81,7 +81,9 @@
> >> /* If this is set, the section belongs in the init part of the module */
> >> #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> >>
> >> +#ifndef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> >> #define data_layout core_layout
> >> +#endif
> >>
> >> /*
> >>* Mutex protects:
> >> @@ -111,6 +113,12 @@ static struct mod_tree_root {
> >> #define module_addr_min mod_tree.addr_min
> >> #define module_addr_max mod_tree.addr_max
> >>
> >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> >> +static struct mod_tree_root mod_data_tree __cacheline_aligned = {
> >> + .addr_min = -1UL,
> >> +};
> >> +#endif
> >> +
> >> #ifdef CONFIG_MODULES_TREE_LOOKUP
> >>
> >> /*
> >> @@ -186,6 +194,11 @@ static void mod_tree_insert(struct module *mod)
> >>__mod_tree_insert(>core_layout.mtn, _tree);
> >>if (mod->init_layout.size)
> >>__mod_tree_insert(>init_layout.mtn, _tree);
> >> +
> >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> >> + mod->data_layout.mtn.mod = mod;
> >> + __mod_tree_insert(>data_layout.mtn, _data_tree);
> >> +#endif
> >
> >
> > kernel/ directory has quite a few files, module.c is the second to
> > largest file, and it has tons of stuff. Aaron is doing work to
> > split things out to make code easier to read and so that its easier
> > to review changes. See:
> >
> > https://lkml.kernel.org/r/20220130213214.1042497-1-atom...@redhat.com
> >
> > I think this is a good patch example which could benefit from that work.
> > So I'd much prefer to see that work go in first than this, so to see if
> > we can make the below changes more compartamentalized.
> >
> > Curious, how much testing has been put into this series?
>
>
> I tested the change up to (including) patch 4 to verify it doesn't
> introduce regression when not using
> CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC,
> Then I tested with patch 5. I first tried with the 'hello world' test
> module. After that I loaded several important modules and checked I
> didn't get any regression, both with and without STRICT_MODULES_RWX and
> I checked the consistency in /proc/vmallocinfo
> /proc/modules /sys/class/modules/*
I wonder if we have a test for STRICT_MODULES_RWX.
> I also tested with a hacked module_alloc() to force branch trampolines.
So to verify that reducing these trampolines actually helps on an
architecture? I wonder if we can generalize this somehow to let archs
verify such strategies can help.
I was hoping for a bit more wider testing, like actually users, etc.
It does not seem like so. So we can get to that by merging this soon
into modules-next and having this bleed out issues with linux-next.
We are in good time to do this now.
The kmod tree has tons of tests:
https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
Can you use that to verify there are no regressions?
Aaron, Michal, if you can do the same that'd be appreciated.
Luis
Re: [PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
Le 03/02/2022 à 01:01, Luis Chamberlain a écrit :
> On Sat, Jan 29, 2022 at 05:02:09PM +, Christophe Leroy wrote:
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 11f51e17fb9f..f3758115ebaa 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -81,7 +81,9 @@
>> /* If this is set, the section belongs in the init part of the module */
>> #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
>>
>> +#ifndef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> #definedata_layout core_layout
>> +#endif
>>
>> /*
>>* Mutex protects:
>> @@ -111,6 +113,12 @@ static struct mod_tree_root {
>> #define module_addr_min mod_tree.addr_min
>> #define module_addr_max mod_tree.addr_max
>>
>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> +static struct mod_tree_root mod_data_tree __cacheline_aligned = {
>> +.addr_min = -1UL,
>> +};
>> +#endif
>> +
>> #ifdef CONFIG_MODULES_TREE_LOOKUP
>>
>> /*
>> @@ -186,6 +194,11 @@ static void mod_tree_insert(struct module *mod)
>> __mod_tree_insert(>core_layout.mtn, _tree);
>> if (mod->init_layout.size)
>> __mod_tree_insert(>init_layout.mtn, _tree);
>> +
>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> +mod->data_layout.mtn.mod = mod;
>> +__mod_tree_insert(>data_layout.mtn, _data_tree);
>> +#endif
>
>
> kernel/ directory has quite a few files, module.c is the second to
> largest file, and it has tons of stuff. Aaron is doing work to
> split things out to make code easier to read and so that its easier
> to review changes. See:
>
> https://lkml.kernel.org/r/20220130213214.1042497-1-atom...@redhat.com
>
> I think this is a good patch example which could benefit from that work.
> So I'd much prefer to see that work go in first than this, so to see if
> we can make the below changes more compartamentalized.
>
> Curious, how much testing has been put into this series?
I tested the change up to (including) patch 4 to verify it doesn't
introduce regression when not using
CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC,
Then I tested with patch 5. I first tried with the 'hello world' test
module. After that I loaded several important modules and checked I
didn't get any regression, both with and without STRICT_MODULES_RWX and
I checked the consistency in /proc/vmallocinfo
/proc/modules /sys/class/modules/*
I also tested with a hacked module_alloc() to force branch trampolines.
Christophe
Re: [PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
On Sat, Jan 29, 2022 at 05:02:09PM +, Christophe Leroy wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 11f51e17fb9f..f3758115ebaa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -81,7 +81,9 @@
> /* If this is set, the section belongs in the init part of the module */
> #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
>
> +#ifndef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> #define data_layout core_layout
> +#endif
>
> /*
> * Mutex protects:
> @@ -111,6 +113,12 @@ static struct mod_tree_root {
> #define module_addr_min mod_tree.addr_min
> #define module_addr_max mod_tree.addr_max
>
> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> +static struct mod_tree_root mod_data_tree __cacheline_aligned = {
> + .addr_min = -1UL,
> +};
> +#endif
> +
> #ifdef CONFIG_MODULES_TREE_LOOKUP
>
> /*
> @@ -186,6 +194,11 @@ static void mod_tree_insert(struct module *mod)
> __mod_tree_insert(>core_layout.mtn, _tree);
> if (mod->init_layout.size)
> __mod_tree_insert(>init_layout.mtn, _tree);
> +
> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> + mod->data_layout.mtn.mod = mod;
> + __mod_tree_insert(>data_layout.mtn, _data_tree);
> +#endif
kernel/ directory has quite a few files, module.c is the second to
largest file, and it has tons of stuff. Aaron is doing work to
split things out to make code easier to read and so that its easier
to review changes. See:
https://lkml.kernel.org/r/20220130213214.1042497-1-atom...@redhat.com
I think this is a good patch example which could benefit from that work.
So I'd much prefer to see that work go in first than this, so to see if
we can make the below changes more compartamentalized.
Curious, how much testing has been put into this series?
Luis
[PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC to allow architectures
to request having modules data in vmalloc area instead of module area.
This is required on powerpc book3s/32 in order to set data non
executable, because it is not possible to set executability on page
basis, this is done per 256 Mbytes segments. The module area has exec
right, vmalloc area has noexec.
This can also be useful on other powerpc/32 in order to maximize the
chance of code being close enough to kernel core to avoid branch
trampolines.
Signed-off-by: Christophe Leroy
Cc: Jason Wessel
Acked-by: Daniel Thompson
Cc: Douglas Anderson
---
arch/Kconfig| 6 +++
include/linux/module.h | 8
kernel/debug/kdb/kdb_main.c | 10 -
kernel/module.c | 76 +++--
4 files changed, 94 insertions(+), 6 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 678a80713b21..b5d1f2c19c27 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -882,6 +882,12 @@ config MODULES_USE_ELF_REL
Modules only use ELF REL relocations. Modules with ELF RELA
relocations will give an error.
+config ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ bool
+ help
+ For architectures like powerpc/32 which have constraints on module
+ allocation and need to allocate module data outside of module area.
+
config HAVE_IRQ_EXIT_ON_IRQ_STACK
bool
help
diff --git a/include/linux/module.h b/include/linux/module.h
index 1e135fd5c076..3a892bdcbb5f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -422,6 +422,9 @@ struct module {
/* Core layout: rbtree is accessed frequently, so keep together. */
struct module_layout core_layout __module_layout_align;
struct module_layout init_layout;
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ struct module_layout data_layout;
+#endif
/* Arch-specific module values */
struct mod_arch_specific arch;
@@ -569,6 +572,11 @@ bool is_module_text_address(unsigned long addr);
static inline bool within_module_core(unsigned long addr,
const struct module *mod)
{
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ if ((unsigned long)mod->data_layout.base <= addr &&
+ addr < (unsigned long)mod->data_layout.base + mod->data_layout.size)
+ return true;
+#endif
return (unsigned long)mod->core_layout.base <= addr &&
addr < (unsigned long)mod->core_layout.base +
mod->core_layout.size;
}
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 0852a537dad4..85d3fd40b7fe 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2022,8 +2022,11 @@ static int kdb_lsmod(int argc, const char **argv)
if (mod->state == MODULE_STATE_UNFORMED)
continue;
- kdb_printf("%-20s%8u 0x%px ", mod->name,
- mod->core_layout.size, (void *)mod);
+ kdb_printf("%-20s%8u", mod->name, mod->core_layout.size);
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ kdb_printf("/%8u", mod->data_layout.size);
+#endif
+ kdb_printf(" 0x%px ", (void *)mod);
#ifdef CONFIG_MODULE_UNLOAD
kdb_printf("%4d ", module_refcount(mod));
#endif
@@ -2034,6 +2037,9 @@ static int kdb_lsmod(int argc, const char **argv)
else
kdb_printf(" (Live)");
kdb_printf(" 0x%px", mod->core_layout.base);
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ kdb_printf("/0x%px", mod->data_layout.base);
+#endif
#ifdef CONFIG_MODULE_UNLOAD
{
diff --git a/kernel/module.c b/kernel/module.c
index 11f51e17fb9f..f3758115ebaa 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -81,7 +81,9 @@
/* If this is set, the section belongs in the init part of the module */
#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
+#ifndef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
#definedata_layout core_layout
+#endif
/*
* Mutex protects:
@@ -111,6 +113,12 @@ static struct mod_tree_root {
#define module_addr_min mod_tree.addr_min
#define module_addr_max mod_tree.addr_max
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+static struct mod_tree_root mod_data_tree __cacheline_aligned = {
+ .addr_min = -1UL,
+};
+#endif
+
#ifdef CONFIG_MODULES_TREE_LOOKUP
/*
@@ -186,6 +194,11 @@ static void mod_tree_insert(struct module *mod)
__mod_tree_insert(>core_layout.mtn, _tree);
if (mod->init_layout.size)
__mod_tree_insert(>init_layout.mtn, _tree);
+
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ mod->data_layout.mtn.mod = mod;
+ __mod_tree_insert(>data_layout.mtn, _data_tree);
+#endif
}
static void mod_tree_remove_init(struct module *mod)
@@ -198,6 +211,9 @@ static void mod_tree_remove(struct module *mod)
{
