Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Alexey Dobriyan]

> +	len = sprintf(buf, "%d\n", READ_ONCE(*field));

READ_ONCE isn't necessary as there is only one read being made.

> +	len = min(count, sizeof(buf) - 1);
> +	if (copy_from_user(buf, user_buf, len))
> +		return -EFAULT;
> +
> +	buf[len] = '\0';
> +	if (kstrtouint(buf, 0, &enable))
> +		return -EINVAL;

There is kstrto*_from_user().

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Andrea Arcangeli]

On Mon, Jan 08, 2018 at 08:43:40PM +0300, Alexey Dobriyan wrote:
> > +	len = sprintf(buf, "%d\n", READ_ONCE(*field));
> 
> READ_ONCE isn't necessary as there is only one read being made.

Others might disagree but you shouldn't ever let gcc touch any memory
that can change under gcc without READ_ONCE or volatile.

Ages ago I was told in a switch() statement gcc could decide to use an
hash and re-read the value and crash.

Not for the simple case above, and we often don't use READ_ONCE and we
might user barrier() instead, but it would be better to use READ_ONCE.

READ_ONCE is more correct and there's no point to try to optimize it
especially if there's only one read being made in that function.

Either version in practice will work, but READ_ONCE is preferable IMHO.

Thanks,
Andrea

[PATCH v2 0/8] IBRS patch series

[Tim Chen]

Thanks to everyone for the feedback on the initial posting.
This is an updated patchset and I hope I've captured all
the review comments.  I've done a lot of code clean up
per everyone's comments. Please let me know if I've missed
something.

The retpoline related changes is moved to the end of the
patch series, so they can be taken out or changed easily
without affecting the other patches.

Many people hate the multi-bits spec_ctrl_ibrs variable so
I got rid of that and replace it with a dynamic_ibrs flag
to indicate if we need to switch IBRS enter/exiting kernel
which is more intuitive and also makes the code cleaner.

Peter/Andrea suggested that we use a static key to control the run time
IBRS enabling/disabling with "STATIC_JUMP_IF_TRUE" kind
of construct.  However, I had some concerns that
JUMP_LABEL config may be disabled and the construct cannot
be used. I also encountered some
OOPs when I'm changing ibrs control state probably
related to changing the jump label branching. I haven't
had time to debug that so I left it out for now.
I will welcome some help here on a patch to get the static key
thing working right.

v2.
1. Added missing feature enumeration in tools/arch/x86/include/asm/cpufeatures.h  
2. Kernel entry macros label cleanup and move them to calling.h
3. Remove unnecessary irqs_diabled check in the mwait
4. Don't use a bit field base sys control variable to make ibrs enabling
   simpler and easier to understand
5. Corrected compile issues for firmware update code
6. Leave IBPB feature bits out from this patch series and will be added
   in its own set of patches later

Tim

---patch series details---
This patch series enables the basic detection and usage of x86 indirect
branch speculation feature.  It enables the indirect branch restricted
speculation (IBRS) on kernel entry and disables it on exit.
It enumerates the indirect branch prediction barrier (IBPB).

The x86 IBRS feature requires corresponding microcode support.
It mitigates the variant 2 vulnerability described in
https://googleprojectzero.blogspot.com/2018/01/reading-privileged-memory-with-side.html

If IBRS is set, near returns and near indirect jumps/calls will not
allow their predicted target address to be controlled by code that
executed in a less privileged prediction mode before the IBRS mode was
last written with a value of 1 or on another logical processor so long
as all RSB entries from the previous less privileged prediction mode
are overwritten.

Both retpoline and IBRS provides mitigation against variant 2 attacks,
with IBRS being the most secured method but could incur more performance
overhead compared to retpoline[1].  If you are very paranoid or you
run on a CPU where IBRS=1 is cheaper, you may also want to run in "IBRS
always" mode.

See: https://docs.google.com/document/d/e/2PACX-1vSMrwkaoSUBAFc6Fjd19F18c1O9pudkfAY-7lGYGOTN8mc9ul-J6pWadcAaBJZcVA7W_3jlLKRtKRbd/pub

More detailed description of IBRS is described in the first patch.

It is applied on top of the page table isolation changes.

A run time and boot time control of the IBRS feature is provided

There are 2 ways to control IBRS

1. At boot time
    noibrs kernel boot parameter will disable IBRS usage

Otherwise if the above parameters are not specified, the system
will enable ibrs and ibpb usage if the cpu supports it.

2. At run time
    echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
    echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
    echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel (IBRS always)

[1] https://lkml.org/lkml/2018/1/4/174


Tim Chen (8):
  x86/feature: Detect the x86 IBRS feature to control Speculation
  x86/enter: MACROS to set/clear IBRS
  x86/enter: Use IBRS on syscall and interrupts
  x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
  x86/idle: Disable IBRS entering idle and enable it on wakeup
  x86/microcode: Recheck IBRS features on microcode reload
  x86: Do not use dynamic IBRS if retpoline is enabled
  x86: Use IBRS for firmware update path

 arch/x86/entry/calling.h                 | 104 +++++++++++++++
 arch/x86/entry/entry_64.S                |  23 ++++
 arch/x86/entry/entry_64_compat.S         |   8 ++
 arch/x86/include/asm/apm.h               |   6 +
 arch/x86/include/asm/cpufeatures.h       |   1 +
 arch/x86/include/asm/efi.h               |  17 ++-
 arch/x86/include/asm/msr-index.h         |   4 +
 arch/x86/include/asm/mwait.h             |  13 ++
 arch/x86/include/asm/spec_ctrl.h         |  54 ++++++++
 arch/x86/kernel/cpu/Makefile             |   1 +
 arch/x86/kernel/cpu/microcode/core.c     |   4 +
 arch/x86/kernel/cpu/scattered.c          |   3 +
 arch/x86/kernel/cpu/spec_ctrl.c          | 209 +++++++++++++++++++++++++++++++
 arch/x86/kernel/process.c                |   9 +-
 tools/arch/x86/include/asm/cpufeatures.h |   1 +
 15 files changed, 453 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/include/asm/spec_ctrl.h
 create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c

-- 
2.9.4

[PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

From: Tim Chen <tim.c.chen@linux.intel.com>
From: Andrea Arcangeli <aarcange@redhat.com>

There are 2 ways to control IBRS

1. At boot time
    noibrs kernel boot parameter will disable IBRS usage

Otherwise if the above parameters are not specified, the system
will enable ibrs and ibpb usage if the cpu supports it.

2. At run time
    echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
    echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
    echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel

The implementation was updated with input and suggestions from Andrea Arcangeli.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/entry/calling.h         |  42 ++++++++--
 arch/x86/include/asm/spec_ctrl.h |  15 ++++
 arch/x86/kernel/cpu/Makefile     |   1 +
 arch/x86/kernel/cpu/scattered.c  |   2 +
 arch/x86/kernel/cpu/spec_ctrl.c  | 160 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 214 insertions(+), 6 deletions(-)
 create mode 100644 arch/x86/include/asm/spec_ctrl.h
 create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 09c870d..6b65d47 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with
 .endm
 
 .macro ENABLE_IBRS
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
+	jmp	.Ldone_\@
+
 .Lskip_\@:
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
 	POP_MSR_REGS
+
 .Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+	jmp	.Ldone_\@
+
 .Lskip_\@:
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+
 .Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	movl	$MSR_IA32_SPEC_CTRL, %ecx
 	rdmsr
 	movl	%eax, \save_reg
@@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel is built with
 	movl	$0, %edx
 	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
 	wrmsr
+	jmp	.Ldone_\@
+
 .Lskip_\@:
+	lfence
+.Ldone_\@:
 .endm
 
 .macro RESTORE_IBRS_CLOBBER save_reg:req
-	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	testl	$1, dynamic_ibrs
+	jz	.Lskip_\@
+
 	/* Set IBRS to the value saved in the save_reg */
 	movl    $MSR_IA32_SPEC_CTRL, %ecx
 	movl    $0, %edx
 	movl    \save_reg, %eax
 	wrmsr
+	jmp	.Ldone_\@
+
 .Lskip_\@:
+	lfence
+.Ldone_\@:
 .endm
diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
new file mode 100644
index 0000000..4fda38b
--- /dev/null
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_X86_SPEC_CTRL_H
+#define _ASM_X86_SPEC_CTRL_H
+
+#include <asm/msr-index.h>
+#include <asm/cpufeatures.h>
+#include <asm/microcode.h>
+
+void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);
+bool ibrs_inuse(void);
+
+extern unsigned int dynamic_ibrs;
+
+#endif /* _ASM_X86_SPEC_CTRL_H */
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 570e8bb..3ffbd24 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -24,6 +24,7 @@ obj-y			+= match.o
 obj-y			+= bugs.o
 obj-y			+= aperfmperf.o
 obj-y			+= cpuid-deps.o
+obj-y			+= spec_ctrl.o
 
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index bc50c40..5756d14 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -8,6 +8,7 @@
 #include <asm/processor.h>
 
 #include <asm/apic.h>
+#include <asm/spec_ctrl.h>
 
 struct cpuid_bit {
 	u16 feature;
@@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 		if (regs[cb->reg] & (1 << cb->bit))
 			set_cpu_cap(c, cb->feature);
 	}
+	scan_spec_ctrl_feature(c);
 }
 
 u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
new file mode 100644
index 0000000..1641bec
--- /dev/null
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -0,0 +1,160 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/mutex.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+
+#include <asm/spec_ctrl.h>
+#include <asm/cpufeature.h>
+
+unsigned int dynamic_ibrs __read_mostly;
+EXPORT_SYMBOL_GPL(dynamic_ibrs);
+
+enum {
+	IBRS_DISABLED,
+	/* in host kernel, disabled in guest and userland */
+	IBRS_ENABLED,
+	/* in host kernel and host userland, disabled in guest */
+	IBRS_ENABLED_USER,
+	IBRS_MAX = IBRS_ENABLED_USER,
+};
+static unsigned int ibrs_enabled;
+static bool ibrs_admin_disabled;
+
+/* mutex to serialize IBRS control changes */
+DEFINE_MUTEX(spec_ctrl_mutex);
+
+void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
+{
+	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
+		if (!ibrs_admin_disabled) {
+			dynamic_ibrs = 1;
+			ibrs_enabled = IBRS_ENABLED;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);
+
+/*
+ * Used after boot phase to rescan spec_ctrl feature,
+ * serialize scan with spec_ctrl_mutex.
+ */
+void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c)
+{
+	mutex_lock(&spec_ctrl_mutex);
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+		if (!ibrs_admin_disabled) {
+			dynamic_ibrs = 1;
+			ibrs_enabled = IBRS_ENABLED;
+		}
+	}
+	mutex_unlock(&spec_ctrl_mutex);
+}
+EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature);
+
+bool ibrs_inuse(void)
+{
+	return ibrs_enabled == IBRS_ENABLED;
+}
+EXPORT_SYMBOL_GPL(ibrs_inuse);
+
+static int __init noibrs(char *str)
+{
+	ibrs_admin_disabled = true;
+	ibrs_enabled = IBRS_DISABLED;
+
+	return 0;
+}
+early_param("noibrs", noibrs);
+
+static ssize_t __enabled_read(struct file *file, char __user *user_buf,
+			      size_t count, loff_t *ppos, unsigned int *field)
+{
+	char buf[32];
+	unsigned int len;
+
+	len = sprintf(buf, "%d\n", READ_ONCE(*field));
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t ibrs_enabled_read(struct file *file, char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	return __enabled_read(file, user_buf, count, ppos, &ibrs_enabled);
+}
+
+static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
+{
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		wrmsrl_on_cpu(cpu, msr_nr, val);
+	put_online_cpus();
+}
+
+static ssize_t ibrs_enabled_write(struct file *file,
+				  const char __user *user_buf,
+				  size_t count, loff_t *ppos)
+{
+	char buf[32];
+	ssize_t len;
+	unsigned int enable;
+
+	len = min(count, sizeof(buf) - 1);
+	if (copy_from_user(buf, user_buf, len))
+		return -EFAULT;
+
+	buf[len] = '\0';
+	if (kstrtouint(buf, 0, &enable))
+		return -EINVAL;
+
+	if (enable > IBRS_MAX)
+		return -EINVAL;
+
+	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+		ibrs_enabled = IBRS_DISABLED;
+		return -EINVAL;
+	}
+
+	mutex_lock(&spec_ctrl_mutex);
+
+	if (enable == IBRS_DISABLED) {
+		/* disable IBRS usage */
+		ibrs_admin_disabled = true;
+		dynamic_ibrs = 0;
+		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
+					 SPEC_CTRL_FEATURE_DISABLE_IBRS);
+
+	} else if (enable == IBRS_ENABLED) {
+		/* enable IBRS usage in kernel */
+		ibrs_admin_disabled = false;
+		dynamic_ibrs = 1;
+
+	} else if (enable == IBRS_ENABLED_USER) {
+		/* enable IBRS all the time in both userspace and kernel */
+		ibrs_admin_disabled = false;
+		dynamic_ibrs = 0;
+		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
+					 SPEC_CTRL_FEATURE_ENABLE_IBRS);
+	}
+
+	ibrs_enabled = enable;
+
+	mutex_unlock(&spec_ctrl_mutex);
+	return count;
+}
+
+static const struct file_operations fops_ibrs_enabled = {
+	.read = ibrs_enabled_read,
+	.write = ibrs_enabled_write,
+	.llseek = default_llseek,
+};
+
+static int __init debugfs_spec_ctrl(void)
+{
+	debugfs_create_file("ibrs_enabled", S_IRUSR | S_IWUSR,
+			    arch_debugfs_dir, NULL, &fops_ibrs_enabled);
+	return 0;
+}
+late_initcall(debugfs_spec_ctrl);
-- 
2.9.4

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Dave Hansen]

On 01/05/2018 06:12 PM, Tim Chen wrote:
>  .macro ENABLE_IBRS
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@

There was an earlier suggestion to use STATIC_JUMP_IF_... here.  That's
a good suggestion, but we encountered some issues with it either
crashing the kernel at boot or not properly turning on/off.

We would love to do that minor really soon, but we figured everyone
would rather see this version than wait for us to debug such a minor tweak.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Fri, Jan 05, 2018 at 07:12:12PM -0800, Dave Hansen wrote:
> On 01/05/2018 06:12 PM, Tim Chen wrote:
> >  .macro ENABLE_IBRS
> > -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> > +	testl	$1, dynamic_ibrs
> > +	jz	.Lskip_\@
> 
> There was an earlier suggestion to use STATIC_JUMP_IF_... here.  That's

Yes, and thanks for Cc'ing me... :/

> a good suggestion, but we encountered some issues with it either
> crashing the kernel at boot or not properly turning on/off.
> 
> We would love to do that minor really soon, but we figured everyone
> would rather see this version than wait for us to debug such a minor tweak.

Its not minor, you need that LFENCE without it. Which makes the whole
thing unconditionally expensive, which sucks.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > a good suggestion, but we encountered some issues with it either
> > crashing the kernel at boot or not properly turning on/off.

The below boots, but I lack stuff to test the enabling.

--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -9,9 +9,8 @@
 
 void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);
 void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c);
-bool ibrs_inuse(void);
 
-extern unsigned int dynamic_ibrs;
+DECLARE_STATIC_KEY_FALSE(ibrs_key);
 
 static inline void __disable_indirect_speculation(void)
 {
@@ -31,21 +30,14 @@ static inline void __enable_indirect_spe
 static inline void unprotected_speculation_begin(void)
 {
 	lockdep_assert_irqs_disabled();
-	if (dynamic_ibrs)
+	if (static_branch_unlikely(&ibrs_key))
 		__enable_indirect_speculation();
 }
 
 static inline void unprotected_speculation_end(void)
 {
-	if (dynamic_ibrs) {
+	if (static_branch_unlikely(&ibrs_key))
 		__disable_indirect_speculation();
-	} else {
-		/*
-		 * rmb prevent unwanted speculation when we
-		 * are setting IBRS
-		 */
-		rmb();
-	}
 }
 
 #endif /* _ASM_X86_SPEC_CTRL_H */
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -7,8 +7,7 @@
 #include <asm/spec_ctrl.h>
 #include <asm/cpufeature.h>
 
-unsigned int dynamic_ibrs __read_mostly;
-EXPORT_SYMBOL_GPL(dynamic_ibrs);
+DEFINE_STATIC_KEY_FALSE(ibrs_key);
 
 enum {
 	IBRS_DISABLED,
@@ -27,7 +26,7 @@ DEFINE_MUTEX(spec_ctrl_mutex);
 static inline void set_ibrs_feature(void)
 {
 	if (!ibrs_admin_disabled) {
-		dynamic_ibrs = 1;
+		static_branch_enable(&ibrs_key);
 		ibrs_enabled = IBRS_ENABLED;
 	}
 }
@@ -54,12 +53,6 @@ void rescan_spec_ctrl_feature(struct cpu
 }
 EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature);
 
-bool ibrs_inuse(void)
-{
-	return ibrs_enabled == IBRS_ENABLED;
-}
-EXPORT_SYMBOL_GPL(ibrs_inuse);
-
 static int __init noibrs(char *str)
 {
 	ibrs_admin_disabled = true;
@@ -124,19 +117,23 @@ static ssize_t ibrs_enabled_write(struct
 	if (enable == IBRS_DISABLED) {
 		/* disable IBRS usage */
 		ibrs_admin_disabled = true;
-		dynamic_ibrs = 0;
+		static_branch_disable(&ibrs_key);
 		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
 					 SPEC_CTRL_FEATURE_DISABLE_IBRS);
 
 	} else if (enable == IBRS_ENABLED) {
 		/* enable IBRS usage in kernel */
 		ibrs_admin_disabled = false;
-		dynamic_ibrs = 1;
+		static_branch_enable(&ibrs_key);
+		/*
+		 * This too needs an IPI to force all active CPUs into a known state.
+		 */
 
 	} else if (enable == IBRS_ENABLED_USER) {
 		/* enable IBRS all the time in both userspace and kernel */
 		ibrs_admin_disabled = false;
-		dynamic_ibrs = 0;
+		static_branch_disable(&ibrs_key);
+
 		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
 					 SPEC_CTRL_FEATURE_ENABLE_IBRS);
 	}
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -373,22 +373,17 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
@@ -398,20 +393,15 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS_CLOBBER
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
 
@@ -419,8 +409,7 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	movl	$MSR_IA32_SPEC_CTRL, %ecx
 	rdmsr
@@ -429,25 +418,18 @@ For 32-bit we have the following convent
 	movl	$0, %edx
 	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
 	wrmsr
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro RESTORE_IBRS_CLOBBER save_reg:req
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	/* Set IBRS to the value saved in the save_reg */
 	movl    $MSR_IA32_SPEC_CTRL, %ecx
 	movl    $0, %edx
 	movl    \save_reg, %eax
 	wrmsr
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
>>> a good suggestion, but we encountered some issues with it either
>>> crashing the kernel at boot or not properly turning on/off.
> 
> The below boots, but I lack stuff to test the enabling.

Peter,

Thanks.  Will give it a spin.  One other concern is if
JUMP_LABEL is not configured, this may not work, and
we may still need fall back to a control variable.

Tim

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Mon, Jan 08, 2018 at 09:28:12AM -0800, Tim Chen wrote:
> On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
> > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> >>> a good suggestion, but we encountered some issues with it either
> >>> crashing the kernel at boot or not properly turning on/off.
> > 
> > The below boots, but I lack stuff to test the enabling.
> 
> Peter,
> 
> Thanks.  Will give it a spin.  One other concern is if
> JUMP_LABEL is not configured, this may not work, and
> we may still need fall back to a control variable.

Urgh yes.. I always forget about that case. Will the retpoline crud be
backported to a GCC old enough to not have asm-goto? If not, we could
make all of this simply require asm-goto.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Woodhouse, David]

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

On Mon, 2018-01-08 at 18:42 +0100, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 09:28:12AM -0800, Tim Chen wrote:
> > On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
> > > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > >>> a good suggestion, but we encountered some issues with it either
> > >>> crashing the kernel at boot or not properly turning on/off.
> > > 
> > > The below boots, but I lack stuff to test the enabling.
> > 
> > Peter,
> > 
> > Thanks.  Will give it a spin.  One other concern is if
> > JUMP_LABEL is not configured, this may not work, and
> > we may still need fall back to a control variable.
> 
> Urgh yes.. I always forget about that case. Will the retpoline crud be
> backported to a GCC old enough to not have asm-goto? If not, we could
> make all of this simply require asm-goto.

I think HJ said he was planning to backport as far as 4.9. When was
asm-goto added?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5210 bytes --]

RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Lu, Hongjiu]

> 
> On Mon, 2018-01-08 at 18:42 +0100, Peter Zijlstra wrote:
> > On Mon, Jan 08, 2018 at 09:28:12AM -0800, Tim Chen wrote:
> > > On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
> > > > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > > >>> a good suggestion, but we encountered some issues with it either
> > > >>> crashing the kernel at boot or not properly turning on/off.
> > > >
> > > > The below boots, but I lack stuff to test the enabling.
> > >
> > > Peter,
> > >
> > > Thanks.  Will give it a spin.  One other concern is if JUMP_LABEL is
> > > not configured, this may not work, and we may still need fall back
> > > to a control variable.
> >
> > Urgh yes.. I always forget about that case. Will the retpoline crud be
> > backported to a GCC old enough to not have asm-goto? If not, we could
> > make all of this simply require asm-goto.
> 
> I think HJ said he was planning to backport as far as 4.9. When was asm-
> goto added?

It was added to GCC 4.5.

H.J.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Thomas Gleixner]

On Mon, 8 Jan 2018, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 09:28:12AM -0800, Tim Chen wrote:
> > On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
> > > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > >>> a good suggestion, but we encountered some issues with it either
> > >>> crashing the kernel at boot or not properly turning on/off.
> > > 
> > > The below boots, but I lack stuff to test the enabling.
> > 
> > Peter,
> > 
> > Thanks.  Will give it a spin.  One other concern is if
> > JUMP_LABEL is not configured, this may not work, and
> > we may still need fall back to a control variable.
> 
> Urgh yes.. I always forget about that case. Will the retpoline crud be
> backported to a GCC old enough to not have asm-goto? If not, we could
> make all of this simply require asm-goto.

No, ifs and buts, really.

This wants to be a jump label and we set the requirements here. I'm tired
of this completely bogus crap just to support some archaic version of GCC.

This is messy enough and no, this whole we need a control variable nonsense
is just not going to happen.

Thanks,

	tglx

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/09/2018 02:40 AM, Thomas Gleixner wrote:
> On Mon, 8 Jan 2018, Peter Zijlstra wrote:
>> On Mon, Jan 08, 2018 at 09:28:12AM -0800, Tim Chen wrote:
>>> On 01/08/2018 08:14 AM, Peter Zijlstra wrote:
>>>> On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
>>>>>> a good suggestion, but we encountered some issues with it either
>>>>>> crashing the kernel at boot or not properly turning on/off.
>>>>
>>>> The below boots, but I lack stuff to test the enabling.
>>>
>>> Peter,
>>>
>>> Thanks.  Will give it a spin.  One other concern is if
>>> JUMP_LABEL is not configured, this may not work, and
>>> we may still need fall back to a control variable.
>>
>> Urgh yes.. I always forget about that case. Will the retpoline crud be
>> backported to a GCC old enough to not have asm-goto? If not, we could
>> make all of this simply require asm-goto.
> 
> No, ifs and buts, really.
> 
> This wants to be a jump label and we set the requirements here. I'm tired
> of this completely bogus crap just to support some archaic version of GCC.
> 
> This is messy enough and no, this whole we need a control variable nonsense
> is just not going to happen.
> 
> 

Thomas,

I'll be sending an updated patchset with boot option opt in for ibrs
and leave the control varaible out.  I agree that we can worry about the
control variable later.

Thanks.

Tim

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[David Woodhouse]

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

On Tue, 2018-01-09 at 09:55 -0800, Tim Chen wrote:
> 
> Thomas,
> 
> I'll be sending an updated patchset with boot option opt in for ibrs
> and leave the control varaible out.  I agree that we can worry about the
> control variable later.

Please base this on the spectre_v2= option that's already in
tip/x86/pti.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/09/2018 10:13 AM, David Woodhouse wrote:
> On Tue, 2018-01-09 at 09:55 -0800, Tim Chen wrote:
>>
>> Thomas,
>>
>> I'll be sending an updated patchset with boot option opt in for ibrs
>> and leave the control varaible out.  I agree that we can worry about the
>> control variable later.
> 
> Please base this on the spectre_v2= option that's already in
> tip/x86/pti.
> 

Will do that for boot option.

Tim

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Konrad Rzeszutek Wilk]

On Mon, Jan 08, 2018 at 05:14:37PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > > a good suggestion, but we encountered some issues with it either
> > > crashing the kernel at boot or not properly turning on/off.
> 
> The below boots, but I lack stuff to test the enabling.

..snip..
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -373,22 +373,17 @@ For 32-bit we have the following convent
>  .endm
>  
>  .macro ENABLE_IBRS
> -	testl	$1, dynamic_ibrs
> -	jz	.Lskip_\@
> +	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
>  
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>  	POP_MSR_REGS
> -	jmp	.Ldone_\@
>  
>  .Lskip_\@:
> -	lfence
> -.Ldone_\@:
>  .endm

I know that this particular patchset is now obsolete as the retpoline
along with stuffing the RSB half or full is the preferred way.

But I am wondering - why was the 'lfence' added in the first place
if dynamic_ibrs was zero?

It certainly is not putting the speculative execution on a wild ride
like: "[tip:x86/pti] x86/retpoline: Use LFENCE instead of PAUSE in the
retpoline/RSB filling RSB macros" https://git.kernel.org/tip/2eb9137c8744f9adf1670e9aa52850948a30112b

So what was the intent behind this? Was it: "oh if we do not have
IBRS let us at least add lfence on  every system call, interrupt, nmi,
exception, etc to do a poor man version of IBRS?"

Thank you.
P.S.
My apologies if this was discussed in the prior versions of this thread.
I must have missed it.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[David Woodhouse]

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

On Sat, 2018-01-27 at 08:59 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 08, 2018 at 05:14:37PM +0100, Peter Zijlstra wrote:
> > 
> > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > > 
> > > > 
> > > > a good suggestion, but we encountered some issues with it either
> > > > crashing the kernel at boot or not properly turning on/off.
> > The below boots, but I lack stuff to test the enabling.
> ..snip..
> > 
> > --- a/arch/x86/entry/calling.h
> > +++ b/arch/x86/entry/calling.h
> > @@ -373,22 +373,17 @@ For 32-bit we have the following convent
> >  .endm
> >  
> >  .macro ENABLE_IBRS
> > -	testl	$1, dynamic_ibrs
> > -	jz	.Lskip_\@
> > +	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
> >  
> >  	PUSH_MSR_REGS
> >  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> >  	POP_MSR_REGS
> > -	jmp	.Ldone_\@
> >  
> >  .Lskip_\@:
> > -	lfence
> > -.Ldone_\@:
> >  .endm
> I know that this particular patchset is now obsolete as the retpoline
> along with stuffing the RSB half or full is the preferred way.
> 
> But I am wondering - why was the 'lfence' added in the first place
> if dynamic_ibrs was zero?

This was in the system call path, just before the jmp *sys_call_table()
indirect jump. If an attacker could cause that 'jz .Lskip_\@' over the
wrmsr to be predicted *taken*, speculative execution would happily
proceed all the way to the indirect jump. Oops :)

Hence the 'else lfence' so that the branch-taken code path still stops
to wait. Hence also the subsequent insistence on using ALTERNATIVE for
it, and my paranoia about relying on GCC not missing optimisations if
we're using static_cpu_has(). 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Greg KH]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.chen@linux.intel.com>
> From: Andrea Arcangeli <aarcange@redhat.com>
> 
> There are 2 ways to control IBRS
> 
> 1. At boot time
>     noibrs kernel boot parameter will disable IBRS usage
> 
> Otherwise if the above parameters are not specified, the system
> will enable ibrs and ibpb usage if the cpu supports it.
> 
> 2. At run time
>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
> 
> The implementation was updated with input and suggestions from Andrea Arcangeli.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---

<snip>

> index 0000000..4fda38b
> --- /dev/null
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Thank you for these markings.

> --- /dev/null
> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/mutex.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/spec_ctrl.h>
> +#include <asm/cpufeature.h>
> +
> +unsigned int dynamic_ibrs __read_mostly;
> +EXPORT_SYMBOL_GPL(dynamic_ibrs);

What kernel module needs this symbol?  A symbol can be "global" and not
be exported, those are two different things.

And again, horrible name for a global symbol, if this really is being
exported to modules (i.e. the world.)

> +
> +enum {
> +	IBRS_DISABLED,

As you are making a user/kernel API here, shouldn't you enforce the
values this enum has "just to be sure"?

> +	/* in host kernel, disabled in guest and userland */
> +	IBRS_ENABLED,
> +	/* in host kernel and host userland, disabled in guest */
> +	IBRS_ENABLED_USER,
> +	IBRS_MAX = IBRS_ENABLED_USER,
> +};

Also, this should be documented somewhere, Documentation/ABI/ perhaps?

> +static unsigned int ibrs_enabled;
> +static bool ibrs_admin_disabled;
> +
> +/* mutex to serialize IBRS control changes */
> +DEFINE_MUTEX(spec_ctrl_mutex);

static?

> +
> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
> +{
> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
> +		if (!ibrs_admin_disabled) {
> +			dynamic_ibrs = 1;
> +			ibrs_enabled = IBRS_ENABLED;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);

Again, what module needs this?

Same for all the exports in this file, and again, if they are needed in
modules, you need to get a better naming scheme.  I'm not trying to
bikeshed, it really matters, our global symbol namespace is a mess,
don't make it worse :)

thanks,

greg k-h

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/06/2018 12:54 AM, Greg KH wrote:
> On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
>> From: Tim Chen <tim.c.chen@linux.intel.com>
>> From: Andrea Arcangeli <aarcange@redhat.com>
>>
>> There are 2 ways to control IBRS
>>
>> 1. At boot time
>>     noibrs kernel boot parameter will disable IBRS usage
>>
>> Otherwise if the above parameters are not specified, the system
>> will enable ibrs and ibpb usage if the cpu supports it.
>>
>> 2. At run time
>>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
>>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
>>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
>>
>> The implementation was updated with input and suggestions from Andrea Arcangeli.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> ---
> 
> <snip>
> 
>> index 0000000..4fda38b
>> --- /dev/null
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Thank you for these markings.
> 
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
>> @@ -0,0 +1,160 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <asm/spec_ctrl.h>
>> +#include <asm/cpufeature.h>
>> +
>> +unsigned int dynamic_ibrs __read_mostly;
>> +EXPORT_SYMBOL_GPL(dynamic_ibrs);
> 
> What kernel module needs this symbol?  A symbol can be "global" and not
> be exported, those are two different things.
> 
> And again, horrible name for a global symbol, if this really is being
> exported to modules (i.e. the world.)
> 
>> +
>> +enum {
>> +	IBRS_DISABLED,
> 
> As you are making a user/kernel API here, shouldn't you enforce the
> values this enum has "just to be sure"?
> 
>> +	/* in host kernel, disabled in guest and userland */
>> +	IBRS_ENABLED,
>> +	/* in host kernel and host userland, disabled in guest */
>> +	IBRS_ENABLED_USER,
>> +	IBRS_MAX = IBRS_ENABLED_USER,
>> +};
> 
> Also, this should be documented somewhere, Documentation/ABI/ perhaps?
> 
>> +static unsigned int ibrs_enabled;
>> +static bool ibrs_admin_disabled;
>> +
>> +/* mutex to serialize IBRS control changes */
>> +DEFINE_MUTEX(spec_ctrl_mutex);
> 
> static?
> 
>> +
>> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
>> +{
>> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
>> +		if (!ibrs_admin_disabled) {
>> +			dynamic_ibrs = 1;
>> +			ibrs_enabled = IBRS_ENABLED;
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);
> 
> Again, what module needs this?

Right now no module will need it.  I'll get rid of it.

> 
> Same for all the exports in this file, and again, if they are needed in
> modules, you need to get a better naming scheme.  I'm not trying to
> bikeshed, it really matters, our global symbol namespace is a mess,
> don't make it worse :)

Noted.  I'll add spec_ctrl in front of those.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Konrad Rzeszutek Wilk]

On Sat, Jan 06, 2018 at 10:10:59AM -0800, Tim Chen wrote:
> 
> 
> On 01/06/2018 12:54 AM, Greg KH wrote:
> > On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> >> From: Tim Chen <tim.c.chen@linux.intel.com>
> >> From: Andrea Arcangeli <aarcange@redhat.com>
> >>
> >> There are 2 ways to control IBRS
> >>
> >> 1. At boot time
> >>     noibrs kernel boot parameter will disable IBRS usage
> >>
> >> Otherwise if the above parameters are not specified, the system
> >> will enable ibrs and ibpb usage if the cpu supports it.
> >>
> >> 2. At run time
> >>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
> >>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
> >>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
> >>


This is going to create headaches in the future.

That is future CPUs there will be no need for this MSR nor retpoline as
the CPUs will observe correctness when switching .. rings/vm-exits/etc
and I would assume that 'ibrs_enabled' will return 0.

And that will make folks scared and run to support/Intel with
complaints.

Furthmore with the 'retpoline' work you can disable IBRS and instead use
'retpoline's as mitigation - and again the 'ibrs_enabled' is now zero.
Cue in horde of customers calling support.

Would it be better to have an global /sys/../spectre_resistent instead
of these 'well, check if the repoline sysfs is enabled, or if that is
not, then look at the cpuid flags'.

It would be good to have this future proof.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Greg KH]

On Sat, Jan 06, 2018 at 04:25:19PM -0500, Konrad Rzeszutek Wilk wrote:
> On Sat, Jan 06, 2018 at 10:10:59AM -0800, Tim Chen wrote:
> > 
> > 
> > On 01/06/2018 12:54 AM, Greg KH wrote:
> > > On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> > >> From: Tim Chen <tim.c.chen@linux.intel.com>
> > >> From: Andrea Arcangeli <aarcange@redhat.com>
> > >>
> > >> There are 2 ways to control IBRS
> > >>
> > >> 1. At boot time
> > >>     noibrs kernel boot parameter will disable IBRS usage
> > >>
> > >> Otherwise if the above parameters are not specified, the system
> > >> will enable ibrs and ibpb usage if the cpu supports it.
> > >>
> > >> 2. At run time
> > >>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
> > >>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
> > >>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
> > >>
> 
> 
> This is going to create headaches in the future.
> 
> That is future CPUs there will be no need for this MSR nor retpoline as
> the CPUs will observe correctness when switching .. rings/vm-exits/etc
> and I would assume that 'ibrs_enabled' will return 0.
> 
> And that will make folks scared and run to support/Intel with
> complaints.
> 
> Furthmore with the 'retpoline' work you can disable IBRS and instead use
> 'retpoline's as mitigation - and again the 'ibrs_enabled' is now zero.
> Cue in horde of customers calling support.
> 
> Would it be better to have an global /sys/../spectre_resistent instead
> of these 'well, check if the repoline sysfs is enabled, or if that is
> not, then look at the cpuid flags'.
> 
> It would be good to have this future proof.

It's a debugfs api, it can be changed at any time, to be anything we
want, and all is fine :)

Let's get this all working first please, and then a "real" api can be
designed and implemented to please everyone.

thanks,

greg k-h

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Konrad Rzeszutek Wilk]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.chen@linux.intel.com>
> From: Andrea Arcangeli <aarcange@redhat.com>
> 
> There are 2 ways to control IBRS
> 
> 1. At boot time
>     noibrs kernel boot parameter will disable IBRS usage
> 
> Otherwise if the above parameters are not specified, the system
> will enable ibrs and ibpb usage if the cpu supports it.
> 
> 2. At run time
>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
> 
> The implementation was updated with input and suggestions from Andrea Arcangeli.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  arch/x86/entry/calling.h         |  42 ++++++++--
>  arch/x86/include/asm/spec_ctrl.h |  15 ++++
>  arch/x86/kernel/cpu/Makefile     |   1 +
>  arch/x86/kernel/cpu/scattered.c  |   2 +
>  arch/x86/kernel/cpu/spec_ctrl.c  | 160 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 214 insertions(+), 6 deletions(-)
>  create mode 100644 arch/x86/include/asm/spec_ctrl.h
>  create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
> 
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 09c870d..6b65d47 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with
>  .endm
>  
>  .macro ENABLE_IBRS
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>  	POP_MSR_REGS
> +	jmp	.Ldone_\@
> +
>  .Lskip_\@:
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro DISABLE_IBRS
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs

On every system call we end up hammering on this 'dynamic_ibrs'
variable. And it looks like it can be flipped via the IPI mechanism.

Would it make sense for this to be per-cpu?

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Dave Hansen]

On 01/06/2018 06:41 AM, Konrad Rzeszutek Wilk wrote:
>>  .macro DISABLE_IBRS
>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>> +	testl	$1, dynamic_ibrs
> On every system call we end up hammering on this 'dynamic_ibrs'
> variable. And it looks like it can be flipped via the IPI mechanism.
> 
> Would it make sense for this to be per-cpu?

It's probably better to either just make it __read_mostly or get the
static branches that folks were suggesting actually working.

RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Van De Ven, Arjan]

> >>  .macro DISABLE_IBRS
> >> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> >> +	testl	$1, dynamic_ibrs
> > On every system call we end up hammering on this 'dynamic_ibrs'
> > variable. And it looks like it can be flipped via the IPI mechanism.
> >
> > Would it make sense for this to be per-cpu?
> 
> It's probably better to either just make it __read_mostly or get the
> static branches that folks were suggesting actually working.

I still wonder if this isn't just better as a boot command line

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Dave Hansen]

On 01/06/2018 09:41 AM, Van De Ven, Arjan wrote:
>>>>  .macro DISABLE_IBRS
>>>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>>>> +	testl	$1, dynamic_ibrs
>>> On every system call we end up hammering on this 'dynamic_ibrs'
>>> variable. And it looks like it can be flipped via the IPI mechanism.
>>>
>>> Would it make sense for this to be per-cpu?
>>
>> It's probably better to either just make it __read_mostly or get the
>> static branches that folks were suggesting actually working.
> 
> I still wonder if this isn't just better as a boot command line

It's simpler that way.  But, ideally, we want to make it runtime
switchable to match the implementation in the distros.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Thomas Gleixner]

On Sat, 6 Jan 2018, Dave Hansen wrote:

> On 01/06/2018 09:41 AM, Van De Ven, Arjan wrote:
> >>>>  .macro DISABLE_IBRS
> >>>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> >>>> +	testl	$1, dynamic_ibrs
> >>> On every system call we end up hammering on this 'dynamic_ibrs'
> >>> variable. And it looks like it can be flipped via the IPI mechanism.
> >>>
> >>> Would it make sense for this to be per-cpu?
> >>
> >> It's probably better to either just make it __read_mostly or get the
> >> static branches that folks were suggesting actually working.
> > 
> > I still wonder if this isn't just better as a boot command line
> 
> It's simpler that way.  But, ideally, we want to make it runtime
> switchable to match the implementation in the distros.

Stop this silly argument please. The distros shipped lots of crap which we
dont want to have at all.

I told you folks yesterday what I want to see and the sysctl thing is the
least on that list and it's not needed for getting the important thing -
the protection - to work.

Can we pretty please do the basics and worry about that sysctl or whatever
people have on their wishlist once the dust settled.

Thanks,

	tglx

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Konrad Rzeszutek Wilk]

On Sat, Jan 06, 2018 at 08:47:19PM +0100, Thomas Gleixner wrote:
> On Sat, 6 Jan 2018, Dave Hansen wrote:
> 
> > On 01/06/2018 09:41 AM, Van De Ven, Arjan wrote:
> > >>>>  .macro DISABLE_IBRS
> > >>>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> > >>>> +	testl	$1, dynamic_ibrs
> > >>> On every system call we end up hammering on this 'dynamic_ibrs'
> > >>> variable. And it looks like it can be flipped via the IPI mechanism.
> > >>>
> > >>> Would it make sense for this to be per-cpu?
> > >>
> > >> It's probably better to either just make it __read_mostly or get the
> > >> static branches that folks were suggesting actually working.
> > > 
> > > I still wonder if this isn't just better as a boot command line
> > 
> > It's simpler that way.  But, ideally, we want to make it runtime
> > switchable to match the implementation in the distros.
> 
> Stop this silly argument please. The distros shipped lots of crap which we
> dont want to have at all.
> 
> I told you folks yesterday what I want to see and the sysctl thing is the
> least on that list and it's not needed for getting the important thing -
> the protection - to work.

I agree. But this is what customers are told to inspect to see if they
are impacted. And if in the future versions this goes away or such - they
will freak out and cause needless escalations.

> 
> Can we pretty please do the basics and worry about that sysctl or whatever
> people have on their wishlist once the dust settled.
> 
> Thanks,
> 
> 	tglx
> 

RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Van De Ven, Arjan]

> I agree. But this is what customers are told to inspect to see if they
> are impacted. And if in the future versions this goes away or such - they
> will freak out and cause needless escalations.


it's a mistake though... retpoline is what people should be using ....
... and only in super exception cases should IBRS even be considered

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Konrad Rzeszutek Wilk]

On Sat, Jan 06, 2018 at 09:34:01PM +0000, Van De Ven, Arjan wrote:
> > I agree. But this is what customers are told to inspect to see if they
> > are impacted. And if in the future versions this goes away or such - they
> > will freak out and cause needless escalations.
> 
> 
> it's a mistake though... retpoline is what people should be using ....
> ... and only in super exception cases should IBRS even be considered

Like on certain Skylake and Broadwell where using the retpoline won't suffice?

As you can imagine having an heuristic that will figure out if:
 - The CPU is doing the correct thing, retpoline or IBRS is not needed at all.
 - The CPU can do retpoline, use that instead.
 - The CPU is unsafe to do retpoline, use IBRS.
 - The CPU can do retpoline, but MSRS are faster (is that even the case?)
 - The CPU hasn't been updated, the retpolines are unsafe, the only solution is to buy a new CPU.

And have all of this nicely rolled out to customers in an 'sysfs' that will
tell the customers - yes your are safe. 

All I m saying is that the 'ibrs_enabled' is a bad name, it should be something else
altogether.

RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Van De Ven, Arjan]

> > it's a mistake though... retpoline is what people should be using ....
> > ... and only in super exception cases should IBRS even be considered
> 
> Like on certain Skylake and Broadwell where using the retpoline won't suffice?

skylake is a bit more complex but that was discussed in earlier emails where a few more things are needed. No idea what you're inventing on broadwell where you would use ibrs but not retpoline


>  - The CPU can do retpoline, but MSRS are faster (is that even the case?)

that is never the case

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Thomas Gleixner]

On Sat, 6 Jan 2018, Konrad Rzeszutek Wilk wrote:
> On Sat, Jan 06, 2018 at 08:47:19PM +0100, Thomas Gleixner wrote:
> > On Sat, 6 Jan 2018, Dave Hansen wrote:
> > 
> > > On 01/06/2018 09:41 AM, Van De Ven, Arjan wrote:
> > > >>>>  .macro DISABLE_IBRS
> > > >>>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> > > >>>> +	testl	$1, dynamic_ibrs
> > > >>> On every system call we end up hammering on this 'dynamic_ibrs'
> > > >>> variable. And it looks like it can be flipped via the IPI mechanism.
> > > >>>
> > > >>> Would it make sense for this to be per-cpu?
> > > >>
> > > >> It's probably better to either just make it __read_mostly or get the
> > > >> static branches that folks were suggesting actually working.
> > > > 
> > > > I still wonder if this isn't just better as a boot command line
> > > 
> > > It's simpler that way.  But, ideally, we want to make it runtime
> > > switchable to match the implementation in the distros.
> > 
> > Stop this silly argument please. The distros shipped lots of crap which we
> > dont want to have at all.
> > 
> > I told you folks yesterday what I want to see and the sysctl thing is the
> > least on that list and it's not needed for getting the important thing -
> > the protection - to work.
> 
> I agree. But this is what customers are told to inspect to see if they
> are impacted. And if in the future versions this goes away or such - they
> will freak out and cause needless escalations.

That's the result of distros cramming stuff into their kernels without
talking to us. It's their problem to explain that their customers.

We can talks about the sysctl _AFTER_ fixing the real issues.

Thanks,

	tglx

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/06/2018 09:33 AM, Dave Hansen wrote:
> On 01/06/2018 06:41 AM, Konrad Rzeszutek Wilk wrote:
>>>  .macro DISABLE_IBRS
>>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>>> +	testl	$1, dynamic_ibrs
>> On every system call we end up hammering on this 'dynamic_ibrs'
>> variable. And it looks like it can be flipped via the IPI mechanism.
>>
>> Would it make sense for this to be per-cpu?
> 
> It's probably better to either just make it __read_mostly or get the
> static branches that folks were suggesting actually working.
> 

dynamic_ibrs is indeed declared __read_mostly

+unsigned int dynamic_ibrs __read_mostly;
+EXPORT_SYMBOL_GPL(dynamic_ibrs);

Tim

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/06/2018 06:41 AM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
>> From: Tim Chen <tim.c.chen@linux.intel.com>
>> From: Andrea Arcangeli <aarcange@redhat.com>
>>
>
>>  
>>  .macro DISABLE_IBRS
>> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>> +	testl	$1, dynamic_ibrs
> 
> On every system call we end up hammering on this 'dynamic_ibrs'
> variable. And it looks like it can be flipped via the IPI mechanism.

On system call, we read dynamic_ibrs value (not change it) and
flip the IBRS msr only if it dynamic_ibrs is true.

We only do global change to all the IBRS msrs on all cpus during
the admin request to change its value, serialized by
spec_ctrl_mutex.  Before we do that, we set dynamic_ibrs to 0,
so each cpu no longer do any change to IBRS.  Then the IPI happens
to update the IBRS MSR values.

> 
> Would it make sense for this to be per-cpu?
> 

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
> +{
> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {

We should test X86_BUG_SPECTRE_V1 here too before default enabling this,
no?

> +		if (!ibrs_admin_disabled) {
> +			dynamic_ibrs = 1;
> +			ibrs_enabled = IBRS_ENABLED;
> +		}
> +	}
> +}

RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Van De Ven, Arjan]

> > +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
> 
> We should test X86_BUG_SPECTRE_V1 here too before default enabling this,
> no?


we shouldn't default enable this.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/08/2018 07:29 AM, Van De Ven, Arjan wrote:
>>> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
>>
>> We should test X86_BUG_SPECTRE_V1 here too before default enabling this,
>> no?
> 
> 
> we shouldn't default enable this.
> 

Patch 7 disables ibrs if retpoline is in use.

Tim

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> +bool ibrs_inuse(void);

> +bool ibrs_inuse(void)
> +{
> +	return ibrs_enabled == IBRS_ENABLED;
> +}
> +EXPORT_SYMBOL_GPL(ibrs_inuse);

Doesn't appear to actually be used anywhere...

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> +static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
> +{
> +	int cpu;
> +
> +	get_online_cpus();
> +	for_each_online_cpu(cpu)
> +		wrmsrl_on_cpu(cpu, msr_nr, val);
> +	put_online_cpus();
> +}
> +
> +static ssize_t ibrs_enabled_write(struct file *file,
> +				  const char __user *user_buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	char buf[32];
> +	ssize_t len;
> +	unsigned int enable;
> +
> +	len = min(count, sizeof(buf) - 1);
> +	if (copy_from_user(buf, user_buf, len))
> +		return -EFAULT;
> +
> +	buf[len] = '\0';
> +	if (kstrtouint(buf, 0, &enable))
> +		return -EINVAL;
> +
> +	if (enable > IBRS_MAX)
> +		return -EINVAL;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> +		ibrs_enabled = IBRS_DISABLED;
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&spec_ctrl_mutex);
> +
> +	if (enable == IBRS_DISABLED) {
> +		/* disable IBRS usage */
> +		ibrs_admin_disabled = true;
> +		dynamic_ibrs = 0;
> +		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
> +					 SPEC_CTRL_FEATURE_DISABLE_IBRS);
> +
> +	} else if (enable == IBRS_ENABLED) {
> +		/* enable IBRS usage in kernel */
> +		ibrs_admin_disabled = false;
> +		dynamic_ibrs = 1;
> +
> +	} else if (enable == IBRS_ENABLED_USER) {
> +		/* enable IBRS all the time in both userspace and kernel */
> +		ibrs_admin_disabled = false;
> +		dynamic_ibrs = 0;
> +		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
> +					 SPEC_CTRL_FEATURE_ENABLE_IBRS);

So what about the CPUs that were offline when you did this?

> +	}
> +
> +	ibrs_enabled = enable;
> +
> +	mutex_unlock(&spec_ctrl_mutex);
> +	return count;
> +}

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> +static ssize_t ibrs_enabled_write(struct file *file,
> +				  const char __user *user_buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	char buf[32];
> +	ssize_t len;
> +	unsigned int enable;
> +
> +	len = min(count, sizeof(buf) - 1);
> +	if (copy_from_user(buf, user_buf, len))
> +		return -EFAULT;
> +
> +	buf[len] = '\0';
> +	if (kstrtouint(buf, 0, &enable))
> +		return -EINVAL;
> +
> +	if (enable > IBRS_MAX)
> +		return -EINVAL;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> +		ibrs_enabled = IBRS_DISABLED;
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&spec_ctrl_mutex);
> +
> +	if (enable == IBRS_DISABLED) {
> +		/* disable IBRS usage */
> +		ibrs_admin_disabled = true;
> +		dynamic_ibrs = 0;
> +		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
> +					 SPEC_CTRL_FEATURE_DISABLE_IBRS);
> +
> +	} else if (enable == IBRS_ENABLED) {
> +		/* enable IBRS usage in kernel */
> +		ibrs_admin_disabled = false;
> +		dynamic_ibrs = 1;

I think you need to do:

		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
					 SPEC_CTRL_FEATURE_ENABLE_IBRS);

here as well, to force all CPUs into a known state.

> +
> +	} else if (enable == IBRS_ENABLED_USER) {
> +		/* enable IBRS all the time in both userspace and kernel */
> +		ibrs_admin_disabled = false;
> +		dynamic_ibrs = 0;
> +		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
> +					 SPEC_CTRL_FEATURE_ENABLE_IBRS);
> +	}
> +
> +	ibrs_enabled = enable;
> +
> +	mutex_unlock(&spec_ctrl_mutex);
> +	return count;
> +}

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Borislav Petkov]

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.chen@linux.intel.com>
> From: Andrea Arcangeli <aarcange@redhat.com>

There's Co-Developed-by for this:

 - Co-Developed-by: states that the patch was also created by another developer
   along with the original author.  This is useful at times when multiple
   people work on a single patch.  Note, this person also needs to have a
   Signed-off-by: line in the patch as well.

> There are 2 ways to control IBRS
> 
> 1. At boot time
>     noibrs kernel boot parameter will disable IBRS usage
> 
> Otherwise if the above parameters are not specified, the system
> will enable ibrs and ibpb usage if the cpu supports it.

s/cpu/CPU/

> 
> 2. At run time
>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
> 
> The implementation was updated with input and suggestions from Andrea Arcangeli.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  arch/x86/entry/calling.h         |  42 ++++++++--
>  arch/x86/include/asm/spec_ctrl.h |  15 ++++
>  arch/x86/kernel/cpu/Makefile     |   1 +
>  arch/x86/kernel/cpu/scattered.c  |   2 +
>  arch/x86/kernel/cpu/spec_ctrl.c  | 160 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 214 insertions(+), 6 deletions(-)
>  create mode 100644 arch/x86/include/asm/spec_ctrl.h
>  create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
> 
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 09c870d..6b65d47 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with
>  .endm
>  
>  .macro ENABLE_IBRS
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>  	POP_MSR_REGS
> +	jmp	.Ldone_\@
> +
>  .Lskip_\@:
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro DISABLE_IBRS
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>  	POP_MSR_REGS
> +
>  .Lskip_\@:
>  .endm
>  
>  .macro ENABLE_IBRS_CLOBBER
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> +	jmp	.Ldone_\@
> +
>  .Lskip_\@:
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro DISABLE_IBRS_CLOBBER
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
> +
>  .Lskip_\@:
>  .endm
>  
>  .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	movl	$MSR_IA32_SPEC_CTRL, %ecx
>  	rdmsr
>  	movl	%eax, \save_reg
> @@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel is built with
>  	movl	$0, %edx
>  	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
>  	wrmsr
> +	jmp	.Ldone_\@
> +
>  .Lskip_\@:
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro RESTORE_IBRS_CLOBBER save_reg:req
> -	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	testl	$1, dynamic_ibrs
> +	jz	.Lskip_\@
> +
>  	/* Set IBRS to the value saved in the save_reg */
>  	movl    $MSR_IA32_SPEC_CTRL, %ecx
>  	movl    $0, %edx
>  	movl    \save_reg, %eax
>  	wrmsr
> +	jmp	.Ldone_\@
> +
>  .Lskip_\@:
> +	lfence
> +.Ldone_\@:
>  .endm

Why not put all macros in spec_ctrl.h ?

> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> new file mode 100644
> index 0000000..4fda38b
> --- /dev/null
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_X86_SPEC_CTRL_H
> +#define _ASM_X86_SPEC_CTRL_H
> +
> +#include <asm/msr-index.h>
> +#include <asm/cpufeatures.h>
> +#include <asm/microcode.h>

Left over include from a previous version.

Instead, you need

#include <linux/cpu.h>

in spec_ctrl.c for get/put_online_cpus().

> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);
> +bool ibrs_inuse(void);
> +
> +extern unsigned int dynamic_ibrs;
> +
> +#endif /* _ASM_X86_SPEC_CTRL_H */
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 570e8bb..3ffbd24 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -24,6 +24,7 @@ obj-y			+= match.o
>  obj-y			+= bugs.o
>  obj-y			+= aperfmperf.o
>  obj-y			+= cpuid-deps.o
> +obj-y			+= spec_ctrl.o
>  
>  obj-$(CONFIG_PROC_FS)	+= proc.o
>  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index bc50c40..5756d14 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -8,6 +8,7 @@
>  #include <asm/processor.h>
>  
>  #include <asm/apic.h>
> +#include <asm/spec_ctrl.h>
>  
>  struct cpuid_bit {
>  	u16 feature;
> @@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  		if (regs[cb->reg] & (1 << cb->bit))
>  			set_cpu_cap(c, cb->feature);
>  	}
> +	scan_spec_ctrl_feature(c);

Hell no!

This function is only for setting the feature bits.

>  u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
> new file mode 100644
> index 0000000..1641bec
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/mutex.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/spec_ctrl.h>
> +#include <asm/cpufeature.h>
> +
> +unsigned int dynamic_ibrs __read_mostly;

WTH is dynamic_ibrs?

> +EXPORT_SYMBOL_GPL(dynamic_ibrs);
> +
> +enum {
> +	IBRS_DISABLED,
> +	/* in host kernel, disabled in guest and userland */
> +	IBRS_ENABLED,
> +	/* in host kernel and host userland, disabled in guest */
> +	IBRS_ENABLED_USER,
> +	IBRS_MAX = IBRS_ENABLED_USER,
> +};
> +static unsigned int ibrs_enabled;
> +static bool ibrs_admin_disabled;

Srsly?!

That's *three* variables controlling IBRS. This needs simplification.

> +
> +/* mutex to serialize IBRS control changes */
> +DEFINE_MUTEX(spec_ctrl_mutex);

static

> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
> +{
> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {

What is !c->cpu_index? Checking whether this is the BSP? What for?

> +		if (!ibrs_admin_disabled) {
> +			dynamic_ibrs = 1;
> +			ibrs_enabled = IBRS_ENABLED;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);
> +
> +/*
> + * Used after boot phase to rescan spec_ctrl feature,
> + * serialize scan with spec_ctrl_mutex.
> + */
> +void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c)
> +{
> +	mutex_lock(&spec_ctrl_mutex);
> +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> +		if (!ibrs_admin_disabled) {
> +			dynamic_ibrs = 1;
> +			ibrs_enabled = IBRS_ENABLED;
> +		}
> +	}
> +	mutex_unlock(&spec_ctrl_mutex);
> +}
> +EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature);
> +
> +bool ibrs_inuse(void)
> +{
> +	return ibrs_enabled == IBRS_ENABLED;
> +}

So from looking at this, you can have a single ibrs_state variable and
set bits in it and have *all* possible settings you wanna have.

> +EXPORT_SYMBOL_GPL(ibrs_inuse);

That naming is not really correct: otherwise IBRS_ENABLED_USER means
ibrs is *not* in use.

> +
> +static int __init noibrs(char *str)
> +{
> +	ibrs_admin_disabled = true;
> +	ibrs_enabled = IBRS_DISABLED;
> +
> +	return 0;
> +}
> +early_param("noibrs", noibrs);
> +
> +static ssize_t __enabled_read(struct file *file, char __user *user_buf,
> +			      size_t count, loff_t *ppos, unsigned int *field)
> +{
> +	char buf[32];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%d\n", READ_ONCE(*field));
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t ibrs_enabled_read(struct file *file, char __user *user_buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	return __enabled_read(file, user_buf, count, ppos, &ibrs_enabled);
> +}
> +
> +static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
> +{
> +	int cpu;
> +
> +	get_online_cpus();
> +	for_each_online_cpu(cpu)
> +		wrmsrl_on_cpu(cpu, msr_nr, val);
> +	put_online_cpus();
> +}
> +
> +static ssize_t ibrs_enabled_write(struct file *file,
> +				  const char __user *user_buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	char buf[32];
> +	ssize_t len;
> +	unsigned int enable;
> +
> +	len = min(count, sizeof(buf) - 1);
> +	if (copy_from_user(buf, user_buf, len))
> +		return -EFAULT;
> +
> +	buf[len] = '\0';
> +	if (kstrtouint(buf, 0, &enable))
> +		return -EINVAL;
> +
> +	if (enable > IBRS_MAX)
> +		return -EINVAL;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {

That capability check needs to happen first: no need to do anything if
the CPU doesn't have IBRS.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen]

On 01/08/2018 04:29 PM, Borislav Petkov wrote:
> On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
>> From: Tim Chen <tim.c.chen@linux.intel.com>
>> From: Andrea Arcangeli <aarcange@redhat.com>
> 
> There's Co-Developed-by for this:
> 
>  - Co-Developed-by: states that the patch was also created by another developer
>    along with the original author.  This is useful at times when multiple
>    people work on a single patch.  Note, this person also needs to have a
>    Signed-off-by: line in the patch as well.
> 

Thanks.  Will do.


>>  .Lskip_\@:
>> +	lfence
>> +.Ldone_\@:
>>  .endm
> 
> Why not put all macros in spec_ctrl.h ?

There were a previous discussion thread in v1 patch with Peter Z and Dave.
Peter and Dave prefer that all these entrance macros be consolidated
in calling.h

> 
>> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
>> new file mode 100644
>> index 0000000..4fda38b
>> --- /dev/null
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef _ASM_X86_SPEC_CTRL_H
>> +#define _ASM_X86_SPEC_CTRL_H
>> +
>> +#include <asm/msr-index.h>
>> +#include <asm/cpufeatures.h>
>> +#include <asm/microcode.h>
> 
> Left over include from a previous version.
> 
> Instead, you need
> 
> #include <linux/cpu.h>
> 
> in spec_ctrl.c for get/put_online_cpus().
> 
>> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);
>> +bool ibrs_inuse(void);
>> +
>> +extern unsigned int dynamic_ibrs;
>> +
>> +#endif /* _ASM_X86_SPEC_CTRL_H */
>> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> index 570e8bb..3ffbd24 100644
>> --- a/arch/x86/kernel/cpu/Makefile
>> +++ b/arch/x86/kernel/cpu/Makefile
>> @@ -24,6 +24,7 @@ obj-y			+= match.o
>>  obj-y			+= bugs.o
>>  obj-y			+= aperfmperf.o
>>  obj-y			+= cpuid-deps.o
>> +obj-y			+= spec_ctrl.o
>>  
>>  obj-$(CONFIG_PROC_FS)	+= proc.o
>>  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
>> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
>> index bc50c40..5756d14 100644
>> --- a/arch/x86/kernel/cpu/scattered.c
>> +++ b/arch/x86/kernel/cpu/scattered.c
>> @@ -8,6 +8,7 @@
>>  #include <asm/processor.h>
>>  
>>  #include <asm/apic.h>
>> +#include <asm/spec_ctrl.h>
>>  
>>  struct cpuid_bit {
>>  	u16 feature;
>> @@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>>  		if (regs[cb->reg] & (1 << cb->bit))
>>  			set_cpu_cap(c, cb->feature);
>>  	}
>> +	scan_spec_ctrl_feature(c);
> 
> Hell no!
> 
> This function is only for setting the feature bits.
> 
>>  u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
>> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
>> new file mode 100644
>> index 0000000..1641bec
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
>> @@ -0,0 +1,160 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <asm/spec_ctrl.h>
>> +#include <asm/cpufeature.h>
>> +
>> +unsigned int dynamic_ibrs __read_mostly;
> 
> WTH is dynamic_ibrs?

Dynamic ibrs because we enable the IBRS in MSR 0x48
entering the kernel and disable it when we exit the
kernel.
 
> 
>> +EXPORT_SYMBOL_GPL(dynamic_ibrs);
>> +
>> +enum {
>> +	IBRS_DISABLED,
>> +	/* in host kernel, disabled in guest and userland */
>> +	IBRS_ENABLED,
>> +	/* in host kernel and host userland, disabled in guest */
>> +	IBRS_ENABLED_USER,
>> +	IBRS_MAX = IBRS_ENABLED_USER,
>> +};
>> +static unsigned int ibrs_enabled;
>> +static bool ibrs_admin_disabled;
> 
> Srsly?!
> 
> That's *three* variables controlling IBRS. This needs simplification.
> 
>> +
>> +/* mutex to serialize IBRS control changes */
>> +DEFINE_MUTEX(spec_ctrl_mutex);
> 
> static
> 
>> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
>> +{
>> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
> 
> What is !c->cpu_index? Checking whether this is the BSP? What for?

This is to ensure that we only do the operation once during boot.

Thanks.

Tim