Fault Injection issues: stacktrace x86_64 and failslab NUMA

Fault Injection issues: stacktrace x86_64 and failslab NUMA

[Scott Porter]

I'm attempting to use Fault Injection stacktrace filtering on an x86_64
platform (see config details below) and finding problems:

(1) Apparently stacktrace on x86_64 isn't always reliable but the fault
injection code path to save a stack trace looks *completely* unreliable

(2) CONFIG_NUMA and failslab don't mix -- the NUMA code successfully
allocates after the fault injection code thinks it has failed!

Details:

(1) When the fault injection code is saving a stack trace to compare
require-start/require-end addresses, it employs the following code in
arch/x86_64/kernel/traps.c where the task is the currently running
thread:

        if (!stack) {
                unsigned long dummy;
                stack = &dummy;
                if (tsk && tsk != current)
                        stack = (unsigned long *)tsk->thread.rsp;
        }

Can anyone explain how this code could possibly get a valid stack
pointer using "&dummy" on an automatic variable defined in the middle of
a routine that has arguments and other auto variables?  When I look at
the stack buffer that is saved it contains only entries for
"should_fail()" and so fail_stacktrace() NEVER matches on the start/end
addresses.

Prior to this patch:

  2006-09-26 Andi Kleen [PATCH] Merge stacktrace and show_trace

the save_stack_trace() routine would get the current thread stack
pointer this way:

-       if (!task || task == current) {
-               /* Grab rbp right from our regs: */
-               asm ("mov %%rbp, %0" : "=r" (stack));

Any pointers you can give to help me understand x86_64 stack frames
and/or how this code should work is appreciated!

I'd like to take advantage of the Fault Injection capabilities to debug
error paths in a driver but without reliable stacktrace it looks like
I'm left to invent my own kmalloc() error injection methods.  Anybody
know how to interpose (a la LD_PRELOAD) on exported kernel interfaces
like kmalloc() for a loadable kernel module?!  I'm thinking that Fault
Injection for failslab and fail-page-alloc provided as a separate
loadable module would be more appealing than having to add debug code
into the kernel anyways?

(2) The fault injection code in mm/slab.c within ____cache_alloc()
returns NULL when should_failslab() returns true.  HOWEVER, later code
in __cache_alloc() compiled in with NUMA_BUILD (via CONFIG_NUMA)
successfully allocates memory from another node and prevents the error
injection from really failing:

    if (!objp)
            objp = ____cache_alloc(cachep, flags);
    /*
     * We may just have run out of memory on the local node.
     * ____cache_alloc_node() knows how to locate memory on other nodes
     */
    if (NUMA_BUILD && !objp)
            objp = ____cache_alloc_node(cachep, flags, numa_node_id());

Not sure what the best way is to deal with this (other than turning off
CONFIG_NUMA!).

Config details:

2.6.20.1 x86_64
CONFIG_DEBUG_KERNEL=y
CONFIG_STACKTRACE=y
CONFIG_FRAME_POINTER=y
CONFIG_FAULT_INJECTION=y
CONFIG_FAILSLAB=y
CONFIG_FAIL_PAGE_ALLOC=y
# CONFIG_FAIL_MAKE_REQUEST is not set
CONFIG_FAULT_INJECTION_DEBUG_FS=y
CONFIG_NUMA=y

Re: Fault Injection issues: stacktrace x86_64 and failslab NUMA

[Scott Porter]

On Fri, 2007-04-20 at 13:55 -0500, Scott Porter wrote:
> I'm attempting to use Fault Injection stacktrace filtering on an x86_64
> platform (see config details below) and finding problems:
> 
> (1) Apparently stacktrace on x86_64 isn't always reliable but the fault
> injection code path to save a stack trace looks *completely* unreliable
> 

An update after a closer look at the x86_64 stacktrace code:

There is no FRAME_POINTER support in the x86_64 code.  Apparently it was
removed when the unwind code was added but now that has been removed as
well!

Anyways, the current dump_trace() code scans the *entire* process-level
kernel stack looking for anything resembling a kernel text address.
Guess where Fault Injection saves the stacktrace entries it is going to
compare -- on the same kernel stack!  So, the code is tripping over
itself such that once Fault Injection with stacktrace has been enabled
for a while the stack is littered with saved kernel text addresses.
This causes false positives in fail_stacktrace() when matching stale
addresses from prior call chains as well as missed matches due to the
buffer filling up with these stale entries.  Not to mention the fact
that dump_stack() happily reports all these stale addresses in the trace
back!

Digging around in the archives it looks like Andi Kleen knew this was an
issue with the x86_64 fallback stacktrace code.  Now that there is no
unwind code to even attempt to avoid the problem what should be done?
How about:

(1) Make it clear the Fault Injection with STACKTRACE on x86_64 is at
best "Russian Roulette" -- maybe a !X86_64 in Kconfig.debug?

(2) Introduce FRAME_POINTER support back into the x86_64 code.  This is
what Fault Injection really wants.

(3) Keep the saved stack address entries array out of sight of the
fallback save_stack_trace() code.  Lockdep does this by storing it in
static space but this requires locking which would be ugly for Fault
Injection.  Another option is to mask the saved addresses so they fail
the __kernel_text_address() test but fail_stacktrace() uses the same
mask to make it's comparisons.  There's still the problem of avoiding
kernel text addresses stored on the stack by other code (that is, other
than the expected stack chain uses).

Comments?

- Scott

P.S. The good news is if/when the unwind code is ready to merge back in
there is a testcase ready and waiting -- just enable Fault Injection
with failslab and the stacks will get unwound on every Kmalloc call!

Re: Fault Injection issues: stacktrace x86_64 and failslab NUMA

[Andi Kleen]

> There is no FRAME_POINTER support in the x86_64 code.  Apparently it was
> removed when the unwind code was added but now that has been removed as
> well!

That's because no assembly code in x86-64 knows anything about frame pointers.
You will always get truncated traces if anything assembler is in trace.
That's just dangerous.

> Digging around in the archives it looks like Andi Kleen knew this was an
> issue with the x86_64 fallback stacktrace code.  Now that there is no

No I didn't.

> unwind code to even attempt to avoid the problem what should be done?
> How about:
> 
> (1) Make it clear the Fault Injection with STACKTRACE on x86_64 is at
> best "Russian Roulette" -- maybe a !X86_64 in Kconfig.debug?
> 
> (2) Introduce FRAME_POINTER support back into the x86_64 code.  This is
> what Fault Injection really wants.
> 
> (3) Keep the saved stack address entries array out of sight of the
> fallback save_stack_trace() code.  Lockdep does this by storing it in
> static space but this requires locking which would be ugly for Fault
> Injection.  Another option is to mask the saved addresses so they fail
> the __kernel_text_address() test but fail_stacktrace() uses the same
> mask to make it's comparisons.  There's still the problem of avoiding
> kernel text addresses stored on the stack by other code (that is, other
> than the expected stack chain uses).

Some hack in (3) would be probably best, otherwise (1).
At some point I hope we can get the dwarf2 unwinder back, then
the problem should be also solved. But then you would need to force
the dwarf2 unwinder with fault injection on, but that shouldn't
be a problem.

-Andi

Re: Fault Injection issues: stacktrace x86_64 and failslab NUMA

[Akinobu Mita]

On Sun, Apr 22, 2007 at 12:10:33AM +0200, Andi Kleen wrote:
> > unwind code to even attempt to avoid the problem what should be done?
> > How about:
> > 
> > (1) Make it clear the Fault Injection with STACKTRACE on x86_64 is at
> > best "Russian Roulette" -- maybe a !X86_64 in Kconfig.debug?
> > 
> > (2) Introduce FRAME_POINTER support back into the x86_64 code.  This is
> > what Fault Injection really wants.
> > 
> > (3) Keep the saved stack address entries array out of sight of the
> > fallback save_stack_trace() code.  Lockdep does this by storing it in
> > static space but this requires locking which would be ugly for Fault
> > Injection.  Another option is to mask the saved addresses so they fail
> > the __kernel_text_address() test but fail_stacktrace() uses the same
> > mask to make it's comparisons.  There's still the problem of avoiding
> > kernel text addresses stored on the stack by other code (that is, other
> > than the expected stack chain uses).
> 
> Some hack in (3) would be probably best, otherwise (1).
> At some point I hope we can get the dwarf2 unwinder back, then
> the problem should be also solved. But then you would need to force
> the dwarf2 unwinder with fault injection on, but that shouldn't
> be a problem.

I'm goint to send the patch that disables stacktrace filter
(CONFIG_FAULT_INJECTION_STACKTRACE_FILTER) on x86_64.

But dwarf2 unwinder is still available on -mm. So I'll
send -mm only patch that enables it at the same time.

BTW, are there any pending issues in dwarf2 unwinder?

[PATCH] fault injection: disable stacktrace filter for x86-64

[Akinobu Mita]

Disable stacktrace filter support for x86-64 for now.
Will be enable when we can get the dwarf2 unwinder back.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 lib/Kconfig.debug |    1 +
 1 file changed, 1 insertion(+)

Index: 2.6-git/lib/Kconfig.debug
===================================================================
--- 2.6-git.orig/lib/Kconfig.debug
+++ 2.6-git/lib/Kconfig.debug
@@ -442,6 +442,7 @@ config FAULT_INJECTION_DEBUG_FS
 config FAULT_INJECTION_STACKTRACE_FILTER
 	bool "stacktrace filter for fault-injection capabilities"
 	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
+	depends on !X86_64
 	select STACKTRACE
 	select FRAME_POINTER
 	help

[PATCH] fault injection: fix failslab with CONFIG_NUMA

[Akinobu Mita]

Currently failslab injects failures into ____cache_alloc().
But with enabling CONFIG_NUMA it's not enough to let actual
slab allocator functions (kmalloc, kmem_cache_alloc, ...) return NULL.

This patch moves fault injection hook inside of __cache_alloc() and
__cache_alloc_node(). These are lower call path than ____cache_alloc()
and enable to inject faulures to slab allocators with CONFIG_NUMA.

Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 mm/slab.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: 2.6-git/mm/slab.c
===================================================================
--- 2.6-git.orig/mm/slab.c
+++ 2.6-git/mm/slab.c
@@ -3142,7 +3142,7 @@ static int __init failslab_debugfs(void)
 	struct dentry *dir;
 	int err;
 
-       	err = init_fault_attr_dentries(&failslab.attr, "failslab");
+	err = init_fault_attr_dentries(&failslab.attr, "failslab");
 	if (err)
 		return err;
 	dir = failslab.attr.dentries.dir;
@@ -3180,9 +3180,6 @@ static inline void *____cache_alloc(stru
 
 	check_irq_off();
 
-	if (should_failslab(cachep, flags))
-		return NULL;
-
 	ac = cpu_cache_get(cachep);
 	if (likely(ac->avail)) {
 		STATS_INC_ALLOCHIT(cachep);
@@ -3374,6 +3371,9 @@ __cache_alloc_node(struct kmem_cache *ca
 	unsigned long save_flags;
 	void *ptr;
 
+	if (should_failslab(cachep, flags))
+		return NULL;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 
@@ -3444,6 +3444,9 @@ __cache_alloc(struct kmem_cache *cachep,
 	unsigned long save_flags;
 	void *objp;
 
+	if (should_failslab(cachep, flags))
+		return NULL;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 	objp = __do_cache_alloc(cachep, flags);

[PATCH] fault injection: add entry to MAINTAINERS

[Akinobu Mita]

Add maintainer for fault injection support.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 MAINTAINERS |    5 +++++
 1 file changed, 5 insertions(+)

Index: 2.6-git/MAINTAINERS
===================================================================
--- 2.6-git.orig/MAINTAINERS
+++ 2.6-git/MAINTAINERS
@@ -1355,6 +1355,11 @@ M:	kevin.curtis@farsite.co.uk
 W:	http://www.farsite.co.uk/
 S:	Supported
 
+FAULT INJECTION SUPPORT
+P:	Akinobu Mita
+M:	akinobu.mita@gmail.com
+S:	Supported
+
 FRAMEBUFFER LAYER
 P:	Antonino Daplas
 M:	adaplas@gmail.com

Re: [PATCH] fault injection: fix failslab with CONFIG_NUMA

[Pekka J Enberg]

On Sun, 22 Apr 2007, Akinobu Mita wrote:
> Currently failslab injects failures into ____cache_alloc().
> But with enabling CONFIG_NUMA it's not enough to let actual
> slab allocator functions (kmalloc, kmem_cache_alloc, ...) return NULL.
> 
> This patch moves fault injection hook inside of __cache_alloc() and
> __cache_alloc_node(). These are lower call path than ____cache_alloc()
> and enable to inject faulures to slab allocators with CONFIG_NUMA.

Looks good to me.

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>