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

This is a discussion on [RFC][PATCH] x86: don't destroy %rbp on kernel-mode faults - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 5 of 5

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

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

    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
    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
    Cc: Ingo Molnar
    Cc: Arjan van de Ven
    Signed-off-by: Pekka Enberg
    Signed-off-by: Vegard Nossum
    ---
    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    > 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
    > Cc: Ingo Molnar
    > Cc: Arjan van de Ven
    > Signed-off-by: Pekka Enberg
    > Signed-off-by: Vegard Nossum


    looks good to me; good debugging!

    Acked-by: Arjan van de Ven
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, May 19, 2008 at 11:16 PM, Andi Kleen 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > 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)


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread