* Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()
2017-11-30 15:57 [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context() Andy Lutomirski
@ 2017-11-30 15:59 ` Andy Lutomirski
2017-11-30 16:00 ` Thomas Gleixner
2017-12-01 9:06 ` Jarkko Nikula
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Andy Lutomirski @ 2017-11-30 15:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: X86 ML, Jarkko Nikula, linux-kernel, Borislav Petkov,
Peter Zijlstra, Linus Torvalds
On Thu, Nov 30, 2017 at 7:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
> __restore_processor_context() had a couple of ordering bugs. It
> restored GSBASE after calling load_gs_index(), and the latter can
> call into tracing code. It also tried to restore segment registers
> before restoring the LDT, which is straight-up wrong.
Ingo, Thomas, if you apply this version, can you fix up the following
changelog bug:
> .seg_not_present = 1,
That should be = 0 above.
If I send a v2, I'll fix it.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()
2017-11-30 15:59 ` Andy Lutomirski
@ 2017-11-30 16:00 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2017-11-30 16:00 UTC (permalink / raw)
To: Andy Lutomirski
Cc: X86 ML, Jarkko Nikula, linux-kernel, Borislav Petkov,
Peter Zijlstra, Linus Torvalds
On Thu, 30 Nov 2017, Andy Lutomirski wrote:
> On Thu, Nov 30, 2017 at 7:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > __restore_processor_context() had a couple of ordering bugs. It
> > restored GSBASE after calling load_gs_index(), and the latter can
> > call into tracing code. It also tried to restore segment registers
> > before restoring the LDT, which is straight-up wrong.
>
> Ingo, Thomas, if you apply this version, can you fix up the following
> changelog bug:
>
> > .seg_not_present = 1,
>
> That should be = 0 above.
Sure. I'll wait for Jarkko to confirm.
> If I send a v2, I'll fix it.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()
2017-11-30 15:57 [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context() Andy Lutomirski
2017-11-30 15:59 ` Andy Lutomirski
@ 2017-12-01 9:06 ` Jarkko Nikula
2017-12-04 22:45 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski
2017-12-06 11:36 ` tip-bot for Andy Lutomirski
3 siblings, 0 replies; 6+ messages in thread
From: Jarkko Nikula @ 2017-12-01 9:06 UTC (permalink / raw)
To: Andy Lutomirski, x86
Cc: linux-kernel, Borislav Petkov, Peter Zijlstra, Linus Torvalds
On 11/30/2017 05:57 PM, Andy Lutomirski wrote:
> __restore_processor_context() had a couple of ordering bugs. It
> restored GSBASE after calling load_gs_index(), and the latter can
> call into tracing code. It also tried to restore segment registers
> before restoring the LDT, which is straight-up wrong.
>
> Reorder the code so that we restore GSBASE, then the descriptor
> tables, then the segments.
>
> This fixes two bugs. First, it fixes a regression that broke resume
> under certain configurations due to irqflag tracing in
> native_load_gs_index(). Second, it fixes resume when the userspace
> process that initiated suspect had funny segments. The latter can be
> reproduced by compiling this:
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * ldt_echo.c - Echo argv[1] while using an LDT segment
> */
>
> int main(int argc, char **argv)
> {
> int ret;
> size_t len;
> char *buf;
>
> const struct user_desc desc = {
> .entry_number = 0,
> .base_addr = 0,
> .limit = 0xfffff,
> .seg_32bit = 1,
> .contents = 0, /* Data, grow-up */
> .read_exec_only = 0,
> .limit_in_pages = 1,
> .seg_not_present = 1,
> .useable = 0
> };
>
> if (argc != 2)
> errx(1, "Usage: %s STRING", argv[0]);
>
> len = asprintf(&buf, "%s\n", argv[1]);
> if (len < 0)
> errx(1, "Out of memory");
>
> ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
> if (ret < -1)
> errno = -ret;
> if (ret)
> err(1, "modify_ldt");
>
> asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
> write(1, buf, len);
> return 0;
> }
>
> and running ldt_echo >/sys/power/mem
>
> Without the fix, the latter causes a triple fault on resume.
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> Jarkko, can you test this version?
>
> arch/x86/power/cpu.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
It does fix the suspend/resume issue I saw. Patch applied on top of
current head df8ba95c572a and a loop below completed fine on a few
machines where I tested it. All of them had issue with the ca37e57bbe0c.
for ((i=0;i<10;i++)); do rtcwake -s 5 -m mem; echo $i; done
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* [tip:x86/urgent] x86/power: Fix some ordering bugs in __restore_processor_context()
2017-11-30 15:57 [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context() Andy Lutomirski
2017-11-30 15:59 ` Andy Lutomirski
2017-12-01 9:06 ` Jarkko Nikula
@ 2017-12-04 22:45 ` tip-bot for Andy Lutomirski
2017-12-06 11:36 ` tip-bot for Andy Lutomirski
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Andy Lutomirski @ 2017-12-04 22:45 UTC (permalink / raw)
To: linux-tip-commits
Cc: hpa, tglx, bp, luto, jarkko.nikula, torvalds, linux-kernel,
mingo, a.p.zijlstra
Commit-ID: cdf577209aad4cdbe3455d3efa6cf631f838c55d
Gitweb: https://git.kernel.org/tip/cdf577209aad4cdbe3455d3efa6cf631f838c55d
Author: Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Nov 2017 07:57:57 -0800
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 4 Dec 2017 23:41:42 +0100
x86/power: Fix some ordering bugs in __restore_processor_context()
__restore_processor_context() had a couple of ordering bugs. It
restored GSBASE after calling load_gs_index(), and the latter can
call into tracing code. It also tried to restore segment registers
before restoring the LDT, which is straight-up wrong.
Reorder the code so that we restore GSBASE, then the descriptor
tables, then the segments.
This fixes two bugs. First, it fixes a regression that broke resume
under certain configurations due to irqflag tracing in
native_load_gs_index(). Second, it fixes resume when the userspace
process that initiated suspect had funny segments. The latter can be
reproduced by compiling this:
// SPDX-License-Identifier: GPL-2.0
/*
* ldt_echo.c - Echo argv[1] while using an LDT segment
*/
int main(int argc, char **argv)
{
int ret;
size_t len;
char *buf;
const struct user_desc desc = {
.entry_number = 0,
.base_addr = 0,
.limit = 0xfffff,
.seg_32bit = 1,
.contents = 0, /* Data, grow-up */
.read_exec_only = 0,
.limit_in_pages = 1,
.seg_not_present = 0,
.useable = 0
};
if (argc != 2)
errx(1, "Usage: %s STRING", argv[0]);
len = asprintf(&buf, "%s\n", argv[1]);
if (len < 0)
errx(1, "Out of memory");
ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
if (ret < -1)
errno = -ret;
if (ret)
err(1, "modify_ldt");
asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
write(1, buf, len);
return 0;
}
and running ldt_echo >/sys/power/mem
Without the fix, the latter causes a triple fault on resume.
Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lkml.kernel.org/r/6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org
---
arch/x86/power/cpu.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 84fcfde..5191de1 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -226,8 +226,20 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
load_idt((const struct desc_ptr *)&ctxt->idt_limit);
#endif
+#ifdef CONFIG_X86_64
/*
- * segment registers
+ * We need GSBASE restored before percpu access can work.
+ * percpu access can happen in exception handlers or in complicated
+ * helpers like load_gs_index().
+ */
+ wrmsrl(MSR_GS_BASE, ctxt->gs_base);
+#endif
+
+ fix_processor_context();
+
+ /*
+ * Restore segment registers. This happens after restoring the GDT
+ * and LDT, which happen in fix_processor_context().
*/
#ifdef CONFIG_X86_32
loadsegment(es, ctxt->es);
@@ -248,13 +260,14 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
load_gs_index(ctxt->gs);
asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
+ /*
+ * Restore FSBASE and user GSBASE after reloading the respective
+ * segment selectors.
+ */
wrmsrl(MSR_FS_BASE, ctxt->fs_base);
- wrmsrl(MSR_GS_BASE, ctxt->gs_base);
wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
#endif
- fix_processor_context();
-
do_fpu_end();
tsc_verify_tsc_adjust(true);
x86_platform.restore_sched_clock_state();
^ permalink raw reply related [flat|nested] 6+ messages in thread* [tip:x86/urgent] x86/power: Fix some ordering bugs in __restore_processor_context()
2017-11-30 15:57 [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context() Andy Lutomirski
` (2 preceding siblings ...)
2017-12-04 22:45 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski
@ 2017-12-06 11:36 ` tip-bot for Andy Lutomirski
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Andy Lutomirski @ 2017-12-06 11:36 UTC (permalink / raw)
To: linux-tip-commits
Cc: tglx, hpa, mingo, bp, jarkko.nikula, linux-kernel, a.p.zijlstra,
luto, torvalds
Commit-ID: 5b06bbcfc2c621da3009da8decb7511500c293ed
Gitweb: https://git.kernel.org/tip/5b06bbcfc2c621da3009da8decb7511500c293ed
Author: Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Nov 2017 07:57:57 -0800
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Dec 2017 12:29:12 +0100
x86/power: Fix some ordering bugs in __restore_processor_context()
__restore_processor_context() had a couple of ordering bugs. It
restored GSBASE after calling load_gs_index(), and the latter can
call into tracing code. It also tried to restore segment registers
before restoring the LDT, which is straight-up wrong.
Reorder the code so that we restore GSBASE, then the descriptor
tables, then the segments.
This fixes two bugs. First, it fixes a regression that broke resume
under certain configurations due to irqflag tracing in
native_load_gs_index(). Second, it fixes resume when the userspace
process that initiated suspect had funny segments. The latter can be
reproduced by compiling this:
// SPDX-License-Identifier: GPL-2.0
/*
* ldt_echo.c - Echo argv[1] while using an LDT segment
*/
int main(int argc, char **argv)
{
int ret;
size_t len;
char *buf;
const struct user_desc desc = {
.entry_number = 0,
.base_addr = 0,
.limit = 0xfffff,
.seg_32bit = 1,
.contents = 0, /* Data, grow-up */
.read_exec_only = 0,
.limit_in_pages = 1,
.seg_not_present = 0,
.useable = 0
};
if (argc != 2)
errx(1, "Usage: %s STRING", argv[0]);
len = asprintf(&buf, "%s\n", argv[1]);
if (len < 0)
errx(1, "Out of memory");
ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
if (ret < -1)
errno = -ret;
if (ret)
err(1, "modify_ldt");
asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
write(1, buf, len);
return 0;
}
and running ldt_echo >/sys/power/mem
Without the fix, the latter causes a triple fault on resume.
Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lkml.kernel.org/r/6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/power/cpu.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 84fcfde..5191de1 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -226,8 +226,20 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
load_idt((const struct desc_ptr *)&ctxt->idt_limit);
#endif
+#ifdef CONFIG_X86_64
/*
- * segment registers
+ * We need GSBASE restored before percpu access can work.
+ * percpu access can happen in exception handlers or in complicated
+ * helpers like load_gs_index().
+ */
+ wrmsrl(MSR_GS_BASE, ctxt->gs_base);
+#endif
+
+ fix_processor_context();
+
+ /*
+ * Restore segment registers. This happens after restoring the GDT
+ * and LDT, which happen in fix_processor_context().
*/
#ifdef CONFIG_X86_32
loadsegment(es, ctxt->es);
@@ -248,13 +260,14 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
load_gs_index(ctxt->gs);
asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
+ /*
+ * Restore FSBASE and user GSBASE after reloading the respective
+ * segment selectors.
+ */
wrmsrl(MSR_FS_BASE, ctxt->fs_base);
- wrmsrl(MSR_GS_BASE, ctxt->gs_base);
wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
#endif
- fix_processor_context();
-
do_fpu_end();
tsc_verify_tsc_adjust(true);
x86_platform.restore_sched_clock_state();
^ permalink raw reply related [flat|nested] 6+ messages in thread