LinuxLists.cc - [PATCH v9 00/17] Enable FSGSBASE instructions

[permalink] [raw]

Subject: [PATCH v9 01/17] x86/ptrace: Prevent ptrace from clearing the FS/GS selector

When a ptracer writes a ptracee's FS/GS base with a different value, the
selector is also cleared. This behavior is not correct as the selector
should be preserved.

Update only the base value and leave the selector intact. To simplify the
code further remove the conditional checking for the same value as this
code is not performance-critical.

The only recognizable downside of this change is when the selector is
already nonzero on write. The base will be reloaded according to the
selector. But the case is highly unexpected in real usages.

Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8: none

Changes from v7:
* Fixed to call correct helper functions
* Massaged changelog by Thomas
* Used '[FS|GS] base' consistently, instead of '[FS|GS]BASE'
---
arch/x86/kernel/ptrace.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 3c5bbe8..df222e2 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -370,22 +370,12 @@ static int putreg(struct task_struct *child,
case offsetof(struct user_regs_struct,fs_base):
if (value >= TASK_SIZE_MAX)
return -EIO;
- /*
- * When changing the FS base, use do_arch_prctl_64()
- * to set the index to zero and to set the base
- * as requested.
- */
- if (child->thread.fsbase != value)
- return do_arch_prctl_64(child, ARCH_SET_FS, value);
+ x86_fsbase_write_task(child, value);
return 0;
case offsetof(struct user_regs_struct,gs_base):
- /*
- * Exactly the same here as the %fs handling above.
- */
if (value >= TASK_SIZE_MAX)
return -EIO;
- if (child->thread.gsbase != value)
- return do_arch_prctl_64(child, ARCH_SET_GS, value);
+ x86_gsbase_write_task(child, value);
return 0;
#endif
}
--
2.7.4

2019-10-04 18:21:31

[permalink] [raw]

Subject: [PATCH v9 05/17] x86/entry/64: Switch CR3 before SWAPGS in paranoid entry

When FSGSBASE is enabled, the GS base handling in paranoid entry will need
to retrieve the kernel GS base which requires that the kernel page table is
active.

As the CR3 switch to the kernel page tables (PTI is active) does not depend
on kernel GS base, move the CR3 switch in front of the GS base handling.

Comment the EBX content while at it.

No functional change.

Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Vegard Nossum <[email protected]>
---

Changes from v8: none

Changes from v7:
* Rebased onto the LFENCE-based SWAPGS mitigation code
* Dropped the READ_MSR_GSBASE macro by Thomas
* Rewrote changelog and comments by Thomas
* Use 'GS base' consistently, instead of 'GSBASE'
---
arch/x86/entry/entry_64.S | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index dd0d62a..edb4160 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1219,15 +1219,7 @@ ENTRY(paranoid_entry)
cld
PUSH_AND_CLEAR_REGS save_ret=1
ENCODE_FRAME_POINTER 8
- movl $1, %ebx
- movl $MSR_GS_BASE, %ecx
- rdmsr
- testl %edx, %edx
- js 1f /* negative -> in kernel */
- SWAPGS
- xorl %ebx, %ebx

-1:
/*
* Always stash CR3 in %r14. This value will be restored,
* verbatim, at exit. Needed if paranoid_entry interrupted
@@ -1237,16 +1229,31 @@ ENTRY(paranoid_entry)
* This is also why CS (stashed in the "iret frame" by the
* hardware at entry) can not be used: this may be a return
* to kernel code, but with a user CR3 value.
+ *
+ * Switching CR3 does not depend on kernel GS base so it can
+ * be done before switching to the kernel GS base. This is
+ * required for FSGSBASE because the kernel GS base has to
+ * be retrieved from a kernel internal table.
*/
SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14

+ /* EBX = 1 -> kernel GSBASE active, no restore required */
+ movl $1, %ebx
/*
- * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
- * unconditional CR3 write, even in the PTI case. So do an lfence
- * to prevent GS speculation, regardless of whether PTI is enabled.
+ * The kernel-enforced convention is a negative GS base indicates
+ * a kernel value. No SWAPGS needed on entry and exit.
*/
- FENCE_SWAPGS_KERNEL_ENTRY
+ movl $MSR_GS_BASE, %ecx
+ rdmsr
+ testl %edx, %edx
+ jns .Lparanoid_entry_swapgs
+ ret

+.Lparanoid_entry_swapgs:
+ SWAPGS
+ FENCE_SWAPGS_KERNEL_ENTRY
+ /* EBX = 0 -> SWAPGS required on exit */
+ xorl %ebx, %ebx
ret
END(paranoid_entry)

--
2.7.4

2019-10-04 18:21:35

[permalink] [raw]

Subject: [PATCH v9 08/17] x86/entry/64: Document GSBASE handling in the paranoid path

On FSGSBASE systems, the way to handle GS base in the paranoid path is
different from the existing SWAPGS-based entry/exit path handling. Document
the reason and what has to be done for FSGSBASE enabled systems.

Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8: none

Changes from v7:
* Massaged doc and changelog by Thomas
* Used 'GS base' consistently, instead of 'GSBASE'
---
Documentation/x86/entry_64.rst | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/Documentation/x86/entry_64.rst b/Documentation/x86/entry_64.rst
index a48b3f6..0499a40 100644
--- a/Documentation/x86/entry_64.rst
+++ b/Documentation/x86/entry_64.rst
@@ -108,3 +108,12 @@ We try to only use IST entries and the paranoid entry code for vectors
that absolutely need the more expensive check for the GS base - and we
generate all 'normal' entry points with the regular (faster) paranoid=0
variant.
+
+On FSGSBASE systems, however, user space can set GS without kernel
+interaction. It means the value of GS base itself does not imply anything,
+whether a kernel value or a user space value. So, there is no longer a safe
+way to check whether the exception is entering from user mode or kernel
+mode in the paranoid entry code path. So the GS base value needs to be read
+out, saved and the kernel GS base value written. On exit, the saved GS base
+value needs to be restored unconditionally. The non-paranoid entry/exit
+code still uses SWAPGS unconditionally as the state is known.
--
2.7.4

2019-10-04 18:21:36

[permalink] [raw]

Subject: [PATCH v9 07/17] x86/entry/64: Handle FSGSBASE enabled paranoid entry/exit

Without FSGSBASE, user space cannot change GS base other than through a
PRCTL. The kernel enforces that the user space GS base value is positive
as negative values are used for detecting the kernel space GS base value
in the paranoid entry code.

If FSGSBASE is enabled, user space can set arbitrary GS base values without
kernel intervention, including negative ones, which breaks the paranoid
entry assumptions.

To avoid this, paranoid entry needs to unconditionally save the current
GS base value independent of the interrupted context, retrieve and write
the kernel GS base and unconditionally restore the saved value on exit.
The restore happens either in paranoid exit or in the special exit path of
the NMI low level code.

All other entry code paths which use unconditional SWAPGS are not affected
as they do not depend on the actual content.

The new logic for paranoid entry, when FSGSBASE is enabled, removes SWAPGS
and replaces with unconditional WRGSBASE. Hence no fences are needed.

Suggested-by: H. Peter Anvin <[email protected]>
Suggested-by: Andy Lutomirski <[email protected]>
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Acked-by: Tom Lendacky <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Vegard Nossum <[email protected]>
---

Changes from v8: none

Changes from v7:
* Rebased paranoid exit changes on the precedent cleanup patch
* Massaged changelog and comment by Thomas
* Added comments related to the SWAPGS mitigation
* Used 'GS base' consistently, instead of 'GSBASE'
---
arch/x86/entry/calling.h | 6 ++++
arch/x86/entry/entry_64.S | 78 +++++++++++++++++++++++++++++++++++++++++------
2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index c222302..673d086 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -340,6 +340,12 @@ For 32-bit we have the following conventions - kernel is built with
#endif
.endm

+.macro SAVE_AND_SET_GSBASE scratch_reg:req save_reg:req
+ rdgsbase \save_reg
+ GET_PERCPU_BASE \scratch_reg
+ wrgsbase \scratch_reg
+.endm
+
#endif /* CONFIG_X86_64 */

.macro STACKLEAK_ERASE
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index edb4160..d554754 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -38,6 +38,7 @@
#include <asm/export.h>
#include <asm/frame.h>
#include <asm/nospec-branch.h>
+#include <asm/fsgsbase.h>
#include <linux/err.h>

#include "calling.h"
@@ -1210,9 +1211,14 @@ idtentry machine_check do_mce has_error_code=0 paranoid=1
#endif

/*
- * Save all registers in pt_regs, and switch gs if needed.
- * Use slow, but surefire "are we in kernel?" check.
- * Return: ebx=0: need swapgs on exit, ebx=1: otherwise
+ * Save all registers in pt_regs. Return GS base related information
+ * in EBX depending on the availability of the FSGSBASE instructions:
+ *
+ * FSGSBASE R/EBX
+ * N 0 -> SWAPGS on exit
+ * 1 -> no SWAPGS on exit
+ *
+ * Y GS base value at entry, must be restored in paranoid_exit
*/
ENTRY(paranoid_entry)
UNWIND_HINT_FUNC
@@ -1237,7 +1243,29 @@ ENTRY(paranoid_entry)
*/
SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14

- /* EBX = 1 -> kernel GSBASE active, no restore required */
+ /*
+ * Handling GS base depends on the availability of FSGSBASE.
+ *
+ * Without FSGSBASE the kernel enforces that negative GS base
+ * values indicate kernel GS base. With FSGSBASE no assumptions
+ * can be made about the GS base value when entering from user
+ * space.
+ */
+ ALTERNATIVE "jmp .Lparanoid_entry_checkgs", "", X86_FEATURE_FSGSBASE
+
+ /*
+ * Read the current GS base and store it in %rbx unconditionally,
+ * retrieve and set the current CPUs kernel GS base. The stored value
+ * has to be restored in paranoid_exit unconditionally.
+ *
+ * This unconditional write of GS base ensures no subsequent load
+ * based on a mispredicted GS base.
+ */
+ SAVE_AND_SET_GSBASE scratch_reg=%rax save_reg=%rbx
+ ret
+
+.Lparanoid_entry_checkgs:
+ /* EBX = 1 -> kernel GS base active, no restore required */
movl $1, %ebx
/*
* The kernel-enforced convention is a negative GS base indicates
@@ -1264,10 +1292,17 @@ END(paranoid_entry)
*
* We may be returning to very strange contexts (e.g. very early
* in syscall entry), so checking for preemption here would
- * be complicated. Fortunately, we there's no good reason
- * to try to handle preemption here.
+ * be complicated. Fortunately, there's no good reason to try
+ * to handle preemption here.
+ *
+ * R/EBX contains the GS base related information depending on the
+ * availability of the FSGSBASE instructions:
+ *
+ * FSGSBASE R/EBX
+ * N 0 -> SWAPGS on exit
+ * 1 -> no SWAPGS on exit
*
- * On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
+ * Y User space GS base, must be restored unconditionally
*/
ENTRY(paranoid_exit)
UNWIND_HINT_REGS
@@ -1284,7 +1319,15 @@ ENTRY(paranoid_exit)
TRACE_IRQS_OFF_DEBUG
RESTORE_CR3 scratch_reg=%rax save_reg=%r14

- /* If EBX is 0, SWAPGS is required */
+ /* Handle the three GS base cases */
+ ALTERNATIVE "jmp .Lparanoid_exit_checkgs", "", X86_FEATURE_FSGSBASE
+
+ /* With FSGSBASE enabled, unconditionally resotre GS base */
+ wrgsbase %rbx
+ jmp restore_regs_and_return_to_kernel
+
+.Lparanoid_exit_checkgs:
+ /* On non-FSGSBASE systems, conditionally do SWAPGS */
testl %ebx, %ebx
jnz restore_regs_and_return_to_kernel

@@ -1698,10 +1741,27 @@ end_repeat_nmi:
/* Always restore stashed CR3 value (see paranoid_entry) */
RESTORE_CR3 scratch_reg=%r15 save_reg=%r14

- testl %ebx, %ebx /* swapgs needed? */
+ /*
+ * The above invocation of paranoid_entry stored the GS base
+ * related information in R/EBX depending on the availability
+ * of FSGSBASE.
+ *
+ * If FSGSBASE is enabled, restore the saved GS base value
+ * unconditionally, otherwise take the conditional SWAPGS path.
+ */
+ ALTERNATIVE "jmp nmi_no_fsgsbase", "", X86_FEATURE_FSGSBASE
+
+ wrgsbase %rbx
+ jmp nmi_restore
+
+nmi_no_fsgsbase:
+ /* EBX == 0 -> invoke SWAPGS */
+ testl %ebx, %ebx
jnz nmi_restore
+
nmi_swapgs:
SWAPGS_UNSAFE_STACK
+
nmi_restore:
POP_REGS

--
2.7.4

2019-10-04 18:21:39

[permalink] [raw]

Subject: [PATCH v9 15/17] x86/fsgsbase/64: Enable FSGSBASE on 64bit by default and add a chicken bit

From: Andy Lutomirski <[email protected]>

Now that FSGSBASE is fully supported, remove unsafe_fsgsbase, enable
FSGSBASE by default, and add nofsgsbase to disable it.

Signed-off-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8:
* Massaged the print message for FSGSBASE enablement by Thomas. This
change was missed in v7.

Changes from v7:
* No code change
* Massaged title by Thomas
---
Documentation/admin-guide/kernel-parameters.txt | 3 +--
arch/x86/kernel/cpu/common.c | 32 +++++++++++--------------
2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index eb9a491..a14289a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2899,8 +2899,7 @@
no5lvl [X86-64] Disable 5-level paging mode. Forces
kernel to use 4-level paging instead.

- unsafe_fsgsbase [X86] Allow FSGSBASE instructions. This will be
- replaced with a nofsgsbase flag.
+ nofsgsbase [X86] Disables FSGSBASE instructions.

no_console_suspend
[HW] Never suspend the console
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9f57fb0..9b59377bb 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -437,21 +437,21 @@ static void __init setup_cr_pinning(void)
static_key_enable(&cr_pinning.key);
}

-/*
- * Temporary hack: FSGSBASE is unsafe until a few kernel code paths are
- * updated. This allows us to get the kernel ready incrementally.
- *
- * Once all the pieces are in place, these will go away and be replaced with
- * a nofsgsbase chicken flag.
- */
-static bool unsafe_fsgsbase;
-
-static __init int setup_unsafe_fsgsbase(char *arg)
+static __init int x86_nofsgsbase_setup(char *arg)
{
- unsafe_fsgsbase = true;
+ /* Require an exact match without trailing characters. */
+ if (strlen(arg))
+ return 0;
+
+ /* Do not emit a message if the feature is not present. */
+ if (!boot_cpu_has(X86_FEATURE_FSGSBASE))
+ return 1;
+
+ setup_clear_cpu_cap(X86_FEATURE_FSGSBASE);
+ pr_info("FSGSBASE disabled via kernel command line\n");
return 1;
}
-__setup("unsafe_fsgsbase", setup_unsafe_fsgsbase);
+__setup("nofsgsbase", x86_nofsgsbase_setup);

/*
* Protection Keys are not available in 32-bit mode.
@@ -1472,12 +1472,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
setup_umip(c);

/* Enable FSGSBASE instructions if available. */
- if (cpu_has(c, X86_FEATURE_FSGSBASE)) {
- if (unsafe_fsgsbase)
- cr4_set_bits(X86_CR4_FSGSBASE);
- else
- clear_cpu_cap(c, X86_FEATURE_FSGSBASE);
- }
+ if (cpu_has(c, X86_FEATURE_FSGSBASE))
+ cr4_set_bits(X86_CR4_FSGSBASE);

/*
* The vendor-specific functions might have changed features.
--
2.7.4

2019-10-04 18:21:39

[permalink] [raw]

Subject: [PATCH v9 16/17] x86/elf: Enumerate kernel FSGSBASE capability in AT_HWCAP2

From: Andi Kleen <[email protected]>

The kernel needs to explicitly enable FSGSBASE. So, the application needs
to know if it can safely use these instructions. Just looking at the CPUID
bit is not enough because it may be running in a kernel that does not
enable the instructions.

One way for the application would be to just try and catch the SIGILL.
But that is difficult to do in libraries which may not want to overwrite
the signal handlers of the main application.

Enumerate the enabled FSGSBASE capability in bit 1 of AT_HWCAP2 in the ELF
aux vector. AT_HWCAP2 is already used by PPC for similar purposes.

The application can access it open coded or by using the getauxval()
function in newer versions of glibc.

Signed-off-by: Andi Kleen <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8: none

Changes from v7:
* No code change
* Massaged changelog by Thomas
---
arch/x86/include/uapi/asm/hwcap2.h | 3 +++
arch/x86/kernel/cpu/common.c | 4 +++-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/hwcap2.h b/arch/x86/include/uapi/asm/hwcap2.h
index 8b2effe..5fdfcb4 100644
--- a/arch/x86/include/uapi/asm/hwcap2.h
+++ b/arch/x86/include/uapi/asm/hwcap2.h
@@ -5,4 +5,7 @@
/* MONITOR/MWAIT enabled in Ring 3 */
#define HWCAP2_RING3MWAIT (1 << 0)

+/* Kernel allows FSGSBASE instructions available in Ring 3 */
+#define HWCAP2_FSGSBASE BIT(1)
+
#endif
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9b59377bb..90d7e95 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1472,8 +1472,10 @@ static void identify_cpu(struct cpuinfo_x86 *c)
setup_umip(c);

/* Enable FSGSBASE instructions if available. */
- if (cpu_has(c, X86_FEATURE_FSGSBASE))
+ if (cpu_has(c, X86_FEATURE_FSGSBASE)) {
cr4_set_bits(X86_CR4_FSGSBASE);
+ elf_hwcap2 |= HWCAP2_FSGSBASE;
+ }

/*
* The vendor-specific functions might have changed features.
--
2.7.4

2019-10-04 18:21:42

[permalink] [raw]

Subject: [PATCH v9 14/17] selftests/x86/fsgsbase: Test ptracer-induced GS base write with FSGSBASE

This validates that GS selector and base are independently preserved in
ptrace commands.

Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8: none

Changes from v7:
* Fixed the test message
---
tools/testing/selftests/x86/fsgsbase.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/fsgsbase.c b/tools/testing/selftests/x86/fsgsbase.c
index 950a48b..9a43498 100644
--- a/tools/testing/selftests/x86/fsgsbase.c
+++ b/tools/testing/selftests/x86/fsgsbase.c
@@ -465,7 +465,7 @@ static void test_ptrace_write_gsbase(void)
wait(&status);

if (WSTOPSIG(status) == SIGTRAP) {
- unsigned long gs;
+ unsigned long gs, base;
unsigned long gs_offset = USER_REGS_OFFSET(gs);
unsigned long base_offset = USER_REGS_OFFSET(gs_base);

@@ -481,6 +481,7 @@ static void test_ptrace_write_gsbase(void)
err(1, "PTRACE_POKEUSER");

gs = ptrace(PTRACE_PEEKUSER, child, gs_offset, NULL);
+ base = ptrace(PTRACE_PEEKUSER, child, base_offset, NULL);

/*
* In a non-FSGSBASE system, the nonzero selector will load
@@ -501,8 +502,14 @@ static void test_ptrace_write_gsbase(void)
*/
if (gs == 0)
printf("\tNote: this is expected behavior on older kernels.\n");
+ } else if (have_fsgsbase && (base != 0xFF)) {
+ nerrs++;
+ printf("[FAIL]\tGSBASE changed to %lx\n", base);
} else {
- printf("[OK]\tGS remained 0x%hx\n", *shared_scratch);
+ printf("[OK]\tGS remained 0x%hx", *shared_scratch);
+ if (have_fsgsbase)
+ printf(" and GSBASE changed to 0xFF");
+ printf("\n");
}
}

--
2.7.4

2019-10-04 18:21:45

[permalink] [raw]

Subject: [PATCH v9 11/17] x86/fsgsbase/64: Use FSGSBASE in switch_to() if available

From: Andy Lutomirski <[email protected]>

With the new FSGSBASE instructions, FS/GS base can be efficiently read
and written in __switch_to(). Use that capability to preserve the full
state.

This will enable user code to do whatever it wants with the new
instructions without any kernel-induced gotchas. (There can still be
architectural gotchas: movl %gs,%eax; movl %eax,%gs may change GS base
if WRGSBASE was used, but users are expected to read the CPU manual
before doing things like that.)

This is a considerable speedup. It seems to save about 100 cycles per
context switch compared to the baseline 4.6-rc1 behavior on a Skylake
laptop.

[ chang: 5~10% performance improvements were seen by a context switch
benchmark that ran threads with different FS/GS base values (to the
baseline 4.16). ]

Signed-off-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8:
* Rebased on the precedent helper changes; removed the interrupt
condition check and IRQ disablement from save_fsgs() and
x86_fsgsbase_load().

Changes from v7:
* Used appropriate GS base read/write functions depending on interrupt
conditions. This fixes the bug in v7.
* Massaged changelog by Thomas
* Used '[FS|GS] base' consistently, instead of '[FS|GS]BASE'
---
arch/x86/kernel/process_64.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 295aa0c..56c0e5b 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -200,8 +200,18 @@ static __always_inline void save_fsgs(struct task_struct *task)
{
savesegment(fs, task->thread.fsindex);
savesegment(gs, task->thread.gsindex);
- save_base_legacy(task, task->thread.fsindex, FS);
- save_base_legacy(task, task->thread.gsindex, GS);
+ if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
+ /*
+ * If FSGSBASE is enabled, we can't make any useful guesses
+ * about the base, and user code expects us to save the current
+ * value. Fortunately, reading the base directly is efficient.
+ */
+ task->thread.fsbase = rdfsbase();
+ task->thread.gsbase = x86_gsbase_read_cpu_inactive();
+ } else {
+ save_base_legacy(task, task->thread.fsindex, FS);
+ save_base_legacy(task, task->thread.gsindex, GS);
+ }
}

#if IS_ENABLED(CONFIG_KVM)
@@ -280,10 +290,22 @@ static __always_inline void load_seg_legacy(unsigned short prev_index,
static __always_inline void x86_fsgsbase_load(struct thread_struct *prev,
struct thread_struct *next)
{
- load_seg_legacy(prev->fsindex, prev->fsbase,
- next->fsindex, next->fsbase, FS);
- load_seg_legacy(prev->gsindex, prev->gsbase,
- next->gsindex, next->gsbase, GS);
+ if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
+ /* Update the FS and GS selectors if they could have changed. */
+ if (unlikely(prev->fsindex || next->fsindex))
+ loadseg(FS, next->fsindex);
+ if (unlikely(prev->gsindex || next->gsindex))
+ loadseg(GS, next->gsindex);
+
+ /* Update the bases. */
+ wrfsbase(next->fsbase);
+ x86_gsbase_write_cpu_inactive(next->gsbase);
+ } else {
+ load_seg_legacy(prev->fsindex, prev->fsbase,
+ next->fsindex, next->fsbase, FS);
+ load_seg_legacy(prev->gsindex, prev->gsbase,
+ next->gsindex, next->gsbase, GS);
+ }
}

static unsigned long x86_fsgsbase_read_task(struct task_struct *task,
--
2.7.4

2019-10-04 18:22:04

[permalink] [raw]

Subject: [PATCH v9 03/17] x86/cpu: Add 'unsafe_fsgsbase' to enable CR4.FSGSBASE

From: Andy Lutomirski <[email protected]>

This is temporary. It will allow the next few patches to be tested
incrementally.

Setting unsafe_fsgsbase is a root hole. Don't do it.

Signed-off-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
---

Changes from v8: none
Changes from v7: none
---
Documentation/admin-guide/kernel-parameters.txt | 3 +++
arch/x86/kernel/cpu/common.c | 24 ++++++++++++++++++++++++
2 files changed, 27 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c7ac2f3..eb9a491 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2899,6 +2899,9 @@
no5lvl [X86-64] Disable 5-level paging mode. Forces
kernel to use 4-level paging instead.

+ unsafe_fsgsbase [X86] Allow FSGSBASE instructions. This will be
+ replaced with a nofsgsbase flag.
+
no_console_suspend
[HW] Never suspend the console
Disable suspending of consoles during suspend and
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9ae7d1b..9f57fb0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -438,6 +438,22 @@ static void __init setup_cr_pinning(void)
}

/*
+ * Temporary hack: FSGSBASE is unsafe until a few kernel code paths are
+ * updated. This allows us to get the kernel ready incrementally.
+ *
+ * Once all the pieces are in place, these will go away and be replaced with
+ * a nofsgsbase chicken flag.
+ */
+static bool unsafe_fsgsbase;
+
+static __init int setup_unsafe_fsgsbase(char *arg)
+{
+ unsafe_fsgsbase = true;
+ return 1;
+}
+__setup("unsafe_fsgsbase", setup_unsafe_fsgsbase);
+
+/*
* Protection Keys are not available in 32-bit mode.
*/
static bool pku_disabled;
@@ -1455,6 +1471,14 @@ static void identify_cpu(struct cpuinfo_x86 *c)
setup_smap(c);
setup_umip(c);

+ /* Enable FSGSBASE instructions if available. */
+ if (cpu_has(c, X86_FEATURE_FSGSBASE)) {
+ if (unsafe_fsgsbase)
+ cr4_set_bits(X86_CR4_FSGSBASE);
+ else
+ clear_cpu_cap(c, X86_FEATURE_FSGSBASE);
+ }
+
/*
* The vendor-specific functions might have changed features.
* Now we do "generic changes."
--
2.7.4

2019-10-04 18:22:05

[permalink] [raw]

Subject: [PATCH v9 04/17] x86/entry/64: Clean up paranoid exit

From: Andy Lutomirski <[email protected]>

All that paranoid exit needs to do is to disable IRQs, handle IRQ tracing,
then restore CR3, and restore GS base. Simply do those actions in that
order. Cleaning up the spaghetti code.

Signed-off-by: Andy Lutomirski <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Vegard Nossum <[email protected]>
---

Changes from v8: none

Changes from v7:
* Included as a new patch. Took the cleanup part from the Andy Lutomirski's
original patch [*] and edited its changelog a little bit.

[*] https://lkml.kernel.org/r/59725ceb08977359489fbed979716949ad45f616.1562035429.git.luto@kernel.org
---
arch/x86/entry/entry_64.S | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index b7c3ea4..dd0d62a 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1265,20 +1265,25 @@ END(paranoid_entry)
ENTRY(paranoid_exit)
UNWIND_HINT_REGS
DISABLE_INTERRUPTS(CLBR_ANY)
+
+ /*
+ * The order of operations is important. IRQ tracing requires
+ * kernel GS base and CR3. RESTORE_CR3 requires kernel GS base.
+ *
+ * NB to anyone to try to optimize this code: this code does
+ * not execute at all for exceptions from user mode. Those
+ * exceptions go through error_exit instead.
+ */
TRACE_IRQS_OFF_DEBUG
- testl %ebx, %ebx /* swapgs needed? */
- jnz .Lparanoid_exit_no_swapgs
- TRACE_IRQS_IRETQ
- /* Always restore stashed CR3 value (see paranoid_entry) */
- RESTORE_CR3 scratch_reg=%rbx save_reg=%r14
+ RESTORE_CR3 scratch_reg=%rax save_reg=%r14
+
+ /* If EBX is 0, SWAPGS is required */
+ testl %ebx, %ebx
+ jnz restore_regs_and_return_to_kernel
+
+ /* We are returning to a context with user GS base */
SWAPGS_UNSAFE_STACK
- jmp .Lparanoid_exit_restore
-.Lparanoid_exit_no_swapgs:
- TRACE_IRQS_IRETQ_DEBUG
- /* Always restore stashed CR3 value (see paranoid_entry) */
- RESTORE_CR3 scratch_reg=%rbx save_reg=%r14
-.Lparanoid_exit_restore:
- jmp restore_regs_and_return_to_kernel
+ jmp restore_regs_and_return_to_kernel
END(paranoid_exit)

/*
--
2.7.4

2019-11-15 18:31:03

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Fri, 4 Oct 2019, Chang S. Bae wrote:
>
> Updates from v8 [10]:
> * Internalized the interrupt check in the helper functions (Andy L.)
> * Simplified GS base helper functions (Tony L.)
> * Changed the patch order to put the paranoid path changes before the
> context switch changes (Tony L.)
> * Fixed typos (Randy D.) and massaged a few sentences in the documentation
> * Massaged the FSGSBASE enablement message

That still lacks what Andy requested quite some time ago in the V8 thread:

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

"I also think that, before this series can have my ack, it needs an
actual gdb maintainer to chime in, publicly, and state that they have
thought about and tested the ABI changes and that gdb still works on
patched kernels with and without FSGSBASE enabled. I realize that there
were all kinds of discussions, but they were all quite theoretical, and
I think that the actual patches need to be considered by people who
understand the concerns. Specific test cases would be nice, too."

What's the state of this?

Thanks,

tglx

2019-11-15 19:14:44

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Fri, Nov 15, 2019 at 07:29:17PM +0100, Thomas Gleixner wrote:
> On Fri, 4 Oct 2019, Chang S. Bae wrote:
> >
> > Updates from v8 [10]:
> > * Internalized the interrupt check in the helper functions (Andy L.)
> > * Simplified GS base helper functions (Tony L.)
> > * Changed the patch order to put the paranoid path changes before the
> > context switch changes (Tony L.)
> > * Fixed typos (Randy D.) and massaged a few sentences in the documentation
> > * Massaged the FSGSBASE enablement message
>
> That still lacks what Andy requested quite some time ago in the V8 thread:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> "I also think that, before this series can have my ack, it needs an
> actual gdb maintainer to chime in, publicly, and state that they have
> thought about and tested the ABI changes and that gdb still works on
> patched kernels with and without FSGSBASE enabled. I realize that there
> were all kinds of discussions, but they were all quite theoretical, and
> I think that the actual patches need to be considered by people who
> understand the concerns. Specific test cases would be nice, too."
>
> What's the state of this?

Adding Markus.

-Andi

2019-11-29 14:57:43

by Metzger, Markus T

[permalink] [raw]

Subject: RE: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Fri, Nov 15, 2019 at 07:29:17PM +0100, Thomas Gleixner wrote:
> > On Fri, 4 Oct 2019, Chang S. Bae wrote:
> > >
> > > Updates from v8 [10]:
> > > * Internalized the interrupt check in the helper functions (Andy L.)
> > > * Simplified GS base helper functions (Tony L.)
> > > * Changed the patch order to put the paranoid path changes before the
> > > context switch changes (Tony L.)
> > > * Fixed typos (Randy D.) and massaged a few sentences in the documentation
> > > * Massaged the FSGSBASE enablement message
> >
> > That still lacks what Andy requested quite some time ago in the V8 thread:
> >
> > https://lore.kernel.org/lkml/034aaf3a-a93d-ec03-0bbd-
> [email protected]/
> >
> > "I also think that, before this series can have my ack, it needs an
> > actual gdb maintainer to chime in, publicly, and state that they have
> > thought about and tested the ABI changes and that gdb still works on
> > patched kernels with and without FSGSBASE enabled. I realize that there
> > were all kinds of discussions, but they were all quite theoretical, and
> > I think that the actual patches need to be considered by people who
> > understand the concerns. Specific test cases would be nice, too."
> >
> > What's the state of this?

On branch users/mmetzger/fsgs in sourceware.org/git/binutils-gdb.git,
there's a GDB test covering the behavior discussed theoretically back then.

It covers modifying the selector as well as the base from GDB and using
the modified values for inferior calls as well as for resuming the inferior.

Current kernels allow changing the selector and provide the resulting
base back to the ptracer. They also allow changing the base as long as
the selector is zero. That's the behavior we wanted to preserve IIRC.

The patch series on branch fsgs_tip_5.4-rc1_100319 at
github.com/changbae/Linux-kernel.git breaks tests that modify the
selector and expect that to change the base.

That kernel allows changing the base via ptrace but ignores changes
to the selector.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, http://www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

2019-11-29 16:53:45

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Fri, Nov 29, 2019 at 6:56 AM Metzger, Markus T
<[email protected]> wrote:
>
> > On Fri, Nov 15, 2019 at 07:29:17PM +0100, Thomas Gleixner wrote:
> > > On Fri, 4 Oct 2019, Chang S. Bae wrote:
> > > >
> > > > Updates from v8 [10]:
> > > > * Internalized the interrupt check in the helper functions (Andy L.)
> > > > * Simplified GS base helper functions (Tony L.)
> > > > * Changed the patch order to put the paranoid path changes before the
> > > > context switch changes (Tony L.)
> > > > * Fixed typos (Randy D.) and massaged a few sentences in the documentation
> > > > * Massaged the FSGSBASE enablement message
> > >
> > > That still lacks what Andy requested quite some time ago in the V8 thread:
> > >
> > > https://lore.kernel.org/lkml/034aaf3a-a93d-ec03-0bbd-
> > [email protected]/
> > >
> > > "I also think that, before this series can have my ack, it needs an
> > > actual gdb maintainer to chime in, publicly, and state that they have
> > > thought about and tested the ABI changes and that gdb still works on
> > > patched kernels with and without FSGSBASE enabled. I realize that there
> > > were all kinds of discussions, but they were all quite theoretical, and
> > > I think that the actual patches need to be considered by people who
> > > understand the concerns. Specific test cases would be nice, too."
> > >
> > > What's the state of this?
>
> On branch users/mmetzger/fsgs in sourceware.org/git/binutils-gdb.git,
> there's a GDB test covering the behavior discussed theoretically back then.
>
> It covers modifying the selector as well as the base from GDB and using
> the modified values for inferior calls as well as for resuming the inferior.
>
> Current kernels allow changing the selector and provide the resulting
> base back to the ptracer. They also allow changing the base as long as
> the selector is zero. That's the behavior we wanted to preserve IIRC.

The general kernel rule is that we don't break working applications.
Other than that, we're allowed to change the ABI if existing working
applications don't break. I can't tell whether you wrote a test that
detects a behavior change or whether you wrote a test that tests
behavior that gdb or other programs actually rely on.

Certainly, with a 32-bit *gdb*, writing a nonzero value to FS or GS
using ptrace should change the base accordingly. I think the current
patches get this wrong.

With a 64-bit gdb and a 32-bit inferior, in an ideal world, everything
would work just like full 64-bit, since that's how the hardware works.
But we don't necessary live in an ideal world.

With a 64-bit gdb and a 64-bit inferior, the inferior can set FS to
some nonzero value and then set FSBASE to an arbitrary 64-bit number,
and FS will retain its value. ptrace needs to give gdb some way to
read, save, and restore this state.

I think the ideal behavior is that 64-bit ptrace callers should
control FS and FSBASE independently. The question is: will that break
things? If it will, then we'll need to make sure that there is an API
by which a debugger can independently control FS and FSBASE, and we'll
also need to make sure that whatever existing API debuggers use to
change FS and expect FSBASE to magically change as well continue to
have that effect.

>
> The patch series on branch fsgs_tip_5.4-rc1_100319 at
> github.com/changbae/Linux-kernel.git breaks tests that modify the
> selector and expect that to change the base.
>
> That kernel allows changing the base via ptrace but ignores changes
> to the selector.
>

I don't really understand your test, but I'm pretty sure I found a
couple bugs in the test:

88 int
89 switch_fs_read (unsigned int fs)
90 {
91 __asm__ volatile ("mov %0, %%fs" :: "rm"(fs) : "memory");
92
93 return read_fs ();
94 }

This has fundamentally inconsistent behavior on Intel vs AMD CPUs.
Intel CPUs will clear FSBASE when you write 0 to FS. Older AMD CPUs
do *not* clear FSBASE when you write 0 to FS. Very very new AMD CPUs
behave more like Intel CPUs, I believe.

40 struct user_desc ud;
41 int errcode;
42
43 memset (&ud, 0, sizeof (ud));
44 ud.entry_number = entry;
45 ud.base_addr = (unsigned long) base;
46 ud.limit = (unsigned int) size;
47
48 /* Some 64-bit systems declare ud.base_addr 'unsigned int' instead of
49 'unsigned long'.
50
51 Combined with address space layout randomization, this might
52 truncate our base address and result in a crash when we try to read
53 segment-relative.
54
55 Checking the field size would exclude too many systems so we settle
56 for checking whether we actually truncated the address. */
57
58 if (ud.base_addr != (unsigned long) base)
59 return 0u;

The base of a segment in a descriptor table is 32 bits, full stop.
This is a hardware limitation and has nothing to do with the kernel.
base_addr is correctly unsigned int in the kernel headers. If you
actually find a system where base_addr is unsigned long and unsigned
long is 64 bits, then your test will malfunction.

2019-12-02 08:25:27

by Metzger, Markus T

[permalink] [raw]

Subject: RE: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Fri, Nov 29, 2019 at 6:56 AM Metzger, Markus T
> <[email protected]> wrote:
> >
> > > On Fri, Nov 15, 2019 at 07:29:17PM +0100, Thomas Gleixner wrote:
> > > > On Fri, 4 Oct 2019, Chang S. Bae wrote:
> > > > >
> > > > > Updates from v8 [10]:
> > > > > * Internalized the interrupt check in the helper functions (Andy L.)
> > > > > * Simplified GS base helper functions (Tony L.)
> > > > > * Changed the patch order to put the paranoid path changes before the
> > > > > context switch changes (Tony L.)
> > > > > * Fixed typos (Randy D.) and massaged a few sentences in the
> documentation
> > > > > * Massaged the FSGSBASE enablement message
> > > >
> > > > That still lacks what Andy requested quite some time ago in the V8 thread:
> > > >
> > > > https://lore.kernel.org/lkml/034aaf3a-a93d-ec03-0bbd-
> > > [email protected]/
> > > >
> > > > "I also think that, before this series can have my ack, it needs an
> > > > actual gdb maintainer to chime in, publicly, and state that they have
> > > > thought about and tested the ABI changes and that gdb still works on
> > > > patched kernels with and without FSGSBASE enabled. I realize that there
> > > > were all kinds of discussions, but they were all quite theoretical, and
> > > > I think that the actual patches need to be considered by people who
> > > > understand the concerns. Specific test cases would be nice, too."
> > > >
> > > > What's the state of this?
> >
> > On branch users/mmetzger/fsgs in sourceware.org/git/binutils-gdb.git,
> > there's a GDB test covering the behavior discussed theoretically back then.
> >
> > It covers modifying the selector as well as the base from GDB and using
> > the modified values for inferior calls as well as for resuming the inferior.
> >
> > Current kernels allow changing the selector and provide the resulting
> > base back to the ptracer. They also allow changing the base as long as
> > the selector is zero. That's the behavior we wanted to preserve IIRC.
>
> The general kernel rule is that we don't break working applications.
> Other than that, we're allowed to change the ABI if existing working
> applications don't break. I can't tell whether you wrote a test that
> detects a behavior change or whether you wrote a test that tests
> behavior that gdb or other programs actually rely on.

Well, that's a tough question. The test covers GDB's behavior on today's
systems. GDB itself does not actually rely on that behavior. That is, GDB
itself wouldn't break. You couldn't do all that you could do with it before,
though.

It would be GDB's users that are affected. How do you tell if anyone is
actually relying on it?

> Certainly, with a 32-bit *gdb*, writing a nonzero value to FS or GS
> using ptrace should change the base accordingly. I think the current
> patches get this wrong.
>
> With a 64-bit gdb and a 32-bit inferior, in an ideal world, everything
> would work just like full 64-bit, since that's how the hardware works.

Not sure what you mean. The h/w runs in compatibility mode and the
inferior cannot set the base directly, can it?

> But we don't necessary live in an ideal world.
>
> With a 64-bit gdb and a 64-bit inferior, the inferior can set FS to
> some nonzero value and then set FSBASE to an arbitrary 64-bit number,
> and FS will retain its value. ptrace needs to give gdb some way to
> read, save, and restore this state.

With Chang's patch series, that actually works. You can set FS and then
set FSBASE without setting FS to zero previously. The tests do not cover
that since on current system that leads to the inferior crashing in read_fs().

> I think the ideal behavior is that 64-bit ptrace callers should
> control FS and FSBASE independently. The question is: will that break
> things? If it will, then we'll need to make sure that there is an API
> by which a debugger can independently control FS and FSBASE, and we'll
> also need to make sure that whatever existing API debuggers use to
> change FS and expect FSBASE to magically change as well continue to
> have that effect.

We had discussed this some time ago and proposed the following behavior: "
https://lore.kernel.org/lkml/[email protected]/

In a summary, ptracer's update on FS/GS selector and base
yields such results on tracee's base:
- When FS/GS selector only changed (to nonzero), fetch base
from GDT/LDT (legacy behavior)
- When FS/GS base (regardless of selector) changed, tracee
will have the base
"

The ptracer would need to read registers back after changing the selector
to get the updated base.

The only time when both change at the same time, then, is when registers
are restored after returning from an inferior call. And then, it's the base
we want to take priority since we previously ensured that the base is always
up-to-date.

> > The patch series on branch fsgs_tip_5.4-rc1_100319 at
> > github.com/changbae/Linux-kernel.git breaks tests that modify the
> > selector and expect that to change the base.
> >
> > That kernel allows changing the base via ptrace but ignores changes
> > to the selector.
> >
>
> I don't really understand your test, but I'm pretty sure I found a
> couple bugs in the test:

Thanks for your review.

> 88 int
> 89 switch_fs_read (unsigned int fs)
> 90 {
> 91 __asm__ volatile ("mov %0, %%fs" :: "rm"(fs) : "memory");
> 92
> 93 return read_fs ();
> 94 }
>
> This has fundamentally inconsistent behavior on Intel vs AMD CPUs.
> Intel CPUs will clear FSBASE when you write 0 to FS. Older AMD CPUs
> do *not* clear FSBASE when you write 0 to FS. Very very new AMD CPUs
> behave more like Intel CPUs, I believe.

Thanks for pointing this out but I don't think that this is actually an issue for
this test. This function is only ever used with fs==0xa7 to switch to the LDT
entry that the test program has setup before.

The test sets FS/GS to zero via ptrace from GDB.

> 40 struct user_desc ud;
> 41 int errcode;
> 42
> 43 memset (&ud, 0, sizeof (ud));
> 44 ud.entry_number = entry;
> 45 ud.base_addr = (unsigned long) base;
> 46 ud.limit = (unsigned int) size;
> 47
> 48 /* Some 64-bit systems declare ud.base_addr 'unsigned int' instead of
> 49 'unsigned long'.
> 50
> 51 Combined with address space layout randomization, this might
> 52 truncate our base address and result in a crash when we try to read
> 53 segment-relative.
> 54
> 55 Checking the field size would exclude too many systems so we settle
> 56 for checking whether we actually truncated the address. */
> 57
> 58 if (ud.base_addr != (unsigned long) base)
> 59 return 0u;
>
> The base of a segment in a descriptor table is 32 bits, full stop.
> This is a hardware limitation and has nothing to do with the kernel.
> base_addr is correctly unsigned int in the kernel headers. If you
> actually find a system where base_addr is unsigned long and unsigned
> long is 64 bits, then your test will malfunction.

The modify_ldt(2) man page says: "
The user_desc structure is defined in <asm/ldt.h> as:

struct user_desc {
unsigned int entry_number;
unsigned long base_addr;
unsigned int limit;
unsigned int seg_32bit:1;
unsigned int contents:2;
unsigned int read_exec_only:1;
unsigned int limit_in_pages:1;
unsigned int seg_not_present:1;
unsigned int useable:1;
};
"

The declaration in asm/ldt.h actually defines base_addr as unsigned int.

So my comment about 'some 64-bit systems' is wrong and should actually
say 'all systems'. Will fix.

That by itself is not an issue as long as the main executable is not loaded at
a high address. I only ran into problems with that on some ubuntu system
in our test pool.

Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, http://www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

2019-12-04 20:22:47

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Mon, Dec 2, 2019 at 12:23 AM Metzger, Markus T
<[email protected]> wrote:
>
> > On Fri, Nov 29, 2019 at 6:56 AM Metzger, Markus T
> > <[email protected]> wrote:
> > >
> > > > On Fri, Nov 15, 2019 at 07:29:17PM +0100, Thomas Gleixner wrote:
> > > > > On Fri, 4 Oct 2019, Chang S. Bae wrote:
> > > > > >
> > > > > > Updates from v8 [10]:
> > > > > > * Internalized the interrupt check in the helper functions (Andy L.)
> > > > > > * Simplified GS base helper functions (Tony L.)
> > > > > > * Changed the patch order to put the paranoid path changes before the
> > > > > > context switch changes (Tony L.)
> > > > > > * Fixed typos (Randy D.) and massaged a few sentences in the
> > documentation
> > > > > > * Massaged the FSGSBASE enablement message
> > > > >
> > > > > That still lacks what Andy requested quite some time ago in the V8 thread:
> > > > >
> > > > > https://lore.kernel.org/lkml/034aaf3a-a93d-ec03-0bbd-
> > > > [email protected]/
> > > > >
> > > > > "I also think that, before this series can have my ack, it needs an
> > > > > actual gdb maintainer to chime in, publicly, and state that they have
> > > > > thought about and tested the ABI changes and that gdb still works on
> > > > > patched kernels with and without FSGSBASE enabled. I realize that there
> > > > > were all kinds of discussions, but they were all quite theoretical, and
> > > > > I think that the actual patches need to be considered by people who
> > > > > understand the concerns. Specific test cases would be nice, too."
> > > > >
> > > > > What's the state of this?
> > >
> > > On branch users/mmetzger/fsgs in sourceware.org/git/binutils-gdb.git,
> > > there's a GDB test covering the behavior discussed theoretically back then.
> > >
> > > It covers modifying the selector as well as the base from GDB and using
> > > the modified values for inferior calls as well as for resuming the inferior.
> > >
> > > Current kernels allow changing the selector and provide the resulting
> > > base back to the ptracer. They also allow changing the base as long as
> > > the selector is zero. That's the behavior we wanted to preserve IIRC.
> >
> > The general kernel rule is that we don't break working applications.
> > Other than that, we're allowed to change the ABI if existing working
> > applications don't break. I can't tell whether you wrote a test that
> > detects a behavior change or whether you wrote a test that tests
> > behavior that gdb or other programs actually rely on.
>
> Well, that's a tough question. The test covers GDB's behavior on today's
> systems. GDB itself does not actually rely on that behavior. That is, GDB
> itself wouldn't break. You couldn't do all that you could do with it before,
> though.

GDB does rely on at least some behavior. If I tell gdb to call a
function on my behalf, doesn't it save the old state, call the
function, and then restore the state? Surely it expects the restore
operation to actually restore the state.

>
> It would be GDB's users that are affected. How do you tell if anyone is
> actually relying on it?

No clue. But at least if this type of use is mostly interactive, then
users should be that badly affected.

It also helps that very, very few 64-bit applications use nonzero
segments at all. They used to because of a kernel optimization to
automatically load a segment if an FS or GSBASE less than 4GB was
requested, but that's been gone for a while. Calling
set_thread_area() at all in a 64-bit program requires considerable
gymnastics, and distributions can and do disable modify_ldt() outright
without significant ill effects.

So we're mostly talking about compatibility with 32-bit programs and
exotic users like Wine and DOSEMU.

>
>
> > Certainly, with a 32-bit *gdb*, writing a nonzero value to FS or GS
> > using ptrace should change the base accordingly. I think the current
> > patches get this wrong.
> >
> > With a 64-bit gdb and a 32-bit inferior, in an ideal world, everything
> > would work just like full 64-bit, since that's how the hardware works.
>
> Not sure what you mean. The h/w runs in compatibility mode and the
> inferior cannot set the base directly, can it?

I think there's a general impedance mismatch between gdb and the
kernel/hw here. On Linux on a 64-bit machine, there's isn't really a
strong concept of a "32-bit process" versus a "64-bit process". All
tasks have 64-bit values in RAX, all tasks have R8-R15, all tasks have
a GDT and an LDT, etc. "32-bit tasks" are merely tasks that happen to
be running with a compatibility selector loaded into CS at the time.
Tasks can and do switch freely between compatibility and long mode
using LJMP or LRET. As far as I can tell, however, gdb doesn't really
understand this and thinks that 32-bit tasks are their own special
thing.

This causes me real problems: gdb explodes horribly if I connect gdb
to QEMU's gdbserver (qemu -s) and try to debug during boot when the
inferior switches between 32-bit and long mode.

As far as FSGSBASE goes, a "32-bit task" absolutely can set
independent values in FS and FSBASE, although it's awkward to do so:
the task would have to do a far transfer to long mode, then WRFSBASE,
then far transfer back to compat mode. But this entire sequence of
events could occur without entering the kernel at all, and the ptrace
API should be able to represent the result. I think that, ideally, a
64-bit debugger would understand the essential 64-bitness of even
compat tasks and work sensibly. I don't really expect gdb to be able
to do this any time soon, though.

>
>
> > But we don't necessary live in an ideal world.
> >
> > With a 64-bit gdb and a 64-bit inferior, the inferior can set FS to
> > some nonzero value and then set FSBASE to an arbitrary 64-bit number,
> > and FS will retain its value. ptrace needs to give gdb some way to
> > read, save, and restore this state.
>
> With Chang's patch series, that actually works. You can set FS and then
> set FSBASE without setting FS to zero previously. The tests do not cover
> that since on current system that leads to the inferior crashing in read_fs().
>
>
> > I think the ideal behavior is that 64-bit ptrace callers should
> > control FS and FSBASE independently. The question is: will that break
> > things? If it will, then we'll need to make sure that there is an API
> > by which a debugger can independently control FS and FSBASE, and we'll
> > also need to make sure that whatever existing API debuggers use to
> > change FS and expect FSBASE to magically change as well continue to
> > have that effect.
>
> We had discussed this some time ago and proposed the following behavior: "
> https://lore.kernel.org/lkml/[email protected]/
>
> In a summary, ptracer's update on FS/GS selector and base
> yields such results on tracee's base:
> - When FS/GS selector only changed (to nonzero), fetch base
> from GDT/LDT (legacy behavior)
> - When FS/GS base (regardless of selector) changed, tracee
> will have the base
> "

Indeed. But I never understood how this behavior could be implemented
with the current ABI. As I understand it, gdb only ever sets the
inferior register state by using a single ptrace() call to load the
entire state, which means that the kernel does not know whether just
FS is being written or whether FS and FSBASE are being written.

What actual ptrace() call does gdb use when a 64-bit gdb debugs a
64-bit inferior? How about a 32-bit inferior?

>
> The ptracer would need to read registers back after changing the selector
> to get the updated base.

What would the actual API be?

I think it could make sense to add a whole new ptrace() command to
tell the tracee to, in effect, MOV a specified value to a segment
register. This call would have the actual correct semantics in which
it would return an error code if the specified value is invalid and
would return 0 on success. And then a second ptrace() call could be
issued to read out FSBASE or GSBASE if needed. Would this be useful?
What gdb commands would invoke it?

>
> The only time when both change at the same time, then, is when registers
> are restored after returning from an inferior call. And then, it's the base
> we want to take priority since we previously ensured that the base is always
> up-to-date.

Right. But how does the kernel tell the difference?

> >
> > The base of a segment in a descriptor table is 32 bits, full stop.
> > This is a hardware limitation and has nothing to do with the kernel.
> > base_addr is correctly unsigned int in the kernel headers. If you
> > actually find a system where base_addr is unsigned long and unsigned
> > long is 64 bits, then your test will malfunction.
>
> The modify_ldt(2) man page says: "
> The user_desc structure is defined in <asm/ldt.h> as:
>
> struct user_desc {
> unsigned int entry_number;
> unsigned long base_addr;
> unsigned int limit;
> unsigned int seg_32bit:1;
> unsigned int contents:2;
> unsigned int read_exec_only:1;
> unsigned int limit_in_pages:1;
> unsigned int seg_not_present:1;
> unsigned int useable:1;
> };
> "
>
> The declaration in asm/ldt.h actually defines base_addr as unsigned int.
>
> So my comment about 'some 64-bit systems' is wrong and should actually
> say 'all systems'. Will fix.

>
> That by itself is not an issue as long as the main executable is not loaded at
> a high address. I only ran into problems with that on some ubuntu system
> in our test pool.

In my test cases, I use mmap() with MAP_32BIT to avoid this issue.

2019-12-10 08:28:38

by Metzger, Markus T

[permalink] [raw]

Subject: RE: [PATCH v9 00/17] Enable FSGSBASE instructions

> > > The general kernel rule is that we don't break working applications.
> > > Other than that, we're allowed to change the ABI if existing working
> > > applications don't break. I can't tell whether you wrote a test that
> > > detects a behavior change or whether you wrote a test that tests
> > > behavior that gdb or other programs actually rely on.
> >
> > Well, that's a tough question. The test covers GDB's behavior on today's
> > systems. GDB itself does not actually rely on that behavior. That is, GDB
> > itself wouldn't break. You couldn't do all that you could do with it before,
> > though.
>
> GDB does rely on at least some behavior. If I tell gdb to call a
> function on my behalf, doesn't it save the old state, call the
> function, and then restore the state? Surely it expects the restore
> operation to actually restore the state.

It does. If we managed to break that, inferior calls in GDB would be
broken. Users who don't use inferior calls wouldn't know or care,
though. That's the point I was trying to make previously.

> It also helps that very, very few 64-bit applications use nonzero
> segments at all. They used to because of a kernel optimization to
> automatically load a segment if an FS or GSBASE less than 4GB was
> requested, but that's been gone for a while. Calling
> set_thread_area() at all in a 64-bit program requires considerable
> gymnastics, and distributions can and do disable modify_ldt() outright
> without significant ill effects.
>
> So we're mostly talking about compatibility with 32-bit programs and
> exotic users like Wine and DOSEMU.

I agree that this should mostly affect 32-bit programs.

> > > Certainly, with a 32-bit *gdb*, writing a nonzero value to FS or GS
> > > using ptrace should change the base accordingly. I think the current
> > > patches get this wrong.
> > >
> > > With a 64-bit gdb and a 32-bit inferior, in an ideal world, everything
> > > would work just like full 64-bit, since that's how the hardware works.
> >
> > Not sure what you mean. The h/w runs in compatibility mode and the
> > inferior cannot set the base directly, can it?
>
> I think there's a general impedance mismatch between gdb and the
> kernel/hw here. On Linux on a 64-bit machine, there's isn't really a
> strong concept of a "32-bit process" versus a "64-bit process". All
> tasks have 64-bit values in RAX, all tasks have R8-R15, all tasks have
> a GDT and an LDT, etc. "32-bit tasks" are merely tasks that happen to
> be running with a compatibility selector loaded into CS at the time.
> Tasks can and do switch freely between compatibility and long mode
> using LJMP or LRET. As far as I can tell, however, gdb doesn't really
> understand this and thinks that 32-bit tasks are their own special
> thing.
>
> This causes me real problems: gdb explodes horribly if I connect gdb
> to QEMU's gdbserver (qemu -s) and try to debug during boot when the
> inferior switches between 32-bit and long mode.
>
> As far as FSGSBASE goes, a "32-bit task" absolutely can set
> independent values in FS and FSBASE, although it's awkward to do so:
> the task would have to do a far transfer to long mode, then WRFSBASE,
> then far transfer back to compat mode. But this entire sequence of
> events could occur without entering the kernel at all, and the ptrace
> API should be able to represent the result. I think that, ideally, a
> 64-bit debugger would understand the essential 64-bitness of even
> compat tasks and work sensibly. I don't really expect gdb to be able
> to do this any time soon, though.

I guess the primary use-case would be an application that was originally
written for 32-bit and is being maintained since then. GDB is probably
64-bit in that case.

> > We had discussed this some time ago and proposed the following behavior: "
> > https://lore.kernel.org/lkml/1521481767-22113-15-git-send-email-
> [email protected]/
> >
> > In a summary, ptracer's update on FS/GS selector and base
> > yields such results on tracee's base:
> > - When FS/GS selector only changed (to nonzero), fetch base
> > from GDT/LDT (legacy behavior)
> > - When FS/GS base (regardless of selector) changed, tracee
> > will have the base
> > "
>
> Indeed. But I never understood how this behavior could be implemented
> with the current ABI. As I understand it, gdb only ever sets the
> inferior register state by using a single ptrace() call to load the
> entire state, which means that the kernel does not know whether just
> FS is being written or whether FS and FSBASE are being written.

GDB writes the register state as soon as the user changes one of them.

> What actual ptrace() call does gdb use when a 64-bit gdb debugs a
> 64-bit inferior? How about a 32-bit inferior?

GDB uses GETREGS both for 64-bit and 32-bit inferiors. If GETREGS is
not available, it errors out on 64-bit and falls back to PEEKUSER on 32-bit.

> > The ptracer would need to read registers back after changing the selector
> > to get the updated base.
>
> What would the actual API be?

GETREGS and PEEKUSER.

> I think it could make sense to add a whole new ptrace() command to
> tell the tracee to, in effect, MOV a specified value to a segment
> register. This call would have the actual correct semantics in which
> it would return an error code if the specified value is invalid and
> would return 0 on success. And then a second ptrace() call could be
> issued to read out FSBASE or GSBASE if needed. Would this be useful?
> What gdb commands would invoke it?

Could SETREGS handle it based on the above proposal?

> > The only time when both change at the same time, then, is when registers
> > are restored after returning from an inferior call. And then, it's the base
> > we want to take priority since we previously ensured that the base is always
> > up-to-date.
>
> Right. But how does the kernel tell the difference?

The other times only one changes. Could the kernel compare the old and new
values for selector and base and detect if one or both change at the same time?

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, http://www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

2020-02-24 18:04:19

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Dec 4, 2019, at 12:20, Andy Lutomirski <[email protected]> wrote:
>
> I think it could make sense to add a whole new ptrace() command to
> tell the tracee to, in effect, MOV a specified value to a segment
> register. This call would have the actual correct semantics in which
> it would return an error code if the specified value is invalid and
> would return 0 on success. And then a second ptrace() call could be
> issued to read out FSBASE or GSBASE if needed. Would this be useful?
> What gdb commands would invoke it?

We consider new commands to access GDT/LDT that hpa posted before [1] may be
helpful. If the kernel provides the interfaces to ptracer, we expect GDB for
both 32-/64-bits can make such changes for inferior calls:
(1) When FS/GS selector only updated,
GDB used to write the selector value via SETREGS. Now it can read the
base value from the new APIs and write the base also. This change does
not harm today's kernel, and it retains the legacy behavior on
FSGSBASE-enabled kernels in the future.
(2) When FS/GS base only updated,
(3) When both FS/GS selector and base updated,
GDB has no change from what it used to do. The new FSGSBASE-enabled
kernel improves the behavior by keeping the base regardless of a
selector.

The proposed change in GDB would do an additional GETREGS for every SETREGS
to obtain the old value. Other ptrace-users may need a similar patch if
sensitive to the outcome from writing FS/GS selector, but last time when we
surveyed for other tools [2, 3], we didn't find the issue. We also didn't
find actual users who rely on legacy behavior in practice.

We'd like to hear a clear opinion of whether the GDB changes along with the
new ptrace APIs are necessary and sufficient as preparing the FSGSBASE
support in the kernel.

[1] https://lore.kernel.org/patchwork/cover/954471/
[2] https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
[3] https://lists.openvz.org/pipermail/criu/2018-March/040654.html

Thanks,
Chang

2020-04-14 13:34:51

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Mon, Feb 24, 2020 at 06:02:17PM +0000, Bae, Chang Seok wrote:
>
>> On Dec 4, 2019, at 12:20, Andy Lutomirski <[email protected]> wrote:
>>
>> I think it could make sense to add a whole new ptrace() command to
>> tell the tracee to, in effect, MOV a specified value to a segment
>> register. This call would have the actual correct semantics in which
>> it would return an error code if the specified value is invalid and
>> would return 0 on success. And then a second ptrace() call could be
>> issued to read out FSBASE or GSBASE if needed. Would this be useful?
>> What gdb commands would invoke it?
>
>We consider new commands to access GDT/LDT that hpa posted before [1] may be
>helpful. If the kernel provides the interfaces to ptracer, we expect GDB for
>both 32-/64-bits can make such changes for inferior calls:
>(1) When FS/GS selector only updated,
> GDB used to write the selector value via SETREGS. Now it can read the
> base value from the new APIs and write the base also. This change does
> not harm today's kernel, and it retains the legacy behavior on
> FSGSBASE-enabled kernels in the future.
>(2) When FS/GS base only updated,
>(3) When both FS/GS selector and base updated,
> GDB has no change from what it used to do. The new FSGSBASE-enabled
> kernel improves the behavior by keeping the base regardless of a
> selector.
>
>The proposed change in GDB would do an additional GETREGS for every SETREGS
>to obtain the old value. Other ptrace-users may need a similar patch if
>sensitive to the outcome from writing FS/GS selector, but last time when we
>surveyed for other tools [2, 3], we didn't find the issue. We also didn't
>find actual users who rely on legacy behavior in practice.
>
>We'd like to hear a clear opinion of whether the GDB changes along with the
>new ptrace APIs are necessary and sufficient as preparing the FSGSBASE
>support in the kernel.

Hi folks,

Let me try to revive this work as I think that it's blocked due to
misunderstanding of the current situation.

What I gather from the Intel folks is that the GDB folks are okay with
the change as is and don't expect to be doing any changes on their end.

The intel folks are interested in resolving this, but haven't heard back
on their proposed plan (above).

Thomas/Andy want to make sure that we are doing the right thing and are
not breaking anything:

1. The ptrace modifications are correct (we do the right thing around
updating FS/GS).
2. The ptrace changes don't break existing userspace. I think that
the Intel folks confirmed it above.

Is my attempt at understanding the current situation correct?

--
Thanks,
Sasha

2020-04-14 16:52:09

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Apr 13, 2020, at 13:03, Sasha Levin <[email protected]> wrote:
>
> What I gather from the Intel folks is that the GDB folks are okay with
> the change as is and don't expect to be doing any changes on their end.

As far as I know, we never get any comments from GDB maintainers for this in
public.

Thanks,
Chang

2020-04-15 05:31:20

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> Is my attempt at understanding the current situation correct?

Yes.

Nothing breaks, and it's a nice improvement for context switch
performance, in NMI/PMU performance, and also gives user space two free
registers to play around with.

-Andi

2020-04-17 13:33:59

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Mon, Apr 13, 2020 at 05:32:05PM -0700, Andi Kleen wrote:
>> Is my attempt at understanding the current situation correct?
>
>Yes.
>
>Nothing breaks, and it's a nice improvement for context switch
>performance, in NMI/PMU performance, and also gives user space two free
>registers to play around with.

Thomas, Andy,

Could you list your outstanding objections to this patchset? I know it
might be rehashing stuff you've already mentioned in this thread but I
think that there's a disconnect between folks and it'll help with
restarting everything.

--
Thanks,
Sasha

2020-04-17 15:53:39

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

tip-On Fri, Apr 17, 2020 at 6:30 AM Sasha Levin <[email protected]> wrote:
>
> On Mon, Apr 13, 2020 at 05:32:05PM -0700, Andi Kleen wrote:
> >> Is my attempt at understanding the current situation correct?
> >
> >Yes.
> >
> >Nothing breaks, and it's a nice improvement for context switch
> >performance, in NMI/PMU performance, and also gives user space two free
> >registers to play around with.
>
> Thomas, Andy,
>
> Could you list your outstanding objections to this patchset? I know it
> might be rehashing stuff you've already mentioned in this thread but I
> think that there's a disconnect between folks and it'll help with
> restarting everything.
>

My outstanding objections are:

1. The previous submission was broken. This should obviously be fixed.

2. The issues documented here need to be addressed:

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=56f2ab41b652251f336a0f471b1033afeaedd161

3. Adding FSGSBASE fundamentally changes the user ABI, and the changes
will be observable. This means that something could break, especially
in the case where ptrace is in use and the tracee uses the new
instructions. The old behavior cannot sanely be preserved with
FSGSBASE enabled. This isn't a showstopper, but whoever resubmits
this thing needs to document what changes and what use cases might
break. I'm hopeful that the only thing that will break is actual
human beings using a tool like gdb to manually poke at the registers.
This is fine -- the behavior of the registers is different and human
beings debugging need to be aware of this. But the existing automated
stuff that gdb, lldb, etc do needs to continue working. This
especially includes using gdb to force the tracee to call a function,
e.g. 'p function()'.

4. The exising ptrace API does not provide a sane way to ask what the
base value associated with a selector would be. This means that,
under the natural way to make FSGSBASE and ptrace work together (e.g.
as implemented in the previous submission), the tracer has no good way
to emulate 'MOV [whatever], %gs' in the tracee.

Now maybe no one cares about #4. I certainly have the impression that
the *gdb developers* don't care. But gdb isn't exactly a good example
of a piece of software that tries to work correctly when dealing with
unusual software. Maybe other things like rr will care more. It
might be nice to avoid a situation where a piece of careful software
(like rr?) can support kernel 5.y, but breaks in 5.y+1 because of
FSGSBASE, and only starts working again in 5.y+3 because we added the
ptrace API that's needed.

So maybe the first version should have a PTRACE_LOAD_SEGMENT that
sticks a selector in FS or GS and changes the base accordingly, even
if no current userspace has spoken up and said they need it. And a
selftest.

--Andy

2020-04-20 15:13:09

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> Now maybe no one cares about #4.

Yes noone cares. Selectors are largely obsolete.

> the *gdb developers* don't care. But gdb isn't exactly a good example
> of a piece of software that tries to work correctly when dealing with
> unusual software. Maybe other things like rr will care more. It

rr is used to replay modern software, and modern software
doesn't care about selectors, thus rr doesn't care either.

Please stop the FUD.

Thanks,
-Andi

2020-04-20 17:18:35

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

Andi Kleen <[email protected]> writes:
>> the *gdb developers* don't care. But gdb isn't exactly a good example
>> of a piece of software that tries to work correctly when dealing with
>> unusual software. Maybe other things like rr will care more. It
>
> rr is used to replay modern software, and modern software
> doesn't care about selectors, thus rr doesn't care either.
>
> Please stop the FUD.

There is absolutely no FUD. Being careful about not breaking existing
user space is a legitimate request.

It's up to those who change the ABI to prove that it does not matter and
not up to the maintainers to figure it out.

This sits in limbo for months now just because Intel doesn't get it's
homework done.

Stop making false accusations and provide factual information instead.

Thanks,

tglx

2020-04-21 16:10:53

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Mon, Apr 20, 2020 at 07:14:46PM +0200, Thomas Gleixner wrote:
>Andi Kleen <[email protected]> writes:
>>> the *gdb developers* don't care. But gdb isn't exactly a good example
>>> of a piece of software that tries to work correctly when dealing with
>>> unusual software. Maybe other things like rr will care more. It
>>
>> rr is used to replay modern software, and modern software
>> doesn't care about selectors, thus rr doesn't care either.
>>
>> Please stop the FUD.
>
>There is absolutely no FUD. Being careful about not breaking existing
>user space is a legitimate request.
>
>It's up to those who change the ABI to prove that it does not matter and
>not up to the maintainers to figure it out.

I think that this is a difficult ask; "prove that god doesn't exist".

Andi's point is that there is no known user it breaks, and the Intel
folks did some digging into potential users who might be affected by
this, including 'rr' brought up by Andy, and concluded that there won't
be breakage as a result of this patchset:

https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html

Sure, if you poke at it you could see a behavior change, but is there
an actual user that will be affected by it? I suspect not.

>This sits in limbo for months now just because Intel doesn't get it's
>homework done.
>
>Stop making false accusations and provide factual information instead.

If there's no known user that will be broken here, can we consider
merging this to be disabled by default and let distros try it out? This
will let us find these users while providing an easy way to work around
the problem.

--
Thanks,
Sasha

2020-04-21 16:50:59

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Apr 21, 2020, at 9:06 AM, Sasha Levin <[email protected]> wrote:
>
> On Mon, Apr 20, 2020 at 07:14:46PM +0200, Thomas Gleixner wrote:
>> Andi Kleen <[email protected]> writes:
>>>> the *gdb developers* don't care. But gdb isn't exactly a good example
>>>> of a piece of software that tries to work correctly when dealing with
>>>> unusual software. Maybe other things like rr will care more. It
>>>
>>> rr is used to replay modern software, and modern software
>>> doesn't care about selectors, thus rr doesn't care either.
>>>
>>> Please stop the FUD.
>>
>> There is absolutely no FUD. Being careful about not breaking existing
>> user space is a legitimate request.
>>
>> It's up to those who change the ABI to prove that it does not matter and
>> not up to the maintainers to figure it out.
>
> I think that this is a difficult ask; "prove that god doesn't exist".
>
> Andi's point is that there is no known user it breaks, and the Intel
> folks did some digging into potential users who might be affected by
> this, including 'rr' brought up by Andy, and concluded that there won't
> be breakage as a result of this patchset:
>
> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
>
> Sure, if you poke at it you could see a behavior change, but is there
> an actual user that will be affected by it? I suspect not.
>
>> This sits in limbo for months now just because Intel doesn't get it's
>> homework done.
>>
>> Stop making false accusations and provide factual information instead.
>
> If there's no known user that will be broken here, can we consider
> merging this to be disabled by default and let distros try it out? This
> will let us find these users while providing an easy way to work around
> the problem.

No. Once it’s merged, people will write user code using the ABI, and that means we need to get the ABI right.

The very early versions had severely problematic ABIs. The new ones are probably okay except for, maybe, ptrace. If we had merged the old ones, then we might have gotten stuck with the old, problematic ABI.

>
> --
> Thanks,
> Sasha

2020-04-21 17:17:06

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Apr 21, 2020, at 09:06, Sasha Levin <[email protected]> wrote:
>
> Andi's point is that there is no known user it breaks, and the Intel
> folks did some digging into potential users who might be affected by
> this, including 'rr' brought up by Andy, and concluded that there won't
> be breakage as a result of this patchset:

FWIW, we surveyed tools like rr and CRIU before. Their comments are [*,**]:

"Anyway I think rr will be fine with the new behavior. Our modifications
to fs/gs/fs_base/gs_base are always either a) setting values that the
kernel set during recording to make them happen during replay or b)
emulating PTRACE_SET_REGS that a tracee ptracer tried to set on another
tracee. Either way I think the effects are going to be the same as what
would happen if the program were run without rr.”

"Internally in criu we fetch the regset via ptrace and keep them in
images as they were at moment of dump (if ldt is being used we don't
support such tasks) so I think the changes should not break criu."

What we took away was that those tools reactively follow the underlying
kernel's behavior; so, they should be fine with the FSGSBASE-brought new
behaviors.

[*] https://mail.mozilla.org/pipermail/rr-dev/2018-March/000615.html
[**] https://lists.openvz.org/pipermail/criu/2018-March/040654.html

Thanks,
Chang

2020-04-21 19:59:35

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> Andi's point is that there is no known user it breaks, and the Intel
> folks did some digging into potential users who might be affected by
> this, including 'rr' brought up by Andy, and concluded that there won't
> be breakage as a result of this patchset:
>
> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
>
> Sure, if you poke at it you could see a behavior change, but is there
> an actual user that will be affected by it? I suspect not.

Actually we don't know of any behavior changes caused by the kernel
with selectors.

The application can change itself of course, but only if it uses the
new instructions, which no current application does.

[This was different in the original patch kit long ago which could
change behavior on context switch for programs with out of sync selectors,
but this has been long fixed]

A debugger can also change behavior, but we're not aware of any case
that it would break.

For rr or criu we're also not aware of any case that could break.

I honestly don't know what else could be done in due diligence.

Also just to reiterate merging this would immediately shave off
hundreds of cycles in most context switches.

-Andi

2020-04-21 20:05:45

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> The very early versions had severely problematic ABIs. The new ones are probably okay except for, maybe, ptrace. If we had merged the old ones, then we might have gotten stuck with the old, problematic ABI.

This is beyond vague. Is there a problem with the ABI or not?

If yes please point it out in an actionable concrete way that it can
be addressed.

If not there shouldn't be any reason to further block it.

Thanks

-Andi

2020-04-21 20:26:02

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

> On Apr 21, 2020, at 12:56 PM, Andi Kleen <[email protected]> wrote:
>
>
>>
>> Andi's point is that there is no known user it breaks, and the Intel
>> folks did some digging into potential users who might be affected by
>> this, including 'rr' brought up by Andy, and concluded that there won't
>> be breakage as a result of this patchset:
>>
>> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
>>
>> Sure, if you poke at it you could see a behavior change, but is there
>> an actual user that will be affected by it? I suspect not.
>
> Actually we don't know of any behavior changes caused by the kernel
> with selectors.
>
> The application can change itself of course, but only if it uses the
> new instructions, which no current application does.

If you use ptrace to change the gs selector, the behavior is different on a patched kernel.

Again, I’m not saying that the change is problematic. But I will say that the fact that anyone involved in this series keeps ignoring this fact makes me quite uncomfortable with the patch set.

>
> [This was different in the original patch kit long ago which could
> change behavior on context switch for programs with out of sync selectors,
> but this has been long fixed]

That’s the issue I was referring to.

>
> A debugger can also change behavior, but we're not aware of any case
> that it would break.

How hard did you look?

>
> For rr or criu we're also not aware of any case that could break.
>
> I honestly don't know what else could be done in due diligence.
>
> Also just to reiterate merging this would immediately shave off
> hundreds of cycles in most context switches.
>
> -Andi

2020-04-21 20:54:56

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Tue, Apr 21, 2020 at 01:21:39PM -0700, Andy Lutomirski wrote:
>
>
>> On Apr 21, 2020, at 12:56 PM, Andi Kleen <[email protected]> wrote:
>>
>>
>>>
>>> Andi's point is that there is no known user it breaks, and the Intel
>>> folks did some digging into potential users who might be affected by
>>> this, including 'rr' brought up by Andy, and concluded that there won't
>>> be breakage as a result of this patchset:
>>>
>>> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
>>>
>>> Sure, if you poke at it you could see a behavior change, but is there
>>> an actual user that will be affected by it? I suspect not.
>>
>> Actually we don't know of any behavior changes caused by the kernel
>> with selectors.
>>
>> The application can change itself of course, but only if it uses the
>> new instructions, which no current application does.
>
>If you use ptrace to change the gs selector, the behavior is different on a patched kernel.
>
>Again, I’m not saying that the change is problematic. But I will say that the fact that anyone involved in this series keeps ignoring this fact makes me quite uncomfortable with the patch set.

That's what I referred to with "poke at it". While the behavior may be
different, I fail to find anyone who cares.

>>
>> [This was different in the original patch kit long ago which could
>> change behavior on context switch for programs with out of sync selectors,
>> but this has been long fixed]
>
>That’s the issue I was referring to.
>
>>
>> A debugger can also change behavior, but we're not aware of any case
>> that it would break.
>
>How hard did you look?

Come on, how does one respond to this?

Is there a real use case affected by this? If so, point it out and I'll
be happy to go test it. This was already done (per your previous
request) for gdb and rr.

--
Thanks,
Sasha

2020-04-22 23:03:12

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Tue, Apr 21, 2020 at 1:51 PM Sasha Levin <[email protected]> wrote:
>
> On Tue, Apr 21, 2020 at 01:21:39PM -0700, Andy Lutomirski wrote:
> >
> >
> >> On Apr 21, 2020, at 12:56 PM, Andi Kleen <[email protected]> wrote:
> >>
> >>
> >>>
> >>> Andi's point is that there is no known user it breaks, and the Intel
> >>> folks did some digging into potential users who might be affected by
> >>> this, including 'rr' brought up by Andy, and concluded that there won't
> >>> be breakage as a result of this patchset:
> >>>
> >>> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
> >>>
> >>> Sure, if you poke at it you could see a behavior change, but is there
> >>> an actual user that will be affected by it? I suspect not.
> >>
> >> Actually we don't know of any behavior changes caused by the kernel
> >> with selectors.
> >>
> >> The application can change itself of course, but only if it uses the
> >> new instructions, which no current application does.
> >
> >If you use ptrace to change the gs selector, the behavior is different on a patched kernel.
> >
> >Again, I’m not saying that the change is problematic. But I will say that the fact that anyone involved in this series keeps ignoring this fact makes me quite uncomfortable with the patch set.
>
> That's what I referred to with "poke at it". While the behavior may be
> different, I fail to find anyone who cares.
>
> >>
> >> [This was different in the original patch kit long ago which could
> >> change behavior on context switch for programs with out of sync selectors,
> >> but this has been long fixed]
> >
> >That’s the issue I was referring to.
> >
> >>
> >> A debugger can also change behavior, but we're not aware of any case
> >> that it would break.
> >
> >How hard did you look?
>
> Come on, how does one respond to this?
>
> Is there a real use case affected by this? If so, point it out and I'll
> be happy to go test it. This was already done (per your previous
> request) for gdb and rr.
>

gdb and rr are certainly a good start. If patches show up, I'll take a look.

2020-04-23 04:12:58

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Wed, Apr 22, 2020 at 04:00:16PM -0700, Andy Lutomirski wrote:
>On Tue, Apr 21, 2020 at 1:51 PM Sasha Levin <[email protected]> wrote:
>>
>> On Tue, Apr 21, 2020 at 01:21:39PM -0700, Andy Lutomirski wrote:
>> >
>> >
>> >> On Apr 21, 2020, at 12:56 PM, Andi Kleen <[email protected]> wrote:
>> >>
>> >>
>> >>>
>> >>> Andi's point is that there is no known user it breaks, and the Intel
>> >>> folks did some digging into potential users who might be affected by
>> >>> this, including 'rr' brought up by Andy, and concluded that there won't
>> >>> be breakage as a result of this patchset:
>> >>>
>> >>> https://mail.mozilla.org/pipermail/rr-dev/2018-March/000616.html
>> >>>
>> >>> Sure, if you poke at it you could see a behavior change, but is there
>> >>> an actual user that will be affected by it? I suspect not.
>> >>
>> >> Actually we don't know of any behavior changes caused by the kernel
>> >> with selectors.
>> >>
>> >> The application can change itself of course, but only if it uses the
>> >> new instructions, which no current application does.
>> >
>> >If you use ptrace to change the gs selector, the behavior is different on a patched kernel.
>> >
>> >Again, I’m not saying that the change is problematic. But I will say that the fact that anyone involved in this series keeps ignoring this fact makes me quite uncomfortable with the patch set.
>>
>> That's what I referred to with "poke at it". While the behavior may be
>> different, I fail to find anyone who cares.
>>
>> >>
>> >> [This was different in the original patch kit long ago which could
>> >> change behavior on context switch for programs with out of sync selectors,
>> >> but this has been long fixed]
>> >
>> >That’s the issue I was referring to.
>> >
>> >>
>> >> A debugger can also change behavior, but we're not aware of any case
>> >> that it would break.
>> >
>> >How hard did you look?
>>
>> Come on, how does one respond to this?
>>
>> Is there a real use case affected by this? If so, point it out and I'll
>> be happy to go test it. This was already done (per your previous
>> request) for gdb and rr.
>>
>
>gdb and rr are certainly a good start. If patches show up, I'll take a look.

I'm sorry, but what patches are we talking about?

I just went to gdb to check again that I'm not crazy, and the scenario
you were worried about seems to work just fine:

134 asm volatile ("mov %%gs:(%%rcx), %%rax" : : "c" (offset) : "rax");
(gdb) p printme()
Hi!
$1 = void
(gdb)

Again, please point me to a specific user we break.

--
Thanks,
Sasha

2020-04-25 22:46:24

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

Sasha Levin <[email protected]> writes:
> On Wed, Apr 22, 2020 at 04:00:16PM -0700, Andy Lutomirski wrote:
>>
>>gdb and rr are certainly a good start. If patches show up, I'll take a look.
>
> I'm sorry, but what patches are we talking about?

About patches which:

- Are rebased to current upstream

- Addressed the outstanding review comments

- Have proper documentation in the changelog of the user space visible
ABI changes why it does not break any existing usage and having the
relevant people who maintain tools which utilize the affected
interfaces Cc'ed on submission.

- Made sure that the cleanups I did when merging them initially have
been picked up. I'm not going to waste another couple of days on
this mess just to revert it because it hadn't seen any serious
testing in development.

Thanks,

tglx

2020-04-26 02:54:51

[permalink] [raw]

Subject: Re: [PATCH v9 00/17] Enable FSGSBASE instructions

On Sun, Apr 26, 2020 at 12:39:27AM +0200, Thomas Gleixner wrote:
>Sasha Levin <[email protected]> writes:
>> On Wed, Apr 22, 2020 at 04:00:16PM -0700, Andy Lutomirski wrote:
>>>
>>>gdb and rr are certainly a good start. If patches show up, I'll take a look.
>>
>> I'm sorry, but what patches are we talking about?
>
>About patches which:
>
> - Are rebased to current upstream

v10 of this series was sent a few days ago and is rebased on top of
v5.7-rc1:
https://lore.kernel.org/lkml/[email protected]/ .

> - Addressed the outstanding review comments

I saw a review that Andy has just sent on patch #1 from the new series,
I'll address that.

> - Have proper documentation in the changelog of the user space visible
> ABI changes why it does not break any existing usage and having the
> relevant people who maintain tools which utilize the affected
> interfaces Cc'ed on submission.

The cover letter has references to mail correspondence with maintainers
of these tools that are affected by this change. Each of those exchanges
goes over what FSGSBASE does and answers any specific questions those
maintainers had.

If you want it out of the cover letter and into one of the patches I'd
be happy to do that. If you want me to go chase down another userspace
which we might be breaking just let me know which.

I didn't want to have them on the Cc line as they have already acked
this change from their end and I wanted to avoid additional noise. I'll
be happy to add them back to the next spin of this.

> - Made sure that the cleanups I did when merging them initially have
> been picked up. I'm not going to waste another couple of days on
> this mess just to revert it because it hadn't seen any serious
> testing in development.

Based on your revert
(https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/cpu&id=049331f277fef1c3f2527c2c9afa1d285e9a1247)
I believe that we have all the relevant patches in the series.

I'll also add here that several groups at Microsoft have been running
workloads that heavily exercise the functionality added by this patch.
I'd say that it has gotten a solid round of testing for the past few
months.

--
Thanks,
Sasha

2020-04-26 10:07:30