strict rejection of init -> core references causes problems

strict rejection of init -> core references causes problems

[David Mosberger]

Tony,

The recent patch of yours to the kernel module loader is causing some
problems with the netfilter modules.  To recap, with that patch
applied, calls from the core section of a module to an init section
are rejected because that's in general a dangerous thing to do (the
called code may be gone by the time the call gets executed).

I'm not terribly familiar with the netfilter code, but I think the
problem there comes from the fact that some code is shared between
init and exit handlers and that shared code ends up in the module
core.

For example, ip_nat_standalone.c:init_or_cleanup() may call
ip_nat_rule_init(), which is marked as an __init function.  This
happens to be safe because ip_nat_rule_init() is only called during
module-initialization.  Seems to me like a dangerous practice, but if
it's common practice, I suppose we don't have much choice but to allow
calls from core to init sections again.  Perhaps someone from the
netfilter team could comment?

	--david

Re: [netfilter-core] strict rejection of init -> core references causes problems

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On Fri, Jul 09, 2004 at 12:02:26AM -0700, David Mosberger wrote:
> Tony,
> 
> To recap, with that patch applied, calls from the core section of a
> module to an init section are rejected because that's in general a
> dangerous thing to do (the called code may be gone by the time the
> call gets executed).

This breaks almost all netfilter modules, indeed.

> I'm not terribly familiar with the netfilter code, but I think the
> problem there comes from the fact that some code is shared between
> init and exit handlers and that shared code ends up in the module
> core.

that's exactly the case.  Since we have fairly long initialization
functions (allocating/initializing dozens of ressources, where each one
can fail), we share the code for partial and full de-initialization
between the failing module_init() routine and the module_exit() routine.

Initially Rusty wrote the modules like this, and I very much agree with
this decision to not replicate the same code in module_init() and
_exit() functions. (which would have to be kept in sync every time some
item is added to the initialization chain).

> it's common practice, I suppose we don't have much choice but to allow
> calls from core to init sections again.  Perhaps someone from the
> netfilter team could comment?

I wuold very much appreciate if this was still possible in the future.

> 	--david

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

RE: strict rejection of init -> core references causes problems

[Luck, Tony]

>The recent patch of yours to the kernel module loader is causing some
>problems with the netfilter modules.  To recap, with that patch
>applied, calls from the core section of a module to an init section
>are rejected because that's in general a dangerous thing to do (the
>called code may be gone by the time the call gets executed).

I was worried that this might be the case.  I did try loading a
selection of stuff as modules, and didn't hit the problem, but
I obviously didn't have netfilter configured as a module.

I'll make a patch to re-allow this later today.

-Tony

Re: [netfilter-core] strict rejection of init -> core references causes problems

[David Mosberger]

>>>>> On Fri, 9 Jul 2004 10:01:27 +0200, Harald Welte <laforge@netfilter.org> said:

  Harald> Initially Rusty wrote the modules like this, and I very much
  Harald> agree with this decision to not replicate the same code in
  Harald> module_init() and _exit() functions. (which would have to be
  Harald> kept in sync every time some item is added to the
  Harald> initialization chain).

Seems to me the maintainance aspect could be taken care of with inline
functions.  No?

  >> it's common practice, I suppose we don't have much choice but to allow
  >> calls from core to init sections again.  Perhaps someone from the
  >> netfilter team could comment?

  Harald> I wuold very much appreciate if this was still possible in
  Harald> the future.

We don't have much of a choice, but I think it would be worthwhile to
consider whether netfilter could avoid doing this in the future.  It
really is very easy to make a mistake when allowing core->init calls.

	--david

RE: strict rejection of init -> core references causes problems

[Luck, Tony]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

Here's what the patch looks like if you do want to
drop the kernel prohibition on calls from module core
to module init routines.

Has anyone got a script that could look for other instances
of modules that do this?  If netfilter is the only one, then
it would be nice to keep the prohibition and fix netfilter
(debugging problems with bad calls after the module init
section has been freed sounds like no fun at all ... especially
if the freed page is later re-allocated to some other module).


Signed-off-by: <tony.luck@intel.com>

[-- Attachment #2: module2.patch --]
[-- Type: application/octet-stream, Size: 1504 bytes --]

diff -ru old/arch/ia64/kernel/module.c new/arch/ia64/kernel/module.c
--- old/arch/ia64/kernel/module.c	2004-07-09 10:37:12.444130040 -0700
+++ new/arch/ia64/kernel/module.c	2004-07-09 10:35:58.744861672 -0700
@@ -656,26 +656,18 @@
 	      case RV_PCREL:
 		switch (r_type) {
 		      case R_IA64_PCREL21B:
-			if (in_init(mod, val)) {
-				/* Calls to init code from core are bad news */
-				if (in_core(mod, (uint64_t)location)) {
-					printk(KERN_ERR "%s: init symbol 0x%lx used in module code at %p\n",
-						mod->name, val, location);
-					return -ENOEXEC;
-				}
-			} else if (in_core(mod, val)) {
+			if ((in_init(mod, val) && in_core(mod, (uint64_t)location)) ||
+			    (in_core(mod, val) && in_init(mod, (uint64_t)location))) {
 				/*
 				 * Init section may have been allocated far away from core,
 				 * if the branch won't reach, then allocate a plt for it.
 				 */
-				if (in_init(mod, (uint64_t)location)) {
-					uint64_t delta = ((int64_t)val - (int64_t)location) / 16;
-					if (delta + (1 << 20) >= (1 << 21)) {
-						val = get_fdesc(mod, val, &ok);
-						val = get_plt(mod, location, val, &ok);
-					}
+				uint64_t delta = ((int64_t)val - (int64_t)location) / 16;
+				if (delta + (1 << 20) >= (1 << 21)) {
+					val = get_fdesc(mod, val, &ok);
+					val = get_plt(mod, location, val, &ok);
 				}
-			} else
+			} else if (!is_internal(mod, val))
 				val = get_plt(mod, location, val, &ok);
 			/* FALL THROUGH */
 		      default:

Re: [netfilter-core] strict rejection of init -> core references causes problems

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Fri, Jul 09, 2004 at 11:36:35AM -0700, David Mosberger wrote:

> We don't have much of a choice, but I think it would be worthwhile to
> consider whether netfilter could avoid doing this in the future.  It
> really is very easy to make a mistake when allowing core->init calls.

well, all we do is:

1) have a common init and de-init function
2) call it from a wrapper funcion declared as module_init()
3) call it from a wrapper function declared __exit as module_exit()

Since 1) is called from init and fini, we cannot declare it as __init
(this would remove it from ram).

So what really happens is: calls from init to 'core' functions and calls
from 'exit' to core functions.

> 	--david

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

RE: [netfilter-core] strict rejection of init -> core references causes problems

[Luck, Tony]

>So what really happens is: calls from init to 'core' functions 
>and calls from 'exit' to core functions.

Hmmm.  Neither of those should have tripped the check in the
kernel module loader.  It should only go bang when you have a
call from "core" to "init".  Does "exit" end up in a separate
section?  Maybe I was mistakenly triggering on the exit->core?

Which module actually failed to load?

-Tony

Re: [netfilter-core] strict rejection of init -> core references causes problems

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On Fri, Jul 09, 2004 at 12:19:46PM -0700, Luck, Tony wrote:
> >So what really happens is: calls from init to 'core' functions 
> >and calls from 'exit' to core functions.
> 
> Hmmm.  Neither of those should have tripped the check in the
> kernel module loader.  It should only go bang when you have a
> call from "core" to "init".  Does "exit" end up in a separate
> section?  Maybe I was mistakenly triggering on the exit->core?

Apparently yes:

  0 .text         00003f78  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  1 .init.text    0000029c  00000000  00000000  00003fac  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  2 .exit.text    00000008  00000000  00000000  00004248  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE

But also, we don't call __init from __exit (which would be crash once
__init has been purged from memory).  

> Which module actually failed to load?

yes, I would be interested in that, too :)

> -Tony

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [netfilter-core] strict rejection of init -> core references

[Jean-Marc Saffroy]

On Sat, 10 Jul 2004, Harald Welte wrote:

> On Fri, Jul 09, 2004 at 12:19:46PM -0700, Luck, Tony wrote:
> > >So what really happens is: calls from init to 'core' functions 
> > >and calls from 'exit' to core functions.
> > 
> > Hmmm.  Neither of those should have tripped the check in the
> > kernel module loader.  It should only go bang when you have a
> > call from "core" to "init".  Does "exit" end up in a separate
> > section?  Maybe I was mistakenly triggering on the exit->core?
> 
> Apparently yes:
> 
>   0 .text         00003f78  00000000  00000000  00000034  2**2
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>   1 .init.text    0000029c  00000000  00000000  00003fac  2**2
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>   2 .exit.text    00000008  00000000  00000000  00004248  2**2
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE

AFAIK, only section with names beginning with ".init" are treated 
differently by the kernel module loader, in:
  kernel/module.c:layout_sections()
  arch/ia64/kernel/module.c:module_frob_arch_sections()

Common code in kernel/module.c:load_module() makes only 2 calls to (arch 
specific) module_alloc(), and uses one area for init code/data/plt/etc. 
and the other for "core" (including .exit sections) code/data/...

So I don't think the patch can cause any problem with exit sections.


-- 
Jean-Marc Saffroy - jean-marc.saffroy@ext.bull.net