Suspend-to-ram/resume regression with commit ca37e57bbe0c

Suspend-to-ram/resume regression with commit ca37e57bbe0c

[Jarkko Nikula]

Hi

Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it 
to commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to 
native_load_gs_index()").

I noticed it on Intel Kabylake (core) and Apollolake (atom) based 
prototype machines. Symptoms are that machine appears to enter into 
suspend but resumes instantly and hangs. Unfortunately no logs.

If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.

-- 
Jarkko

[PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Ingo Molnar]

* Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Hi
> 
> Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it to
> commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
> native_load_gs_index()").
> 
> I noticed it on Intel Kabylake (core) and Apollolake (atom) based prototype
> machines. Symptoms are that machine appears to enter into suspend but
> resumes instantly and hangs. Unfortunately no logs.
> 
> If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.

Hm, that commit looks broken with irq-tracing enabled.
Does the patch below fix it?

In fact the exception handler itself appears to have broken GS handling as well - 
I suspect it never triggers in practice, because it was broken forever.

Andy, do you concur?

On a related note, we should definitely extend the 'intended GS state' annotation 
comments I did in this patch to all SWAPGS instances - this way code review has a 
much higher chance of finding discrepancies between intent and actual code.

Thanks,

	Ingo

=================>
>From 769dbd33a272214c48c0fc5a17bed9c1597e222f Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 29 Nov 2017 07:43:27 +0100
Subject: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

Jarkko Nikula reported a S2R resume hang regression and bisected it back to:

  ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")

Turns out the GS handling of that patch is wrong: when IRQ state tracing is
enabled it calls a kernel function (as part of the TRACE_IRQS_*() functionality),
but we have not switched to the kernel GS yet ...

Fix the SWAPGS handling and also annotate every affected SWAPGS
instance to document the intended state of GS.

Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Bisected-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/0fede9f9-88b0-a6e7-1027-dfb2019b8ef2@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f81d50d7ceac..c0b52df8ee4f 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -945,16 +945,16 @@ idtentry simd_coprocessor_error		do_simd_coprocessor_error	has_error_code=0
 	 */
 ENTRY(native_load_gs_index)
 	FRAME_BEGIN
+	SWAPGS					/* switch from user GS to kernel GS */
 	pushfq
 	DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
 	TRACE_IRQS_OFF
-	SWAPGS
 .Lgs_change:
 	movl	%edi, %gs
 2:	ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
-	SWAPGS
 	TRACE_IRQS_FLAGS (%rsp)
 	popfq
+	SWAPGS					/* switch from kernel GS to user GS */
 	FRAME_END
 	ret
 ENDPROC(native_load_gs_index)
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(native_load_gs_index)
 	.section .fixup, "ax"
 	/* running with kernelgs */
 bad_gs:
-	SWAPGS					/* switch back to user gs */
+	SWAPGS					/* switch back to user GS, to modify GS */
 .macro ZAP_GS
 	/* This can't be a string because the preprocessor needs to see it. */
 	movl $__USER_DS, %eax
@@ -973,6 +973,7 @@ EXPORT_SYMBOL(native_load_gs_index)
 	ALTERNATIVE "", "ZAP_GS", X86_BUG_NULL_SEG
 	xorl	%eax, %eax
 	movl	%eax, %gs
+	SWAPGS					/* switch to kernel GS again before continuing */
 	jmp	2b
 	.previous
 

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Jarkko Nikula]

[-- Attachment #1: Type: text/plain, Size: 3335 bytes --]

On 11/29/2017 09:09 AM, Ingo Molnar wrote:
> 
> * Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> 
>> Hi
>> 
>> Suspend-to-ram and resume stopped working on v4.15-rc1 and I 
>> bisected it to commit ca37e57bbe0c ("x86/entry/64: Add missing 
>> irqflags tracing to native_load_gs_index()").
>> 
>> I noticed it on Intel Kabylake (core) and Apollolake (atom) based 
>> prototype machines. Symptoms are that machine appears to enter
>> into suspend but resumes instantly and hangs. Unfortunately no
>> logs.
>> 
>> If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.
> 
> Hm, that commit looks broken with irq-tracing enabled. Does the
> patch below fix it?
> 
No, it makes the machine not to boot at all :-(

Log below when I used my config (now attached). With x86_64_defconfig it 
booted twice but didn't survive suspend/resume. However several other 
boot attempts with x86_64_defconfig failed somewhat similarly. Not in 
the same place but hanging anyway. With my own config it seems to always 
end up failing in trace_hardirqs_off_caller.

Then I noticed suspend/resume is not working on v4.14 either when I use 
the x86_64_defconfig. Maybe unrelated issue.

...
[    1.917851] clocksource: tsc: mask: 0xffffffffffffffff max_cycles:
0x19f2297dd97, max_idle_ns: 440795236593 ns
[    1.929795] workingset: timestamp_bits=62 max_order=21 bucket_order=0
[    1.939699] BUG: stack guard page was hit at ffffc90000233ff8 (stack
is ffffc90000234000..ffffc90000237fff)
** 606 printk messages dropped **
[    1.940339]  ? native_iret+0x7/0x7
[    1.940340]  ? error_entry+0x6f/0xc0
[    1.940341]  error_entry+0x6f/0xc0
[    1.940342] RIP: 0010:trace_hardirqs_off_caller+0x8/0xc0
[    1.940342] RSP: 0000:ffffc90000257048 EFLAGS: 00010093 ORIG_RAX:
0000000000000000
[    1.940343] RAX: 000000008168ad47 RBX: 0000000000000001 RCX:
ffffffff8168ad47
[    1.940343] RDX: ffff8802b505bb28 RSI: 0000000000000000 RDI:
ffffffff8168b5ef
[    1.940344] RBP: ffffc900002570a0 R08: 00000000ebddc52a R09:
0000000000000001
[    1.940344] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8802b505a840
[    1.940345] R13: ffff8802b505b048 R14: ffff8802b39dc800 R15:
ffff8802b505a840
[    1.940346]  ? page_fault+0xc/0x30
[    1.940347]  ? native_iret+0x7/0x7
[    1.940348]  ? error_entry+0x6f/0xc0
[    1.940349]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[    1.940350]  ? native_iret+0x7/0x7
[    1.940351]  ? error_entry+0x6f/0xc0
[    1.940352]  error_entry+0x6f/0xc0
[    1.940353] RIP: 0010:trace_hardirqs_off_caller+0x8/0xc0
[    1.940353] RSP: 0000:ffffc90000257168 EFLAGS: 00010093 ORIG_RAX:
0000000000000000
[    1.940354] RAX: 000000008168ad47 RBX: 0000000000000001 RCX:
ffffffff8168ad47
[    1.940354] RDX: ffff8802b505bb28 RSI: 0000000000000000 RDI:
ffffffff8168b5ef
[    1.940355] RBP: ffffc900002571c0 R08: 00000000ebddc52a R09:
0000000000000001
[    1.940355] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8802b505a840
[    1.940356] R13: ffff8802b505b048 R14: ffff8802b39dc800 R15:
ffff8802b505a840
[    1.940357]  ? page_fault+0xc/0x30
[    1.940358]  ? native_iret+0x7/0x7
[    1.940359]  ? error_entry+0x6f/0xc0
[    1.940360]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[    1.940361]  ? native_iret+0x7/0x7
[    1.940362]  ? error_entry+0x6f/0xc0
[    1.940363]  error_entry+0x6f/0xc0

and this continues.

-- 
Jarkko


[-- Attachment #2: x86_64-4.15.config.gz --]
[-- Type: application/gzip, Size: 26293 bytes --]

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Thomas Gleixner]

On Wed, 29 Nov 2017, Jarkko Nikula wrote:

> On 11/29/2017 09:09 AM, Ingo Molnar wrote:
> > 
> > * Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> > 
> > > Hi
> > > 
> > > Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it
> > > to commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
> > > native_load_gs_index()").
> > > 
> > > I noticed it on Intel Kabylake (core) and Apollolake (atom) based
> > > prototype machines. Symptoms are that machine appears to enter
> > > into suspend but resumes instantly and hangs. Unfortunately no
> > > logs.
> > > 
> > > If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.
> > 
> > Hm, that commit looks broken with irq-tracing enabled. Does the
> > patch below fix it?
> > 
> No, it makes the machine not to boot at all :-(
> 
> Log below when I used my config (now attached). With x86_64_defconfig it
> booted twice but didn't survive suspend/resume. However several other boot
> attempts with x86_64_defconfig failed somewhat similarly. Not in the same
> place but hanging anyway. With my own config it seems to always end up failing
> in trace_hardirqs_off_caller.

Does it work when you disable all the tracing muck?

Thanks,

	tglx

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Jarkko Nikula]

On 11/29/2017 11:28 AM, Thomas Gleixner wrote:
> On Wed, 29 Nov 2017, Jarkko Nikula wrote:
> 
>> On 11/29/2017 09:09 AM, Ingo Molnar wrote:
>>> Hm, that commit looks broken with irq-tracing enabled. Does the
>>> patch below fix it?
>>>
>> No, it makes the machine not to boot at all :-(
>>
>> Log below when I used my config (now attached). With x86_64_defconfig it
>> booted twice but didn't survive suspend/resume. However several other boot
>> attempts with x86_64_defconfig failed somewhat similarly. Not in the same
>> place but hanging anyway. With my own config it seems to always end up failing
>> in trace_hardirqs_off_caller.
> 
> Does it work when you disable all the tracing muck?
> 
My config seems to work with Ingo's patch if I disable both 
CONFIG_FTRACE and CONFIG_PROVE_LOCKING which disables the 
CONFIG_TRACE_IRQS_FLAGS.

x86_64_defconfig with CONFIG_FTRACE disabled is still unstable. 
Sometimes hangs during bootup and sometimes boots up to userspace.

-- 
Jarkko

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Peter Zijlstra]

On Wed, Nov 29, 2017 at 08:09:51AM +0100, Ingo Molnar wrote:
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index f81d50d7ceac..c0b52df8ee4f 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error		do_simd_coprocessor_error	has_error_code=0
>  	 */
>  ENTRY(native_load_gs_index)
>  	FRAME_BEGIN
> +	SWAPGS					/* switch from user GS to kernel GS */
>  	pushfq
>  	DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>  	TRACE_IRQS_OFF
> -	SWAPGS

I'm thinking those moves it too far back; we should at least have
interrupts disabled when we do SWAPGS, no? Also, curse paravirt.

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

> On Nov 29, 2017, at 4:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Wed, Nov 29, 2017 at 08:09:51AM +0100, Ingo Molnar wrote:
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index f81d50d7ceac..c0b52df8ee4f 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error        do_simd_coprocessor_error    has_error_code=0
>>     */
>> ENTRY(native_load_gs_index)
>>    FRAME_BEGIN
>> +    SWAPGS                    /* switch from user GS to kernel GS */
>>    pushfq
>>    DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>>    TRACE_IRQS_OFF
>> -    SWAPGS
> 
> I'm thinking those moves it too far back; we should at least have
> interrupts disabled when we do SWAPGS, no? Also, curse paravirt.
> 

I'll look in a few hours.

But we definitely can't have irqs on when running with user gs.

RE: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[David Laight]

From: Andy Lutomirski
> Sent: 29 November 2017 14:34
> > On Nov 29, 2017, at 4:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >> On Wed, Nov 29, 2017 at 08:09:51AM +0100, Ingo Molnar wrote:
> >> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> >> index f81d50d7ceac..c0b52df8ee4f 100644
> >> --- a/arch/x86/entry/entry_64.S
> >> +++ b/arch/x86/entry/entry_64.S
> >> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error        do_simd_coprocessor_error
> has_error_code=0
> >>     */
> >> ENTRY(native_load_gs_index)
> >>    FRAME_BEGIN
> >> +    SWAPGS                    /* switch from user GS to kernel GS */
> >>    pushfq
> >>    DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
> >>    TRACE_IRQS_OFF
> >> -    SWAPGS
> >
> > I'm thinking those moves it too far back; we should at least have
> > interrupts disabled when we do SWAPGS, no? Also, curse paravirt.
> >
> 
> I'll look in a few hours.
> 
> But we definitely can't have irqs on when running with user gs.

I can't remember what happens when swapgs itself faults.
I'm pretty sure it can, restoring %ds %es and %fs can definitely fault
(especially for 32 bit apps) if the restored values are invalid.
With user LDT I think it is possible for a valid segment register
to become invalid while a process is sleeping.
The debugger (and possible a signal handler) can set the segment
registers to arbitrary values - so loading them faults.

I fixed NetBSD a few years ago so that all these faults were handled
correctly.

	David

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

On Wed, Nov 29, 2017 at 6:56 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Andy Lutomirski
>> Sent: 29 November 2017 14:34
>> > On Nov 29, 2017, at 4:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> >
>> >> On Wed, Nov 29, 2017 at 08:09:51AM +0100, Ingo Molnar wrote:
>> >> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> >> index f81d50d7ceac..c0b52df8ee4f 100644
>> >> --- a/arch/x86/entry/entry_64.S
>> >> +++ b/arch/x86/entry/entry_64.S
>> >> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error        do_simd_coprocessor_error
>> has_error_code=0
>> >>     */
>> >> ENTRY(native_load_gs_index)
>> >>    FRAME_BEGIN
>> >> +    SWAPGS                    /* switch from user GS to kernel GS */
>> >>    pushfq
>> >>    DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>> >>    TRACE_IRQS_OFF
>> >> -    SWAPGS
>> >
>> > I'm thinking those moves it too far back; we should at least have
>> > interrupts disabled when we do SWAPGS, no? Also, curse paravirt.
>> >
>>
>> I'll look in a few hours.
>>
>> But we definitely can't have irqs on when running with user gs.
>
> I can't remember what happens when swapgs itself faults.
> I'm pretty sure it can, restoring %ds %es and %fs can definitely fault
> (especially for 32 bit apps) if the restored values are invalid.
> With user LDT I think it is possible for a valid segment register
> to become invalid while a process is sleeping.
> The debugger (and possible a signal handler) can set the segment
> registers to arbitrary values - so loading them faults.
>
> I fixed NetBSD a few years ago so that all these faults were handled
> correctly.

SWAPGS never faults.

I did test my patch, and I'm wondering how I missed the problem.  Looking now...

RE: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[David Laight]

From: Andy Lutomirski
> Sent: 29 November 2017 16:23
> > I can't remember what happens when swapgs itself faults.
...
> SWAPGS never faults.

Ah yes, I remember, it only restores the offset.
For 32bit processes you need to do a 'pop %gs' to recover
the segment register itself - and that can fault.

SWAPGS is a PITA. The hardware designers should have tried to write
all the interrupt handling code.

	David

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Linus Torvalds]

On Wed, Nov 29, 2017 at 8:51 AM, David Laight <David.Laight@aculab.com> wrote:
>
> SWAPGS is a PITA. The hardware designers should have tried to write
> all the interrupt handling code.

That's actually my biggest beef with x86-64. The kernel entry/exit is
completely misdesigned.

That was actually better in the original i386, which handles nesting
ok (modulo the NMI disable flag and the STI shadow bit which doesn't
save/restore properly).

Yes, the 386 model of infinite indirection through IDT/GDT/TSS is
broken and should have had a mode bit to just replace with a simple
"use this stack and address for kernel entry" MSR register - but that
simplified kernel entry/exit should have saved/restored _more_ info,
not less (eg "save/restore segment shadow state rather than the
descriptor number that needs the insane indirection" etc)

The problem was never the few push/pop instructions that kernel entry involved.

The entry/exit garbage admittedly started before x86-64 itself - all
those 'syscall' variants are equally broken. Not saving/restoring
state properly is just unbelievable sh*t. x86-64 then made things
worse with SWAPGS etc.

A lot of people hate x86 because of instruction decoding. No, if you
need a reason to dislike x86, it's because of exception handling and
iret.

Rant over.

                Linus

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

On Wed, Nov 29, 2017 at 8:22 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, Nov 29, 2017 at 6:56 AM, David Laight <David.Laight@aculab.com> wrote:
>> From: Andy Lutomirski
>>> Sent: 29 November 2017 14:34
>>> > On Nov 29, 2017, at 4:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>> >
>>> >> On Wed, Nov 29, 2017 at 08:09:51AM +0100, Ingo Molnar wrote:
>>> >> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>>> >> index f81d50d7ceac..c0b52df8ee4f 100644
>>> >> --- a/arch/x86/entry/entry_64.S
>>> >> +++ b/arch/x86/entry/entry_64.S
>>> >> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error        do_simd_coprocessor_error
>>> has_error_code=0
>>> >>     */
>>> >> ENTRY(native_load_gs_index)
>>> >>    FRAME_BEGIN
>>> >> +    SWAPGS                    /* switch from user GS to kernel GS */
>>> >>    pushfq
>>> >>    DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>>> >>    TRACE_IRQS_OFF
>>> >> -    SWAPGS
>>> >
>>> > I'm thinking those moves it too far back; we should at least have
>>> > interrupts disabled when we do SWAPGS, no? Also, curse paravirt.
>>> >
>>>
>>> I'll look in a few hours.
>>>
>>> But we definitely can't have irqs on when running with user gs.
>>
>> I can't remember what happens when swapgs itself faults.
>> I'm pretty sure it can, restoring %ds %es and %fs can definitely fault
>> (especially for 32 bit apps) if the restored values are invalid.
>> With user LDT I think it is possible for a valid segment register
>> to become invalid while a process is sleeping.
>> The debugger (and possible a signal handler) can set the segment
>> registers to arbitrary values - so loading them faults.
>>
>> I fixed NetBSD a few years ago so that all these faults were handled
>> correctly.
>
> SWAPGS never faults.
>
> I did test my patch, and I'm wondering how I missed the problem.  Looking now...

I think my patch was entirely correct and Jarkko's problem is some
other bug.  Lemme see if I can find it.

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

On Tue, Nov 28, 2017 at 11:09 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>
>> Hi
>>
>> Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it to
>> commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
>> native_load_gs_index()").
>>
>> I noticed it on Intel Kabylake (core) and Apollolake (atom) based prototype
>> machines. Symptoms are that machine appears to enter into suspend but
>> resumes instantly and hangs. Unfortunately no logs.
>>
>> If I revert ca37e57bbe0c on v4.15-rc1 it works as expected.
>
> Hm, that commit looks broken with irq-tracing enabled.
> Does the patch below fix it?
>
> In fact the exception handler itself appears to have broken GS handling as well -
> I suspect it never triggers in practice, because it was broken forever.
>
> Andy, do you concur?

No.

>
> On a related note, we should definitely extend the 'intended GS state' annotation
> comments I did in this patch to all SWAPGS instances - this way code review has a
> much higher chance of finding discrepancies between intent and actual code.

Agreed.  I'll send a patch.


> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -945,16 +945,16 @@ idtentry simd_coprocessor_error           do_simd_coprocessor_error       has_error_code=0
>          */
>  ENTRY(native_load_gs_index)
>         FRAME_BEGIN
> +       SWAPGS                                  /* switch from user GS to kernel GS */

No, we start with kernel GS.  It was correct before.

>         pushfq
>         DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>         TRACE_IRQS_OFF
> -       SWAPGS
>  .Lgs_change:
>         movl    %edi, %gs
>  2:     ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
> -       SWAPGS
>         TRACE_IRQS_FLAGS (%rsp)
>         popfq
> +       SWAPGS                                  /* switch from kernel GS to user GS */
>         FRAME_END
>         ret
>  ENDPROC(native_load_gs_index)
> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(native_load_gs_index)
>         .section .fixup, "ax"
>         /* running with kernelgs */
>  bad_gs:
> -       SWAPGS                                  /* switch back to user gs */
> +       SWAPGS                                  /* switch back to user GS, to modify GS */
>  .macro ZAP_GS
>         /* This can't be a string because the preprocessor needs to see it. */
>         movl $__USER_DS, %eax
> @@ -973,6 +973,7 @@ EXPORT_SYMBOL(native_load_gs_index)
>         ALTERNATIVE "", "ZAP_GS", X86_BUG_NULL_SEG
>         xorl    %eax, %eax
>         movl    %eax, %gs
> +       SWAPGS                                  /* switch to kernel GS again before continuing */

Which we don't want to do because the landing site expects user GS.

I suspect we're hitting an entirely different bug, that we're blowing
up if we WARN too early in resume.

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Peter Zijlstra]

On Wed, Nov 29, 2017 at 09:57:43AM -0800, Andy Lutomirski wrote:
> On Tue, Nov 28, 2017 at 11:09 PM, Ingo Molnar <mingo@kernel.org> wrote:

> > On a related note, we should definitely extend the 'intended GS state' annotation
> > comments I did in this patch to all SWAPGS instances - this way code review has a
> > much higher chance of finding discrepancies between intent and actual code.
> 
> Agreed.  I'll send a patch.

Alternatively we could do something like:

  SWITCH_TO_KERNEL_GS
  SWITCH_TO_USER_GS

and have them do the SWAPGS with an (optional) assertion of the actual
GS.

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Wed, Nov 29, 2017 at 9:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Nov 28, 2017 at 11:09 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>>
>>> Hi
>>>
>>> Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it to
>>> commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
>>> native_load_gs_index()").
>>>

Jarkko, can you try the attached patch?  If it survives resume, can
you see if the log contains anything interesting?

[-- Attachment #2: gsindex.patch --]
[-- Type: text/x-patch, Size: 840 bytes --]

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 78c61c8ecfa8..ca5d978b86a0 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -245,14 +245,17 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	asm volatile ("movw %0, %%ds" :: "r" (ctxt->ds));
 	asm volatile ("movw %0, %%es" :: "r" (ctxt->es));
 	asm volatile ("movw %0, %%fs" :: "r" (ctxt->fs));
-	load_gs_index(0xffff);
-	WARN_ON(1);
-	load_gs_index(ctxt->gs);
 	asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
 
 	wrmsrl(MSR_FS_BASE, ctxt->fs_base);
 	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
 	wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
+
+	/*
+	 * load_gs_index() potentially calls normal C code (irqflag tracing),
+	 * so it needs GSBASE restored first.
+	 */
+	load_gs_index(ctxt->gs);
 #endif
 
 	fix_processor_context();

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Linus Torvalds]

On Wed, Nov 29, 2017 at 10:12 AM, Andy Lutomirski <luto@kernel.org> wrote:
>
> Jarkko, can you try the attached patch?  If it survives resume, can
> you see if the log contains anything interesting?

I'm not Jarkko, but I'm not a huge fan of that patch.

If this was the cause of the problem (and it looks likely), wouldn't
it be nicer to instead make sure that __restore_processor_state() is
made to use only low-level code and easy to verify?

That function is already marked "notrace" because it is so fragile,
and it does the segment register reloads manually with inline asms.

Could we make it use "native_load_gs_index()" instead? Or even go all
the way and make it do that user-space %gs load internally with inline
asm, the way it already does the kernel space %gs?

(Maybe "native_wrmsrl()" too?)

Or is this actually all supposed to work even under PV? That sounds really iffy.

               Linus

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

> On Nov 29, 2017, at 12:58 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Wed, Nov 29, 2017 at 10:12 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> 
>> Jarkko, can you try the attached patch?  If it survives resume, can
>> you see if the log contains anything interesting?
> 
> I'm not Jarkko, but I'm not a huge fan of that patch.
> 
> If this was the cause of the problem (and it looks likely), wouldn't
> it be nicer to instead make sure that __restore_processor_state() is
> made to use only low-level code and easy to verify?
> 
> That function is already marked "notrace" because it is so fragile,
> and it does the segment register reloads manually with inline asms.

I completely agree, and I think it might be better to move more of that crap to asm.  Also, it looks quite buggy -- it restores segment registers before it loads the LDT, so they had better not be user registers.

Or we could load fixed values into the segment regs if they're not user values.

> 
> Could we make it use "native_load_gs_index()" instead? Or even go all
> the way and make it do that user-space %gs load internally with inline
> asm, the way it already does the kernel space %gs?

Dunno.  If we need the exception handling, it can't be inlined.

Anyway, this wasn't meant to be an upstreamable fix.  It was meant to make sure the problem I'm fixing is the right problem.

> 
> (Maybe "native_wrmsrl()" too?)
> 
> Or is this actually all supposed to work even under PV? That sounds really iffy.
> 
>               Linus

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

On Wed, Nov 29, 2017 at 1:25 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>> On Nov 29, 2017, at 12:58 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>>> On Wed, Nov 29, 2017 at 10:12 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>>
>>> Jarkko, can you try the attached patch?  If it survives resume, can
>>> you see if the log contains anything interesting?
>>
>> I'm not Jarkko, but I'm not a huge fan of that patch.
>>
>> If this was the cause of the problem (and it looks likely), wouldn't
>> it be nicer to instead make sure that __restore_processor_state() is
>> made to use only low-level code and easy to verify?
>>
>> That function is already marked "notrace" because it is so fragile,
>> and it does the segment register reloads manually with inline asms.
>
> I completely agree, and I think it might be better to move more of that crap to asm.  Also, it looks quite buggy -- it restores segment registers before it loads the LDT, so they had better not be user registers.

It does indeed restore user state.  And it very well may need to work
on Xen PV, too.  Blech.

>
> Or we could load fixed values into the segment regs if they're not user values.
>
>>
>> Could we make it use "native_load_gs_index()" instead? Or even go all
>> the way and make it do that user-space %gs load internally with inline
>> asm, the way it already does the kernel space %gs?
>
> Dunno.  If we need the exception handling, it can't be inlined.
>
> Anyway, this wasn't meant to be an upstreamable fix.  It was meant to make sure the problem I'm fixing is the right problem.
>
>>
>> (Maybe "native_wrmsrl()" too?)
>>
>> Or is this actually all supposed to work even under PV? That sounds really iffy.
>>
>>               Linus

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Andy Lutomirski]

On Wed, Nov 29, 2017 at 1:41 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, Nov 29, 2017 at 1:25 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>
>>> On Nov 29, 2017, at 12:58 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Wed, Nov 29, 2017 at 10:12 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>>>
>>>> Jarkko, can you try the attached patch?  If it survives resume, can
>>>> you see if the log contains anything interesting?
>>>
>>> I'm not Jarkko, but I'm not a huge fan of that patch.
>>>
>>> If this was the cause of the problem (and it looks likely), wouldn't
>>> it be nicer to instead make sure that __restore_processor_state() is
>>> made to use only low-level code and easy to verify?
>>>
>>> That function is already marked "notrace" because it is so fragile,
>>> and it does the segment register reloads manually with inline asms.
>>
>> I completely agree, and I think it might be better to move more of that crap to asm.  Also, it looks quite buggy -- it restores segment registers before it loads the LDT, so they had better not be user registers.
>
> It does indeed restore user state.  And it very well may need to work
> on Xen PV, too.  Blech.

I took another look.  This function is severely busted.  Bugs include:

- 32-bit fails to save and restore %ds.  I have no idea why.

 - 64-bit has inlined restores of segment regs.  This is busted
because it's missing exception handling.  Admittedly, there shouldn't
be exceptions these days since we try pretty hard to keep segments in
sync.

 - GSBASE is restored way too late.

 - Segments are restored before the LDT is restored.  *Boom* if we
write to /sys/power/state from a program compiled with a sufficiently
ancient libc.

 - Because we aren't sensible enough to do all this from a kernel
thread, we probably fail to correctly handle nasty things like
blockstep.

I'll make a patch to fix a few of these bugs.

Re: [PATCH] x86/entry/64: Fix native_load_gs_index() SWAPGS handling with IRQ state tracing enabled

[Jarkko Nikula]

On 11/29/2017 08:12 PM, Andy Lutomirski wrote:
> On Wed, Nov 29, 2017 at 9:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Tue, Nov 28, 2017 at 11:09 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>> * Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>>>
>>>> Hi
>>>>
>>>> Suspend-to-ram and resume stopped working on v4.15-rc1 and I bisected it to
>>>> commit ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to
>>>> native_load_gs_index()").
>>>>
> 
> Jarkko, can you try the attached patch?  If it survives resume, can
> you see if the log contains anything interesting?
> 
Hmm.. this doesn't apply on top of v4.15-rc1, ca37e57bbe0c, current head 
a0908a1b7d68 or linux-next head 11fed7829bef.

-- 
Jarkko