Re: [PATCH v3 4/6] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC

[Luis Chamberlain]

On Thu, Feb 03, 2022 at 07:05:13AM +0000, Christophe Leroy wrote:
> 
> 
> Le 03/02/2022 à 01:01, Luis Chamberlain a écrit :
> > On Sat, Jan 29, 2022 at 05:02:09PM +0000, 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(&mod->core_layout.mtn, &mod_tree);
> >>   	if (mod->init_layout.size)
> >>   		__mod_tree_insert(&mod->init_layout.mtn, &mod_tree);
> >> +
> >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> >> +	mod->data_layout.mtn.mod = mod;
> >> +	__mod_tree_insert(&mod->data_layout.mtn, &mod_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-atomlin@xxxxxxxxxx
> > 
> > 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