Re: WARNING: can't dereference registers at ffffc90004dfff60 for ip error_entry+0x7d/0xd0 (Re: [PATCH v2 00/18] Entry stack switching)

[Andy Lutomirski]

On Wed, Nov 22, 2017 at 5:55 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Wed, Nov 22, 2017 at 08:39:07AM +0100, Ingo Molnar wrote:
>>
>> * Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>>
>> > This sets up stack switching, including for SYSCALL.  I think it's
>> > in decent shape.
>> >
>> > Known issues:
>> >  - I think we're going to want a way to turn the stack switching on and
>> >    off either at boot time or at runtime.  It should be fairly straightforward
>> >    to make it work.
>> >
>> >  - I think the ORC unwinder isn't so good at dealing with stack overflows.
>> >    It bails too early (I think), resulting in lots of ? entries.  This
>> >    isn't a regression with this series -- it's just something that could
>> >    be improved.
>>
>> Note that with the attached config on an Intel testbox I get the following new ORC
>> unwinder warning during bootup:
>>
>> [   12.200554] calling  ghash_pclmulqdqni_mod_init+0x0/0x54 @ 1
>> [   12.209536] WARNING: can't dereference registers at ffffc90004dfff60 for ip error_entry+0x7d/0xd0
>> [   12.231388] initcall ghash_pclmulqdqni_mod_init+0x0/0x54 returned 0 after 23480 usecs
>
> After the stack switch in error_entry(), the pt_regs are at a different
> offset than before, so they aren't where ORC expects them to be.
>
> I think the below patch should fix it, by popping the return address off
> the stack instead of just copying it.  This way we can avoid adding
> another ORC annotation because pt_regs will be at the same offset both
> before and after the sync_regs() call.
>
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 2fdd2127e8e9..8dad83724469 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1291,8 +1291,8 @@ ENTRY(error_entry)
>
>  .Lerror_entry_from_usermode_after_swapgs:
>         /* Put us onto the real thread stack. */
> -       leaq    8(%rsp), %rdi                   /* pt_regs pointer */
> -       movq    (%rsp), %r12
> +       popq    %r12                            /* function return address */
> +       leaq    (%rsp), %rdi                    /* pt_regs pointer */
>         call    sync_regs
>         movq    %rax, %rsp                      /* switch stack */
>         ENCODE_FRAME_POINTER

Ah, I see -- SAVE_C_REGS contains an UNWIND_HINT_REGS hint.  I had
thought I needed to fix it differently.

Ingo, if you want to apply this, I attached it.  I'll fold it in next
time I resend the whole  series.
From e4b840c5a2eee3610d870703d484263226df80cc Mon Sep 17 00:00:00 2001
Message-Id: <e4b840c5a2eee3610d870703d484263226df80cc.1511363929.git.luto@xxxxxxxxxx>
From: Andy Lutomirski <luto@xxxxxxxxxx>
Date: Wed, 22 Nov 2017 07:15:51 -0800
Subject: [PATCH] Fixup for "x86/asm/64: Use a percpu trampoline stack for IDT
 entries"

One of my sync_regs calls was confusing objtool.  I'll fold this in
to the series next time I refresh it.

Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
---
 arch/x86/entry/entry_64.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 2fdd2127e8e9..f1cef194dfba 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1291,8 +1291,8 @@ ENTRY(error_entry)
 
 .Lerror_entry_from_usermode_after_swapgs:
 	/* Put us onto the real thread stack. */
-	leaq	8(%rsp), %rdi			/* pt_regs pointer */
-	movq	(%rsp), %r12
+	popq	%r12				/* save return addr in %12 */
+	movq	%rsp, %rdi			/* arg0 = pt_regs pointer */
 	call	sync_regs
 	movq	%rax, %rsp			/* switch stack */
 	ENCODE_FRAME_POINTER
-- 
2.13.6