[RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[Vegard Nossum]

Hi,

The RFC part of this patch is: Does anybody see why touching %rcx would
be bad? It certainly looks like %ecx is free. This fixes the stacktrace
problem I was seeing, and Pekka tested a bootup to userspace. (Pekka also
did half of the debugging. When will git allow multiple authors for a
patch? :-))


Vegard


>From b1cbf24fcd05aa5ed2e610c80c06bc519d3188f7 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Mon, 19 May 2008 21:39:44 +0200
Subject: [PATCH] x86: don't destroy %rbp on kernel-mode faults

>From the code:

    B stepping K8s sometimes report an truncated RIP for IRET exceptions
    returning to compat mode. Check for these here too.

The code then proceeds to truncate the upper 32 bits of %rbp. This means
that when do_page_fault() is finally called, its prologue,

    do_page_fault:
        push %rbp
        movl %rsp, %rbp

will put the truncated base pointer on the stack. This means that the
stack tracer will not be able to follow the base-pointer changes and
will see all subsequent stack frames as unreliable.

This patch changes the code to use a different register (%rcx) for the
checking and leaves %rbp untouched.

Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 arch/x86/kernel/entry_64.S |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 1edd9ac..ff53692 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -926,11 +926,11 @@ error_kernelspace:
 	   iret run with kernel gs again, so don't set the user space flag.
 	   B stepping K8s sometimes report an truncated RIP for IRET 
 	   exceptions returning to compat mode. Check for these here too. */
-	leaq irq_return(%rip),%rbp
-	cmpq %rbp,RIP(%rsp) 
+	leaq irq_return(%rip),%rcx
+	cmpq %rcx,RIP(%rsp) 
 	je   error_swapgs
-	movl %ebp,%ebp	/* zero extend */
-	cmpq %rbp,RIP(%rsp) 
+	movl %ecx,%ecx	/* zero extend */
+	cmpq %rcx,RIP(%rsp) 
 	je   error_swapgs
 	cmpq $gs_change,RIP(%rsp)
         je   error_swapgs
-- 
1.5.4.1

Re: [RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[Arjan van de Ven]

Vegard Nossum wrote:
> Hi,
> 
> The RFC part of this patch is: Does anybody see why touching %rcx would
> be bad? It certainly looks like %ecx is free. This fixes the stacktrace
> problem I was seeing, and Pekka tested a bootup to userspace. (Pekka also
> did half of the debugging. When will git allow multiple authors for a
> patch? :-))
> 
> 
> Vegard
> 
> 
> From b1cbf24fcd05aa5ed2e610c80c06bc519d3188f7 Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@gmail.com>
> Date: Mon, 19 May 2008 21:39:44 +0200
> Subject: [PATCH] x86: don't destroy %rbp on kernel-mode faults
> 
> From the code:
> 
>     B stepping K8s sometimes report an truncated RIP for IRET exceptions
>     returning to compat mode. Check for these here too.
> 
> The code then proceeds to truncate the upper 32 bits of %rbp. This means
> that when do_page_fault() is finally called, its prologue,
> 
>     do_page_fault:
>         push %rbp
>         movl %rsp, %rbp
> 
> will put the truncated base pointer on the stack. This means that the
> stack tracer will not be able to follow the base-pointer changes and
> will see all subsequent stack frames as unreliable.
> 
> This patch changes the code to use a different register (%rcx) for the
> checking and leaves %rbp untouched.
> 
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>

looks good to me; good debugging!

Acked-by: Arjan van de Ven <arjan@linux.intel.com>

Re: [RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[Andi Kleen]

Vegard Nossum wrote:
> Hi,
> 
> The RFC part of this patch is: Does anybody see why touching %rcx would
> be bad? It certainly looks like %ecx is free. This fixes the stacktrace
> problem I was seeing, and Pekka tested a bootup to userspace. (Pekka also
> did half of the debugging. When will git allow multiple authors for a
> patch? :-))

The patch is ok, but I'm sure there's lots of other assembler code that
destroys %rbp when it was saved elsewhere.

When I wrote all the assembler the assumption was always that a real
unwinder would be used for backtraces, not frame pointer.

-Andi

Re: [RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[Vegard Nossum]

On Mon, May 19, 2008 at 11:16 PM, Andi Kleen <andi@firstfloor.org> wrote:
> Vegard Nossum wrote:
>> Hi,
>>
>> The RFC part of this patch is: Does anybody see why touching %rcx would
>> be bad? It certainly looks like %ecx is free. This fixes the stacktrace
>> problem I was seeing, and Pekka tested a bootup to userspace. (Pekka also
>> did half of the debugging. When will git allow multiple authors for a
>> patch? :-))
>
> The patch is ok, but I'm sure there's lots of other assembler code that
> destroys %rbp when it was saved elsewhere.

Thanks, The real intention of this code (you might have guessed it)
was to fix kmemcheck on 64-bit, and it did, so I'm happy. If we (or
others) hit another similar case, I'm sure we'll be able to fix those
too.

The problem seems to be that %rbp was never restored before it was
used again, and that's what I consider the real error in this case. I
changed it to use a different register for the temporary computation,
but restoring %rbp from wherever it was stored would also have been a
valid, albeit less efficient, solution.

> When I wrote all the assembler the assumption was always that a real
> unwinder would be used for backtraces, not frame pointer.

Hm, I am not sure exactly what a "real unwinder" would be. But I do
think it's fair to say that it is the assembly code in this case that
is violating the binary interface, and not the stack tracer code.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults

[Andi Kleen]

> Hm, I am not sure exactly what a "real unwinder" would be. But I do

A dwarf2 unwinder that doesn't require pipe line stalls on many
CPUs on each function entry point for setting up a frame.  

Instead of letting all code
maintain a frame at runtime the stack frames are described by 
an external unwind table that is then walked by the unwinder.

The unwinder was in for a short time, but 
Linus unfortunately removed it again because it took some time
to debug it in tree and he lost patience. I believe an updated
and stable version is available in the SUSE kernels.

> think it's fair to say that it is the assembly code in this case that
> is violating the binary interface, and not the stack tracer code.

There is a no binary interface for page faults (or other exceptions) 
except that "all registers must be restored in the end". They certainly don't
follow the normal ABI. 

Still the fix is good, just pointing out that you'll likely need
to change a lot more code to get the frame pointer fully supported
everywhere because it was all written under the explicit "no frame pointer" 
assumption.

-Andi (who still prefers the unwinder)