Skip to contentSkip to conversation

x86: Pile o' FS/GS changes

PATCH In review Andy Lutomirski 6 participants 16 messages Apr 8, 2016 – Apr 13, 2016 2 messages not in the archive

Subsystems (4)
lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

v3 cover x86: Pile o' FS/GS changes

Hi all-

This whole mess is intended for x86/urgent. It fixes several bugs.

It's probably a tiny performance regression on some workloads on Intel CPUs. It's probably varies between a less tiny regression and a small speedup on newer AMD CPUs. It's a bigger regression on AMD K8.

The AMD code could be further sped up by adding a new 'clear_gs' pvop with a better native implementation. I don't particularly want to deal with a backported new pvop, so I didn't do that. Maybe I'll do it later on for new kernels, though.

(For people who care about such things, there are probably three bugs in here depending how you count. Somewhere between one and two of them were independently discovered by Rudolf Marek.)

Andy Lutomirski (5):
selftests/x86: Test the FSBASE/GSBASE API and context switching x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS x86/cpu: Probe the behavior of nulling out a segment at boot time x86/switch_to: Rewrite the FS and GS context switch code x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify

Borislav Petkov (2):
x86/cpu: Add Erratum 88 detection on AMD
x86/entry: Make gs_change a local label

 arch/x86/entry/entry_64.S              |  12 +-
 arch/x86/include/asm/cpufeatures.h     |   3 +
 arch/x86/kernel/cpu/amd.c              |   1 +
 arch/x86/kernel/cpu/common.c           |  71 ++++--
 arch/x86/kernel/process_64.c           | 167 ++++++++------
 tools/testing/selftests/x86/Makefile   |   1 +
 tools/testing/selftests/x86/fsgsbase.c | 398 +++++++++++++++++++++++++++++++++
 7 files changed, 560 insertions(+), 93 deletions(-)
 create mode 100644 tools/testing/selftests/x86/fsgsbase.c
Signature · 1 line
2.5.5
author maintainer
#1 lore raw

PATCH v3 1/7 selftests/x86: Test the FSBASE/GSBASE API and context switching

This catches two distinct bugs in the current code. I'll fix them.

---
 tools/testing/selftests/x86/Makefile   |   1 +
 tools/testing/selftests/x86/fsgsbase.c | 398 +++++++++++++++++++++++++++++++++
 2 files changed, 399 insertions(+)
 create mode 100644 tools/testing/selftests/x86/fsgsbase.c
tools/testing/selftests/x86/Makefile +1 -0
index b47ebd170690..c73425de3cfe 100644
@@ -9,6 +9,7 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_sysc
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
+TARGETS_C_64BIT_ONLY := fsgsbase
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
 TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
tools/testing/selftests/x86/fsgsbase.c new file +398 -0

398 lines not on this page · raw message

Signature · 1 line
2.5.5
#2 lore raw

[tip:x86/asm] selftests/x86: Test the FSBASE/GSBASE API and context switching

selftests/x86: Test the FSBASE/GSBASE API and context switching
Commit-ID:  0051202f6ad5fd9c04d220343e66d1eb890f7b81
Gitweb:     http://git.kernel.org/tip/0051202f6ad5fd9c04d220343e66d1eb890f7b81
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Thu, 7 Apr 2016 17:31:44 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 10:20:41 +0200

selftests/x86: Test the FSBASE/GSBASE API and context switching

This catches two distinct bugs in the current code. I'll fix them.

---
 tools/testing/selftests/x86/Makefile   |   1 +
 tools/testing/selftests/x86/fsgsbase.c | 398 +++++++++++++++++++++++++++++++++
 2 files changed, 399 insertions(+)
tools/testing/selftests/x86/Makefile +1 -0
index b47ebd1..c73425d 100644
@@ -9,6 +9,7 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_sysc
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
+TARGETS_C_64BIT_ONLY := fsgsbase
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
 TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
tools/testing/selftests/x86/fsgsbase.c new file +398 -0

398 lines not on this page · raw message

author maintainer
#3 lore raw

PATCH v3 5/7 x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify

In reply to Andy Lutomirski

It was in detect_nopl, which was either a mistake by me or some kind of mis-merge.

Fixes ff236456f072 ("x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify")
---
 arch/x86/kernel/cpu/common.c | 50 ++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)
arch/x86/kernel/cpu/common.c +25 -25
index ceef584ae8b0..9ede8aa41f18 100644
@@ -862,31 +862,6 @@ static void detect_nopl(struct cpuinfo_x86 *c)
 #else
 	set_cpu_cap(c, X86_FEATURE_NOPL);
 #endif
-
-	/*
-	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
-	 * systems that run Linux at CPL > 0 may or may not have the
-	 * issue, but, even if they have the issue, there's absolutely
-	 * nothing we can do about it because we can't use the real IRET
-	 * instruction.
-	 *
-	 * NB: For the time being, only 32-bit kernels support
-	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
-	 * whether to apply espfix using paravirt hooks.  If any
-	 * non-paravirt system ever shows up that does *not* have the
-	 * ESPFIX issue, we can change this.
-	 */
-#ifdef CONFIG_X86_32
-#ifdef CONFIG_PARAVIRT
-	do {
-		extern void native_iret(void);
-		if (pv_cpu_ops.iret == native_iret)
-			set_cpu_bug(c, X86_BUG_ESPFIX);
-	} while (0);
-#else
-	set_cpu_bug(c, X86_BUG_ESPFIX);
-#endif
-#endif
 }
 
 static void detect_null_seg_behavior(struct cpuinfo_x86 *c)
@@ -952,6 +927,31 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	detect_nopl(c);
 
 	detect_null_seg_behavior(c);
+
+	/*
+	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+	 * systems that run Linux at CPL > 0 may or may not have the
+	 * issue, but, even if they have the issue, there's absolutely
+	 * nothing we can do about it because we can't use the real IRET
+	 * instruction.
+	 *
+	 * NB: For the time being, only 32-bit kernels support
+	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+	 * whether to apply espfix using paravirt hooks.  If any
+	 * non-paravirt system ever shows up that does *not* have the
+	 * ESPFIX issue, we can change this.
+	 */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+	do {
+		extern void native_iret(void);
+		if (pv_cpu_ops.iret == native_iret)
+			set_cpu_bug(c, X86_BUG_ESPFIX);
+	} while (0);
+#else
+	set_cpu_bug(c, X86_BUG_ESPFIX);
+#endif
+#endif
 }
 
 static void x86_init_cache_qos(struct cpuinfo_x86 *c)
Signature · 1 line
2.5.5
#4 lore raw

[tip:x86/asm] x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify()

x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify()
Commit-ID:  0230bb038fa99af0c425fc4cffed307e545a9642
Gitweb:     http://git.kernel.org/tip/0230bb038fa99af0c425fc4cffed307e545a9642
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Thu, 7 Apr 2016 17:31:48 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 10:20:42 +0200

x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify()

It was in detect_nopl(), which was either a mistake by me or some kind of mis-merge.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Rudolf Marek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Fixes ff236456f072 ("x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify")
---
 arch/x86/kernel/cpu/common.c | 50 ++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)
arch/x86/kernel/cpu/common.c +25 -25
index 8e40eee..28d3255 100644
@@ -862,31 +862,6 @@ static void detect_nopl(struct cpuinfo_x86 *c)
 #else
 	set_cpu_cap(c, X86_FEATURE_NOPL);
 #endif
-
-	/*
-	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
-	 * systems that run Linux at CPL > 0 may or may not have the
-	 * issue, but, even if they have the issue, there's absolutely
-	 * nothing we can do about it because we can't use the real IRET
-	 * instruction.
-	 *
-	 * NB: For the time being, only 32-bit kernels support
-	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
-	 * whether to apply espfix using paravirt hooks.  If any
-	 * non-paravirt system ever shows up that does *not* have the
-	 * ESPFIX issue, we can change this.
-	 */
-#ifdef CONFIG_X86_32
-#ifdef CONFIG_PARAVIRT
-	do {
-		extern void native_iret(void);
-		if (pv_cpu_ops.iret == native_iret)
-			set_cpu_bug(c, X86_BUG_ESPFIX);
-	} while (0);
-#else
-	set_cpu_bug(c, X86_BUG_ESPFIX);
-#endif
-#endif
 }
 
 static void detect_null_seg_behavior(struct cpuinfo_x86 *c)
@@ -952,6 +927,31 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	detect_nopl(c);
 
 	detect_null_seg_behavior(c);
+
+	/*
+	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+	 * systems that run Linux at CPL > 0 may or may not have the
+	 * issue, but, even if they have the issue, there's absolutely
+	 * nothing we can do about it because we can't use the real IRET
+	 * instruction.
+	 *
+	 * NB: For the time being, only 32-bit kernels support
+	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+	 * whether to apply espfix using paravirt hooks.  If any
+	 * non-paravirt system ever shows up that does *not* have the
+	 * ESPFIX issue, we can change this.
+	 */
+#ifdef CONFIG_X86_32
+# ifdef CONFIG_PARAVIRT
+	do {
+		extern void native_iret(void);
+		if (pv_cpu_ops.iret == native_iret)
+			set_cpu_bug(c, X86_BUG_ESPFIX);
+	} while (0);
+# else
+	set_cpu_bug(c, X86_BUG_ESPFIX);
+# endif
+#endif
 }
 
 static void x86_init_cache_qos(struct cpuinfo_x86 *c)
author maintainer
#5 lore raw

PATCH v3 7/7 x86/entry: Make gs_change a local label

In reply to Andy Lutomirski

From: Borislav Petkov <[email protected]>

... so that it doesn't appear in objdump output.

---
 arch/x86/entry/entry_64.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +5 -5
index 64d2033d1e49..1693c17dbf81 100644
@@ -781,7 +781,7 @@ ENTRY(native_load_gs_index)
 	pushfq
 	DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
 	SWAPGS
-gs_change:
+.Lgs_change:
 	movl	%edi, %gs
 2:	ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
 	SWAPGS
@@ -789,7 +789,7 @@ gs_change:
 	ret
 END(native_load_gs_index)
 
-	_ASM_EXTABLE(gs_change, bad_gs)
+	_ASM_EXTABLE(.Lgs_change, bad_gs)
 	.section .fixup, "ax"
 	/* running with kernelgs */
 bad_gs:
@@ -1019,13 +1019,13 @@ ENTRY(error_entry)
 	movl	%ecx, %eax			/* zero extend */
 	cmpq	%rax, RIP+8(%rsp)
 	je	.Lbstep_iret
-	cmpq	$gs_change, RIP+8(%rsp)
+	cmpq	$.Lgs_change, RIP+8(%rsp)
 	jne	.Lerror_entry_done
 
 	/*
-	 * hack: gs_change can fail with user gsbase.  If this happens, fix up
+	 * hack: .Lgs_change can fail with user gsbase.  If this happens, fix up
 	 * gsbase and proceed.  We'll fix up the exception and land in
-	 * gs_change's error handler with kernel gsbase.
+	 * .Lgs_change's error handler with kernel gsbase.
 	 */
 	jmp	.Lerror_entry_from_usermode_swapgs
 
Signature · 1 line
2.5.5
#6 lore raw

[tip:x86/asm] x86/entry/64: Make gs_change a local label

x86/entry/64: Make gs_change a local label
Commit-ID:  42c748bb2544f21c3d115240527fe4478e193641
Gitweb:     http://git.kernel.org/tip/42c748bb2544f21c3d115240527fe4478e193641
Author:     Borislav Petkov <[email protected]>
AuthorDate: Thu, 7 Apr 2016 17:31:50 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 10:20:42 +0200

x86/entry/64: Make gs_change a local label

... so that it doesn't appear in objdump output.

---
 arch/x86/entry/entry_64.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +5 -5
index 64d2033..1693c17 100644
@@ -781,7 +781,7 @@ ENTRY(native_load_gs_index)
 	pushfq
 	DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
 	SWAPGS
-gs_change:
+.Lgs_change:
 	movl	%edi, %gs
 2:	ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
 	SWAPGS
@@ -789,7 +789,7 @@ gs_change:
 	ret
 END(native_load_gs_index)
 
-	_ASM_EXTABLE(gs_change, bad_gs)
+	_ASM_EXTABLE(.Lgs_change, bad_gs)
 	.section .fixup, "ax"
 	/* running with kernelgs */
 bad_gs:
@@ -1019,13 +1019,13 @@ ENTRY(error_entry)
 	movl	%ecx, %eax			/* zero extend */
 	cmpq	%rax, RIP+8(%rsp)
 	je	.Lbstep_iret
-	cmpq	$gs_change, RIP+8(%rsp)
+	cmpq	$.Lgs_change, RIP+8(%rsp)
 	jne	.Lerror_entry_done
 
 	/*
-	 * hack: gs_change can fail with user gsbase.  If this happens, fix up
+	 * hack: .Lgs_change can fail with user gsbase.  If this happens, fix up
 	 * gsbase and proceed.  We'll fix up the exception and land in
-	 * gs_change's error handler with kernel gsbase.
+	 * .Lgs_change's error handler with kernel gsbase.
 	 */
 	jmp	.Lerror_entry_from_usermode_swapgs
 

A message that is not in the archive (sent off-list or not yet received): aec6b2df1bfc56101d4e9e2e5d5d570bf41663c6.1460075211.git.luto@kernel.org

#8 lore raw

Re: [PATCH v3 6/7] x86/cpu: Add Erratum 88 detection on AMD

Quoted 7 lines · Andy Lutomirski

From: Borislav Petkov <[email protected]>

Erratum 88 affects old AMD K8s, where a SWAPGS fails to cause an input dependency on GS. Therefore, we need to MFENCE before it.

But that MFENCE is expensive and unnecessary on the remaining x86 CPUs out there so patch it out on the CPUs which don't require it.

This is basically identical to:

https://lkml.kernel.org/g/[email protected]

Whoops! I thought I'd seen that somewhere but I couldn't spot it.

Ingo, etc: we should probably apply one of those patches with a -stable tag (to mitigate the otherwise potentially unpleasant performance regression in here), but I don't really care which one. Andi's has a name for the bug that seems nicer by one character to me, but it would have to be (trivally) rebased.

--Andy

Quoted 51 lines
---
 arch/x86/entry/entry_64.S          | 2 +-
 arch/x86/include/asm/cpufeatures.h | 2 ++
 arch/x86/kernel/cpu/amd.c          | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +1 -1
index 858b555e274b..64d2033d1e49 100644
@@ -783,7 +783,7 @@ ENTRY(native_load_gs_index)
        SWAPGS
 gs_change:
        movl    %edi, %gs
-2:     mfence                                  /* workaround */
+2:     ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
        SWAPGS
        popfq
        ret
arch/x86/include/asm/cpufeatures.h +2 -0
index 2a052302bc43..7bfb6b70c745 100644
@@ -295,6 +295,8 @@
 #define X86_BUG_CLFLUSH_MONITOR        X86_BUG(7) /* AAI65, CLFLUSH required before MONITOR */
 #define X86_BUG_SYSRET_SS_ATTRS        X86_BUG(8) /* SYSRET doesn't fix up SS attrs */
 #define X86_BUG_NULL_SEG       X86_BUG(9) /* Nulling a selector preserves the base */
+#define X86_BUG_SWAPGS_FENCE   X86_BUG(10) /* SWAPGS without input dep on GS */
+
 
 #ifdef CONFIG_X86_32
 /*
arch/x86/kernel/cpu/amd.c +1 -1
index 6e47e3a916f1..b7cc9efe08b5 100644
@@ -632,6 +632,7 @@ static void init_amd_k8(struct cpuinfo_x86 *c)
         */
        msr_set_bit(MSR_K7_HWCR, 6);
 #endif
+       set_cpu_bug(c, X86_BUG_SWAPGS_FENCE);
 }
 
 static void init_amd_gh(struct cpuinfo_x86 *c)
--

2.5.5

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
maintainer
#9 lore raw

Re: [PATCH v3 6/7] x86/cpu: Add Erratum 88 detection on AMD

Quoted 5 lines · Andy Lutomirski

Ingo, etc: we should probably apply one of those patches with a -stable tag (to mitigate the otherwise potentially unpleasant performance regression in here), but I don't really care which one. Andi's has a name for the bug that seems nicer by one character to me, but it would have to be (trivally) rebased.

I don't care which one as long as the commit message says "erratum 88" somewhere.

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#10 lore raw

[tip:x86/asm] x86/cpu: Add Erratum 88 detection on AMD

In reply to a message not in the archive
x86/cpu: Add Erratum 88 detection on AMD
Commit-ID:  96e5d28ae7a5250f3deb2434f1895c9daf48b1bd
Gitweb:     http://git.kernel.org/tip/96e5d28ae7a5250f3deb2434f1895c9daf48b1bd
Author:     Borislav Petkov <[email protected]>
AuthorDate: Thu, 7 Apr 2016 17:31:49 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 10:20:42 +0200

x86/cpu: Add Erratum 88 detection on AMD

Erratum 88 affects old AMD K8s, where a SWAPGS fails to cause an input dependency on GS. Therefore, we need to MFENCE before it.

But that MFENCE is expensive and unnecessary on the remaining x86 CPUs out there so patch it out on the CPUs which don't require it.

---
 arch/x86/entry/entry_64.S          | 2 +-
 arch/x86/include/asm/cpufeatures.h | 2 ++
 arch/x86/kernel/cpu/amd.c          | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +1 -1
index 858b555..64d2033 100644
@@ -783,7 +783,7 @@ ENTRY(native_load_gs_index)
 	SWAPGS
 gs_change:
 	movl	%edi, %gs
-2:	mfence					/* workaround */
+2:	ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
 	SWAPGS
 	popfq
 	ret
arch/x86/include/asm/cpufeatures.h +2 -0
index 2a05230..7bfb6b7 100644
@@ -295,6 +295,8 @@
 #define X86_BUG_CLFLUSH_MONITOR	X86_BUG(7) /* AAI65, CLFLUSH required before MONITOR */
 #define X86_BUG_SYSRET_SS_ATTRS	X86_BUG(8) /* SYSRET doesn't fix up SS attrs */
 #define X86_BUG_NULL_SEG	X86_BUG(9) /* Nulling a selector preserves the base */
+#define X86_BUG_SWAPGS_FENCE	X86_BUG(10) /* SWAPGS without input dep on GS */
+
 
 #ifdef CONFIG_X86_32
 /*
arch/x86/kernel/cpu/amd.c +1 -0
index 6e47e3a..b7cc9ef 100644
@@ -632,6 +632,7 @@ static void init_amd_k8(struct cpuinfo_x86 *c)
 	 */
 	msr_set_bit(MSR_K7_HWCR, 6);
 #endif
+	set_cpu_bug(c, X86_BUG_SWAPGS_FENCE);
 }
 
 static void init_amd_gh(struct cpuinfo_x86 *c)

A message that is not in the archive (sent off-list or not yet received): c6e7b507c72ca3bdbf6c7a8a3ceaa0334e873bd9.1460075211.git.luto@kernel.org

#12 lore raw

Re: [PATCH v3 2/7] x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS

Quoted 4 lines · Andy Lutomirski

ARCH_GET_FS and ARCH_GET_GS attempted to figure out the fsbase and gsbase respectively from saved thread state. This was wrong: fsbase and gsbase live in registers while a thread is running, not in memory.

So I'm wondering, the current code looks totally broken,what user-space code can possibly use this? I checked glibc and Wine, and neither of them does. Wine uses ARCH_SET_GS and glibc uses ARCH_SET_FS, but that's all - neither actually tries to use the ARCH_GET_* reading APIs.

So for backporting purposes I'd be much happier about simply returning -EINVAL or -ENOSYS, and we could re-introduce this code in v4.7.

Thanks,

Ingo

#13 lore raw

Re: [PATCH v3 2/7] x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS

2016-04-08 10:13 GMT+03:00 Ingo Molnar <[email protected]>:

Quoted 8 lines · Andy Lutomirski
Quoted 5 lines

ARCH_GET_FS and ARCH_GET_GS attempted to figure out the fsbase and gsbase respectively from saved thread state. This was wrong: fsbase and gsbase live in registers while a thread is running, not in memory.

Quoted 4 lines

So I'm wondering, the current code looks totally broken,what user-space code can possibly use this? I checked glibc and Wine, and neither of them does. Wine uses ARCH_SET_GS and glibc uses ARCH_SET_FS, but that's all - neither actually tries to use the ARCH_GET_* reading APIs.

Just FYI, CRIU also does not use ARCH_GET_{FS,GS}, only ARCH_SET_{FS,GS}. It gets them with the help of PTRACE_GETREGSET.

Quoted 2 lines

So for backporting purposes I'd be much happier about simply returning -EINVAL or -ENOSYS, and we could re-introduce this code in v4.7.

#14 lore raw

Re: [PATCH v3 2/7] x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS

In reply to Ingo Molnar
Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Andy Lutomirski

ARCH_GET_FS and ARCH_GET_GS attempted to figure out the fsbase and gsbase respectively from saved thread state. This was wrong: fsbase and gsbase live in registers while a thread is running, not in memory.

Quoted 8 lines

So I'm wondering, the current code looks totally broken,what user-space code can possibly use this? I checked glibc and Wine, and neither of them does. Wine uses ARCH_SET_GS and glibc uses ARCH_SET_FS, but that's all - neither actually tries to use the ARCH_GET_* reading APIs.

So for backporting purposes I'd be much happier about simply returning -EINVAL or -ENOSYS, and we could re-introduce this code in v4.7.

Let's just not backport this one. There's no security issue here. If you like the rest of the series, can you remove the stable tag from this patch when you apply it?

I think the old code was at least correct enough that if you did ARCH_GET_FS after ARCH_SET_FS with no funny business in between, it would work.

--Andy

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#15 lore raw

[tip:x86/asm] x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS

In reply to a message not in the archive
x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS
Commit-ID:  d47b50e7a111bb7a56fb1c974728b56209d7f515
Gitweb:     http://git.kernel.org/tip/d47b50e7a111bb7a56fb1c974728b56209d7f515
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Thu, 7 Apr 2016 17:31:45 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 10:20:41 +0200

x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS

ARCH_GET_FS and ARCH_GET_GS attempted to figure out the fsbase and gsbase respectively from saved thread state. This was wrong: fsbase and gsbase live in registers while a thread is running, not in memory.

For reasons I can't fathom, the fsbase and gsbase code were different. Since neither was correct, I didn't try to figure out what the point of the difference was.

Change it to simply read the MSRs.

The code for reading the base for a remote thread is also completely wrong if the target thread uses its own descriptors (which is the case for all 32-bit threaded programs), but fixing that is a different story.

---
 arch/x86/kernel/process_64.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
arch/x86/kernel/process_64.c +7 -12
index 6cbab31..c671b9b 100644
@@ -566,10 +566,10 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 		break;
 	case ARCH_GET_FS: {
 		unsigned long base;
-		if (task->thread.fsindex == FS_TLS_SEL)
-			base = read_32bit_tls(task, FS_TLS);
-		else if (doit)
+		if (doit)
 			rdmsrl(MSR_FS_BASE, base);
+		else if (task->thread.fsindex == FS_TLS_SEL)
+			base = read_32bit_tls(task, FS_TLS);
 		else
 			base = task->thread.fs;
 		ret = put_user(base, (unsigned long __user *)addr);
@@ -577,16 +577,11 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 	}
 	case ARCH_GET_GS: {
 		unsigned long base;
-		unsigned gsindex;
-		if (task->thread.gsindex == GS_TLS_SEL)
+		if (doit)
+			rdmsrl(MSR_KERNEL_GS_BASE, base);
+		else if (task->thread.gsindex == GS_TLS_SEL)
 			base = read_32bit_tls(task, GS_TLS);
-		else if (doit) {
-			savesegment(gs, gsindex);
-			if (gsindex)
-				rdmsrl(MSR_KERNEL_GS_BASE, base);
-			else
-				base = task->thread.gs;
-		} else
+		else
 			base = task->thread.gs;
 		ret = put_user(base, (unsigned long __user *)addr);
 		break;
maintainer
#16 lore raw
In reply to Andy Lutomirski
Quoted 8 lines · Andy Lutomirski

Hi all-

This whole mess is intended for x86/urgent. It fixes several bugs.

It's probably a tiny performance regression on some workloads on Intel CPUs. It's probably varies between a less tiny regression and a small speedup on newer AMD CPUs. It's a bigger regression on AMD K8.

Shhh, otherwise the 2-3 K8 users left would crawl out of the woodwork complaining. ;-)

Quoted 4 lines

The AMD code could be further sped up by adding a new 'clear_gs' pvop with a better native implementation. I don't particularly want to deal with a backported new pvop, so I didn't do that. Maybe I'll do it later on for new kernels, though.

Yeah, before you do that, we should probably measure that first to see whether it is worth the trouble and the added pvop gunk at all.

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
maintainer Reviewed-by on the v3 series
#17 lore raw
In reply to Andy Lutomirski
Quoted 29 lines · Andy Lutomirski

Hi all-

This whole mess is intended for x86/urgent. It fixes several bugs.

It's probably a tiny performance regression on some workloads on Intel CPUs. It's probably varies between a less tiny regression and a small speedup on newer AMD CPUs. It's a bigger regression on AMD K8.

The AMD code could be further sped up by adding a new 'clear_gs' pvop with a better native implementation. I don't particularly want to deal with a backported new pvop, so I didn't do that. Maybe I'll do it later on for new kernels, though.

(For people who care about such things, there are probably three bugs in here depending how you count. Somewhere between one and two of them were independently discovered by Rudolf Marek.)

Andy Lutomirski (5):
selftests/x86: Test the FSBASE/GSBASE API and context switching x86/arch_prctl: Fix ARCH_GET_FS and ARCH_GET_GS x86/cpu: Probe the behavior of nulling out a segment at boot time x86/switch_to: Rewrite the FS and GS context switch code x86/cpu: Move X86_BUG_ESPFIX initialization to generic_identify

Borislav Petkov (2):
x86/cpu: Add Erratum 88 detection on AMD
x86/entry: Make gs_change a local label

Quoted 9 lines
 arch/x86/entry/entry_64.S              |  12 +-
 arch/x86/include/asm/cpufeatures.h     |   3 +
 arch/x86/kernel/cpu/amd.c              |   1 +
 arch/x86/kernel/cpu/common.c           |  71 ++++--
 arch/x86/kernel/process_64.c           | 167 ++++++++------
 tools/testing/selftests/x86/Makefile   |   1 +
 tools/testing/selftests/x86/fsgsbase.c | 398 +++++++++++++++++++++++++++++++++
 7 files changed, 560 insertions(+), 93 deletions(-)
 create mode 100644 tools/testing/selftests/x86/fsgsbase.c

Forgot to add:

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.