[PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Rusty Russell]

With allmodconfig (minus non-building modules) on 32-bit x86:
Total size of modules before: 60009790 bytes
Total size of modules after: 55927866 bytes

Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
are kept resident as well.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/module.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -37,10 +37,12 @@ struct kernel_symbol
 	const char *name;
 };
 
+/* This is put in the __versions section of a module to indicate the version
+ * it expects for unknown symbols. */
 struct modversion_info
 {
 	unsigned long crc;
-	char name[MODULE_NAME_LEN];
+	char *name;
 };
 
 struct module;

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Arjan van de Ven]

On Thu, 29 Jan 2009 00:05:52 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> 
> With allmodconfig (minus non-building modules) on 32-bit x86:
> Total size of modules before: 60009790 bytes
> Total size of modules after: 55927866 bytes
> 
> Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
> are kept resident as well.
> 

that reminds me.. can we just simplify MODVERSIONS to be a md5sum (or
sha1 whatver) of the .config file in the VERMAGIC ?
it's a lot more reliable in detecting incompatibilities, and a lot
less space consumed.

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Rusty Russell]

On Thursday 29 January 2009 01:22:31 Arjan van de Ven wrote:
> On Thu, 29 Jan 2009 00:05:52 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > 
> > With allmodconfig (minus non-building modules) on 32-bit x86:
> > Total size of modules before: 60009790 bytes
> > Total size of modules after: 55927866 bytes
> > 
> > Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
> > are kept resident as well.
> > 
> 
> that reminds me.. can we just simplify MODVERSIONS to be a md5sum (or
> sha1 whatver) of the .config file in the VERMAGIC ?
> it's a lot more reliable in detecting incompatibilities, and a lot
> less space consumed.

Unfortunately people really seem to want the finer granularity that MODVERSIONS (sometimes) provides :(  I've tried killing it off several times, and always failed.

We did discuss changing modversions not to descend more than one level deep into types (ie. MODVERSION(int fn(struct foo *)) would depend on the type of struct foo, but not the type of any struct pointers in struct foo) to reduce the problem where header changes cause type definitions to be exposed or hidden and thus change the modversion.

I asked someone to do some analysis on what effect this would have on signatures in real releases, but never heard back (it was in Austin last year, and there was beer...)

Cheers,
Rusty.



 undefined types





> 

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Arjan van de Ven]

On Thu, 29 Jan 2009 08:59:40 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Thursday 29 January 2009 01:22:31 Arjan van de Ven wrote:
> > On Thu, 29 Jan 2009 00:05:52 +1030
> > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > 
> > > 
> > > With allmodconfig (minus non-building modules) on 32-bit x86:
> > > Total size of modules before: 60009790 bytes
> > > Total size of modules after: 55927866 bytes
> > > 
> > > Saving 7% of module size for CONFIG_MODVERSIONS=y; and these
> > > sections are kept resident as well.
> > > 
> > 
> > that reminds me.. can we just simplify MODVERSIONS to be a md5sum
> > (or sha1 whatver) of the .config file in the VERMAGIC ?
> > it's a lot more reliable in detecting incompatibilities, and a lot
> > less space consumed.
> 
> Unfortunately people really seem to want the finer granularity that
> MODVERSIONS (sometimes) provides :(  I've tried killing it off
> several times, and always failed.

but we could just stick the result in VERMAGIC right?
rather than tacking it to every symbol.

how you calculate the global checksum is almost a separate debate.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Rusty Russell]

On Thursday 29 January 2009 09:11:14 Arjan van de Ven wrote:
> On Thu, 29 Jan 2009 08:59:40 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > On Thursday 29 January 2009 01:22:31 Arjan van de Ven wrote:
> > > On Thu, 29 Jan 2009 00:05:52 +1030
> > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > 
> > > > 
> > > > With allmodconfig (minus non-building modules) on 32-bit x86:
> > > > Total size of modules before: 60009790 bytes
> > > > Total size of modules after: 55927866 bytes
> > > > 
> > > > Saving 7% of module size for CONFIG_MODVERSIONS=y; and these
> > > > sections are kept resident as well.
> > > > 
> > > 
> > > that reminds me.. can we just simplify MODVERSIONS to be a md5sum
> > > (or sha1 whatver) of the .config file in the VERMAGIC ?
> > > it's a lot more reliable in detecting incompatibilities, and a lot
> > > less space consumed.
> > 
> > Unfortunately people really seem to want the finer granularity that
> > MODVERSIONS (sometimes) provides :(  I've tried killing it off
> > several times, and always failed.
> 
> but we could just stick the result in VERMAGIC right?
> rather than tacking it to every symbol.

Sure, but who would use it?  If you add a driver I don't want my modules to fail.

Confused,
Rusty.

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Jon Masters]

On Thu, 2009-01-29 at 08:59 +1030, Rusty Russell wrote:

> We did discuss changing modversions not to descend more than
> one level deep into types (ie. MODVERSION(int fn(struct foo *))
> would depend on the type of struct foo, but not the type of any
> struct pointers in struct foo) to reduce the problem where header
> changes cause type definitions to be exposed or hidden and thus
> change the modversion.

Yeah. We looked at that for a certain Enterprise Linux distribution but
various people suggested it was more hassle than it was worth to get it
right. I did do some initial poking - I'll dig it up and post it here.

Jon.

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Shawn Bohrer]

On Thu, Jan 29, 2009 at 12:05:52AM +1030, Rusty Russell wrote:
> With allmodconfig (minus non-building modules) on 32-bit x86:
> Total size of modules before: 60009790 bytes
> Total size of modules after: 55927866 bytes
> 
> Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
> are kept resident as well.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  include/linux/module.h |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -37,10 +37,12 @@ struct kernel_symbol
>  	const char *name;
>  };
>  
> +/* This is put in the __versions section of a module to indicate the version
> + * it expects for unknown symbols. */
>  struct modversion_info
>  {
>  	unsigned long crc;
> -	char name[MODULE_NAME_LEN];
> +	char *name;
>  };

Hey Rusty thanks for the change, but I just got around to testing this
and I have a few observations/questions.  First this breaks:

modprobe --dump-modversions foo.ko

I actually don't care, but just happened to stumble upon it when I also
noticed that your changes seemed to be working a little too well.  I
currently happen to be building my modules out of order for example
module B depends on symbols from module A, but I build module B first.
This means that the exported symbols are not in the Module.symvers file
when module B is compiled.  I would expect module B to fail to load yet
with your patches it magically works and I don't see any errors in the
logs.  If I run:

objdump -s --section __versions B.ko

I only get something like:

Contents of section __versions:
 0000 d782ec86 00000000

Of course if I build the modules in the correct order and dump the
__versions section I see the correct number of crc/char* pairs.

--
Shawn

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Rusty Russell]

On Friday 06 February 2009 02:31:28 Shawn Bohrer wrote:
> On Thu, Jan 29, 2009 at 12:05:52AM +1030, Rusty Russell wrote:
> > With allmodconfig (minus non-building modules) on 32-bit x86:
> > Total size of modules before: 60009790 bytes
> > Total size of modules after: 55927866 bytes
> > 
> > Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
> > are kept resident as well.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > ---
> >  include/linux/module.h |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -37,10 +37,12 @@ struct kernel_symbol
> >  	const char *name;
> >  };
> >  
> > +/* This is put in the __versions section of a module to indicate the version
> > + * it expects for unknown symbols. */
> >  struct modversion_info
> >  {
> >  	unsigned long crc;
> > -	char name[MODULE_NAME_LEN];
> > +	char *name;
> >  };
> 
> Hey Rusty thanks for the change, but I just got around to testing this
> and I have a few observations/questions.  First this breaks:
> 
> modprobe --dump-modversions foo.ko

Hmm, that's an argument not to make this change.  Really, other than disk
space benefit we get all the benefits from just dropping the 
__versions section once loaded.

> I actually don't care, but just happened to stumble upon it when I also
> noticed that your changes seemed to be working a little too well.  I
> currently happen to be building my modules out of order for example
> module B depends on symbols from module A, but I build module B first.
> This means that the exported symbols are not in the Module.symvers file
> when module B is compiled.  I would expect module B to fail to load yet
> with your patches it magically works and I don't see any errors in the
> logs.

Interesting.  What is the value of /proc/sys/kernel/tainted?

And anyway, what was the symbol name which is over 56 characters long which
started this?

Thanks,
Rusty.

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Shawn Bohrer]

On Sat, Feb 07, 2009 at 12:54:28PM +1030, Rusty Russell wrote:
> On Friday 06 February 2009 02:31:28 Shawn Bohrer wrote:
> > On Thu, Jan 29, 2009 at 12:05:52AM +1030, Rusty Russell wrote:
> > > With allmodconfig (minus non-building modules) on 32-bit x86:
> > > Total size of modules before: 60009790 bytes
> > > Total size of modules after: 55927866 bytes
> > > 
> > > Saving 7% of module size for CONFIG_MODVERSIONS=y; and these sections
> > > are kept resident as well.
> > > 
> > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > ---
> > >  include/linux/module.h |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/module.h b/include/linux/module.h
> > > --- a/include/linux/module.h
> > > +++ b/include/linux/module.h
> > > @@ -37,10 +37,12 @@ struct kernel_symbol
> > >  	const char *name;
> > >  };
> > >  
> > > +/* This is put in the __versions section of a module to indicate the version
> > > + * it expects for unknown symbols. */
> > >  struct modversion_info
> > >  {
> > >  	unsigned long crc;
> > > -	char name[MODULE_NAME_LEN];
> > > +	char *name;
> > >  };
> > 
> > Hey Rusty thanks for the change, but I just got around to testing this
> > and I have a few observations/questions.  First this breaks:
> > 
> > modprobe --dump-modversions foo.ko
> 
> Hmm, that's an argument not to make this change.  Really, other than disk
> space benefit we get all the benefits from just dropping the 
> __versions section once loaded.
> 
> > I actually don't care, but just happened to stumble upon it when I also
> > noticed that your changes seemed to be working a little too well.  I
> > currently happen to be building my modules out of order for example
> > module B depends on symbols from module A, but I build module B first.
> > This means that the exported symbols are not in the Module.symvers file
> > when module B is compiled.  I would expect module B to fail to load yet
> > with your patches it magically works and I don't see any errors in the
> > logs.
> 
> Interesting.  What is the value of /proc/sys/kernel/tainted?

It is 3 in both cases (built in the correct and incorrect order).  If
I'm reading this correctly that means my module is proprietary and was
force loaded.  It is proprietary, but was not literally force loaded.
I'm guessing since the modversions information is wrong it sets the
force loaded bit.  I have no idea why the force loaded bit is set even
when things are built in the correct order.

Of course this made me dig a little deeper, and I think I may have been
fighting a regression all along, or possibly I'm missing a kernel config
option.  My real problem is that on openSUSE 11.1 (2.6.27) I was trying
to load my module and modprobe returned:

FATAL: Error inserting foo: Invalid module format

running dmesg I saw the kernel print:

foo: no symbol version for struct_module

After some digging I realized I wasn't building with modversion support
and that led to the long symbol name problem.  I originally believed
that openSUSE 11.1 turned on CONFIG_MODVERSIONS and previous versions
did not have it enabled.  After taking a closer look at openSUSE 10.3
(2.6.22) I found it did have CONFIG_MODVERSIONS=y and that modprobing my
module succeeds but the kernel does print:

foo: no version for "stuct_module" found: kernel tainted

This also results in a taint flag of 3.  So I'm thinking the regression
here is that somewhere between 2.6.25 (I know openSUSE 11 worked too)
and 2.6.27 the kernel stopped allowing modules to load if they didn't
build with modversion support and the kernel was built with
CONFIG_MODVERSIONS=y.

> And anyway, what was the symbol name which is over 56 characters long which
> started this?

Yeah, that is an interesting question isn't it?  Let me just say that
namespaces and name mangling make it really easy to have names longer
than 56 characters.

I actually don't care at all about modversions.  I also don't care if it
taints the kernel, I really just want the modules to load.

--
Shawn

Re: [PATCH 5/6] module: make modversion_info contain a pointer, not an array.

[Rusty Russell]

On Tuesday 10 February 2009 04:20:50 Shawn Bohrer wrote:
> This also results in a taint flag of 3.  So I'm thinking the regression
> here is that somewhere between 2.6.25 (I know openSUSE 11 worked too)
> and 2.6.27 the kernel stopped allowing modules to load if they didn't
> build with modversion support and the kernel was built with
> CONFIG_MODVERSIONS=y.

Before 2.6.26, the kernel ignored the vermagic field when
CONFIG_MODVERSIONS=y.  I changed it to only ignore vermagic *if* there
were CRCs.

If you use modprobe --force, you should be able to load a non-modversions
module into a modversions kernel.

> > And anyway, what was the symbol name which is over 56 characters long which
> > started this?
> 
> Yeah, that is an interesting question isn't it?  Let me just say that
> namespaces and name mangling make it really easy to have names longer
> than 56 characters.

Yes, C++.  This is one reason we don't support it.

Hope that helps,
Rusty.