[PATCH -stable] disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload

[PATCH -stable] disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload

[Steven Rostedt]

While debugging the e1000e corruption bug with Intel, we discovered
today that the dynamic ftrace code in mainline is the likely source of
this bug.

For the stable kernel we are providing the only viable fix patch: labeling
CONFIG_DYNAMIC_FTRACE as broken. (see the patch below)

We will follow up with a backport patch that contains the fixes. But since
the fixes are not a one liner, the safest approach for now is to
disable the code in question.

The cause of the bug is due to the way the current code in mainline
handles dynamic ftrace.  When dynamic ftrace is turned on, it also
turns on CONFIG_FTRACE which enables the -pg config in gcc that places
a call to mcount at every function call. With just CONFIG_FTRACE this
causes a noticeable overhead.  CONFIG_DYNAMIC_FTRACE works to ease this
overhead by dynamically updating the mcount call sites into nops.

The problem arises when we trace functions and modules are unloaded.
The first time a function is called, it will call mcount and the mcount
call will call ftrace_record_ip. This records the calling site and
stores it in a preallocated hash table. Later on a daemon will
wake up and call kstop_machine and convert any mcount callers into
nops.

The evolution of this code first tried to do this without the kstop_machine
and used cmpxchg to update the callers as they were called. But I
was informed that this is dangerous to do on SMP machines if another
CPU is running that same code. The solution was to do this with
kstop_machine.

We still used cmpxchg to test if the code that we are modifying is
indeed code that we expect to be before updating it - as a final
line of defense.

But on 32bit machines, ioremapped memory and modules share the same
address space. When a module would load its code into memory and execute
some code, that would register the function.

On module unload, ftrace incorrectly did not zap these functions from
its hash (this was the bug). The cmpxchg could have saved us in most
cases (via luck) - but with ioremap-ed memory that was exactly the wrong
thing to do - the results of cmpxchg on device memory are undefined.
(and will likely result in a write)

The pending .28 ftrace tree does not have this bug anymore, as a general push
towards more robustness of code patching, this is done differently: we do not
use cmpxchg and we do a WARN_ON and turn the tracer off if anything deviates
from its expected state. Furthermore, patch sites are statically identified
during build time so there's no runtime discovery of dynamic code areas
anymore, and no room for code unmaps to cause the hash to become out of date.

We believe the fragility of dynamic patching has been sufficiently
addressed in the development code via the static patching method, but further
suggestions to make it more robust are welcome.

Signed-off-by: Steven Rostedt <srostedt@goodmis.org>
Acked-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/trace/Kconfig |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-compile.git/kernel/trace/Kconfig
===================================================================
--- linux-compile.git.orig/kernel/trace/Kconfig	2008-10-02 10:18:49.000000000 -0400
+++ linux-compile.git/kernel/trace/Kconfig	2008-10-15 17:29:34.000000000 -0400
@@ -103,7 +103,8 @@ config CONTEXT_SWITCH_TRACER
 	  all switching of tasks.
 
 config DYNAMIC_FTRACE
-	bool "enable/disable ftrace tracepoints dynamically"
+	bool "enable/disable ftrace tracepoints dynamically (BROKEN)"
+	depends on BROKEN
 	depends on FTRACE
 	depends on HAVE_DYNAMIC_FTRACE
 	default y

Re: [PATCH -stable] disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload

[Steven Rostedt]

On Wed, 15 Oct 2008, Steven Rostedt wrote:
> 
> The problem arises when we trace functions and modules are unloaded.

Note, it is not just module unloading. The issue can happen when the
init functions of a module are freed and the nvram is vmapped there as
well.

-- Steve

Re: [PATCH -stable] disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload

[Greg KH]

On Wed, Oct 15, 2008 at 06:46:12PM -0400, Steven Rostedt wrote:
> 
> On Wed, 15 Oct 2008, Steven Rostedt wrote:
> > 
> > The problem arises when we trace functions and modules are unloaded.
> 
> Note, it is not just module unloading. The issue can happen when the
> init functions of a module are freed and the nvram is vmapped there as
> well.

Ok, that makes sense why so many people hit this.

I'll go do a 2.6.27.1 with this patch in it right now, thanks for
spending the time to look into this.

greg k-h

patch disable-config_dynamic_ftrace-due-to-possible-memory-corruption-on-module-unload.patch added to 2.6.27-stable tree

[gregkh]

This is a note to let you know that we have just queued up the patch titled

    Subject: disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload

to the 2.6.27-stable tree.  Its filename is

    disable-config_dynamic_ftrace-due-to-possible-memory-corruption-on-module-unload.patch

A git repo of this tree can be found at 
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary


>From rostedt@goodmis.org  Wed Oct 15 15:55:25 2008
From: Steven Rostedt <rostedt@goodmis.org>
Date: Wed, 15 Oct 2008 18:21:44 -0400 (EDT)
Subject: disable CONFIG_DYNAMIC_FTRACE due to possible memory corruption on module unload
To: LKML <linux-kernel@vger.kernel.org>, stable@kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>, Andrew Morton <akpm@linux-foundation.org>, Arjan van de Ven <arjan@infradead.org>, gregkh@suse.de, jesse.brandeburg@intel.com, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@elte.hu>
Message-ID: <alpine.DEB.1.10.0810151815110.18960@gandalf.stny.rr.com>

From: Steven Rostedt <rostedt@goodmis.org>

While debugging the e1000e corruption bug with Intel, we discovered
today that the dynamic ftrace code in mainline is the likely source of
this bug.

For the stable kernel we are providing the only viable fix patch: labeling
CONFIG_DYNAMIC_FTRACE as broken. (see the patch below)

We will follow up with a backport patch that contains the fixes. But since
the fixes are not a one liner, the safest approach for now is to
disable the code in question.

The cause of the bug is due to the way the current code in mainline
handles dynamic ftrace.  When dynamic ftrace is turned on, it also
turns on CONFIG_FTRACE which enables the -pg config in gcc that places
a call to mcount at every function call. With just CONFIG_FTRACE this
causes a noticeable overhead.  CONFIG_DYNAMIC_FTRACE works to ease this
overhead by dynamically updating the mcount call sites into nops.

The problem arises when we trace functions and modules are unloaded.
The first time a function is called, it will call mcount and the mcount
call will call ftrace_record_ip. This records the calling site and
stores it in a preallocated hash table. Later on a daemon will
wake up and call kstop_machine and convert any mcount callers into
nops.

The evolution of this code first tried to do this without the kstop_machine
and used cmpxchg to update the callers as they were called. But I
was informed that this is dangerous to do on SMP machines if another
CPU is running that same code. The solution was to do this with
kstop_machine.

We still used cmpxchg to test if the code that we are modifying is
indeed code that we expect to be before updating it - as a final
line of defense.

But on 32bit machines, ioremapped memory and modules share the same
address space. When a module would load its code into memory and execute
some code, that would register the function.

On module unload, ftrace incorrectly did not zap these functions from
its hash (this was the bug). The cmpxchg could have saved us in most
cases (via luck) - but with ioremap-ed memory that was exactly the wrong
thing to do - the results of cmpxchg on device memory are undefined.
(and will likely result in a write)

The pending .28 ftrace tree does not have this bug anymore, as a general push
towards more robustness of code patching, this is done differently: we do not
use cmpxchg and we do a WARN_ON and turn the tracer off if anything deviates
from its expected state. Furthermore, patch sites are statically identified
during build time so there's no runtime discovery of dynamic code areas
anymore, and no room for code unmaps to cause the hash to become out of date.

We believe the fragility of dynamic patching has been sufficiently
addressed in the development code via the static patching method, but further
suggestions to make it more robust are welcome.

Signed-off-by: Steven Rostedt <srostedt@goodmis.org>
Acked-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 kernel/trace/Kconfig |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -103,7 +103,8 @@ config CONTEXT_SWITCH_TRACER
 	  all switching of tasks.
 
 config DYNAMIC_FTRACE
-	bool "enable/disable ftrace tracepoints dynamically"
+	bool "enable/disable ftrace tracepoints dynamically (BROKEN)"
+	depends on BROKEN
 	depends on FTRACE
 	depends on HAVE_DYNAMIC_FTRACE
 	default y


Patches currently in stable-queue which might be from rostedt@goodmis.org are

queue-2.6.27/disable-config_dynamic_ftrace-due-to-possible-memory-corruption-on-module-unload.patch