Re: [PATCH 6/7] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Tim Chen <tim.c.chen@linux.intel.com>]

On 01/04/2018 12:16 PM, Greg KH wrote:
> On Thu, Jan 04, 2018 at 09:56:47AM -0800, Tim Chen wrote:
>> 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/ibrs_enabled will turn off IBRS
>>     echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel
>>     echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel
>>
>> The implementation was updated with input from Andrea Arcangeli.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |   4 +
>>  arch/x86/include/asm/spec_ctrl.h                | 163 +++++++++++++++++++-----
>>  arch/x86/kernel/cpu/Makefile                    |   1 +
>>  arch/x86/kernel/cpu/scattered.c                 |  10 ++
>>  arch/x86/kernel/cpu/spec_ctrl.c                 | 124 ++++++++++++++++++
>>  5 files changed, 270 insertions(+), 32 deletions(-)
>>  create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 5dfd262..d64f49f 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2573,6 +2573,10 @@
>>  			noexec=on: enable non-executable mappings (default)
>>  			noexec=off: disable non-executable mappings
>>  
>> +	noibrs          [X86]
>> +			Don't use indirect branch restricted speculation (IBRS)
>> +			feature.
>> +
>>  	nosmap		[X86]
>>  			Disable SMAP (Supervisor Mode Access Prevention)
>>  			even if it is supported by processor.
>> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
>> index 23b2804..2c35571 100644
>> --- a/arch/x86/include/asm/spec_ctrl.h
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -1,13 +1,17 @@
>>  #ifndef _ASM_X86_SPEC_CTRL_H
>>  #define _ASM_X86_SPEC_CTRL_H
>>  
>> -#include <linux/stringify.h>
>>  #include <asm/msr-index.h>
>>  #include <asm/cpufeatures.h>
>> -#include <asm/alternative-asm.h>
>> +
>> +#define SPEC_CTRL_IBRS_INUSE      	(1<<0)	/* OS enables IBRS usage */
>> +#define SPEC_CTRL_IBRS_SUPPORTED	(1<<1)  /* System supports IBRS */
>> +#define SPEC_CTRL_IBRS_ADMIN_DISABLED	(1<<2)  /* Admin disables IBRS */
>>  
>>  #ifdef __ASSEMBLY__
>>  
>> +.extern spec_ctrl_ibrs
>> +
>>  .macro PUSH_MSR_REGS
>>  	pushq %rax
>>  	pushq %rcx
>> @@ -27,35 +31,63 @@
>>  .endm
>>  
>>  .macro ENABLE_IBRS
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> +	jz .Lskip_\@
>> +
>>  	PUSH_MSR_REGS
>>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>>  	POP_MSR_REGS
>> -10:
>> +
>> +	jmp .Ldone_\@
>> +.Lskip_\@:
>> +	/*
>> +	 * prevent speculation beyond here as we could want to
>> +	 * stop speculation by enabling IBRS
>> +	 */
>> +	lfence
>> +.Ldone_\@:
>>  .endm
>>  
>>  .macro DISABLE_IBRS
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> +	jz .Lskip_\@
>> +
>>  	PUSH_MSR_REGS
>>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>>  	POP_MSR_REGS
>> -10:
>> +
>> +.Lskip_\@:
>>  .endm
>>  
>>  .macro ENABLE_IBRS_CLOBBER
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> +	jz .Lskip_\@
>> +
>>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>> -10:
>> +
>> +	jmp .Ldone_\@
>> +.Lskip_\@:
>> +	/*
>> +	 * prevent speculation beyond here as we could want to
>> +	 * stop speculation by enabling IBRS
>> +	 */
>> +	lfence
>> +.Ldone_\@:
>>  .endm
>>  
>>  .macro DISABLE_IBRS_CLOBBER
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> +	jz .Lskip_\@
>> +
>>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>> -10:
>> +
>> +.Lskip_\@:
>>  .endm
>>  
>>  .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> +	jz .Lskip_\@
>> +
>>  	movl	$MSR_IA32_SPEC_CTRL, %ecx
>>  	rdmsr
>>  	movl	%eax, \save_reg
>> @@ -63,22 +95,103 @@
>>  	movl	$0, %edx
>>  	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
>>  	wrmsr
>> -10:
>> +
>> +	jmp .Ldone_\@
>> +.Lskip_\@:
>> +	/*
>> +	 * prevent speculation beyond here as we could want to
>> +	 * stop speculation by enabling IBRS
>> +	 */
>> +	lfence
>> +.Ldone_\@:
>>  .endm
>>  
>>  .macro RESTORE_IBRS_CLOBBER save_reg:req
>> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_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
>> -10:
>> +
>> +	jmp .Ldone_\@
>> +.Lskip_\@:
>> +	/*
>> +	 * prevent speculation beyond here as we could want to
>> +	 * stop speculation by enabling IBRS
>> +	 */
>> +	lfence
>> +.Ldone_\@:
>>  .endm
>>  
>>  #else
>>  #include <asm/microcode.h>
>>  
>> +extern int spec_ctrl_ibrs;
>> +extern struct mutex spec_ctrl_mutex;
>> +extern unsigned int ibrs_enabled;
>> +
>> +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,
> 
> Yup, not a bit field, your documentation is wrong :(
> 
>> +};
>> +
>> +
>> +static inline void set_ibrs_inuse(void)
>> +{
>> +	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> +		spec_ctrl_ibrs |= SPEC_CTRL_IBRS_INUSE;
>> +}
>> +
>> +static inline void clear_ibrs_inuse(void)
>> +{
>> +	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_INUSE;
>> +}
>> +
>> +static inline int ibrs_inuse(void)
>> +{
>> +	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_INUSE)
>> +		return 1;
>> +	else
>> +		/*
>> +		 * prevent speculation beyond here as we could want to
>> +		 * stop speculation by enabling IBRS with this check
>> +		 */
>> +		rmb();
>> +	return 0;
>> +}
>> +
>> +static inline void set_ibrs_supported(void)
>> +{
>> +	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_SUPPORTED;
>> +	if (!(spec_ctrl_ibrs & SPEC_CTRL_IBRS_ADMIN_DISABLED))
>> +		set_ibrs_inuse();
>> +	else
>> +		/*
>> +		 * prevent speculation beyond here as we could want to
>> +		 * stop speculation by enabling IBRS
>> +		 */
>> +		rmb();
>> +}
>> +
>> +static inline void set_ibrs_disabled(void)
>> +{
>> +	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_ADMIN_DISABLED;
>> +	if (ibrs_inuse())
>> +		clear_ibrs_inuse();
>> +}
>> +
>> +static inline void clear_ibrs_disabled(void)
>> +{
>> +	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_ADMIN_DISABLED;
>> +}
>> +
>>  static inline void __disable_indirect_speculation(void)
>>  {
>>          native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> @@ -96,21 +209,14 @@ static inline void __enable_indirect_speculation(void)
>>  static inline void unprotected_speculation_begin(void)
>>  {
>>  	WARN_ON_ONCE(!irqs_disabled());
>> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +	if (ibrs_inuse())
>>  		__enable_indirect_speculation();
>>  }
>>  
>>  static inline void unprotected_speculation_end(void)
>>  {
>> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +	if (ibrs_inuse())
>>  		__disable_indirect_speculation();
>> -	else
>> -		/*
>> -		 * If we intended to disable indirect speculation
>> -		 * but come here due to mis-speculation, we need
>> -		 * to stop the mis-speculation with rmb.
>> -		 */
>> -		rmb();
>>  }
>>  
>>  
>> @@ -121,20 +227,13 @@ static inline void unprotected_speculation_end(void)
>>   */
>>  static inline void unprotected_firmware_begin(void)
>>  {
>> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +	if (ibrs_inuse())
>>                  __disable_indirect_speculation();
>> -	else
>> -		/*
>> -		 * If we intended to disable indirect speculation
>> -		 * but come here due to mis-speculation, we need
>> -		 * to stop the mis-speculation with rmb.
>> -		 */
>> -		rmb();
>>  }
>>  
>>  static inline void unprotected_firmware_end(void)
>>  {
>> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +	if (ibrs_inuse())
>>                  __enable_indirect_speculation();
>>  }
>>  
>> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> index 90cb82d..a25f1ab 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-$(CONFIG_CPU_FREQ)	+= 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..2d23a2fe 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;
>> @@ -56,6 +57,15 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>>  
>>  		if (regs[cb->reg] & (1 << cb->bit))
>>  			set_cpu_cap(c, cb->feature);
>> +
>> +	}
>> +	if (!c->cpu_index) {
>> +		if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
>> +			printk(KERN_INFO "FEATURE SPEC_CTRL Present\n");
>> +			set_ibrs_supported();
>> +			if (ibrs_inuse())
>> +				ibrs_enabled = IBRS_ENABLED;
>> +		}
>>  	}
>>  }
>>  
>> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
>> new file mode 100644
>> index 0000000..6946678
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
>> @@ -0,0 +1,124 @@
>> +#include <linux/export.h>
> 
> No copyright or SPDX line?
> 
> At least it doesn't have the old horrid Intel header boilerplate, so I
> should be thankful of that.  But it isn't ok like this either, sorry.
> 
>> +#include <linux/init.h>
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <asm/spec_ctrl.h>
>> +#include <asm/cpufeature.h>
>> +
>> +/*
>> + * spec_ctrl_ibrs
>> + * bit 0 = indicate if ibrs is currently in use
>> + * bit 1 = indicate if system supports ibrs
>> + * bit 2 = indicate if admin disables ibrs
> 
> Why bits and not integer values?  Can you mix them?

spec_ctrl_ibrs has bit fields for internal speculation
control and is not exposed to user.
ibrs_enabled is reported to the user and takes the enum
values

+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,
+};
+


> 
>> + */
>> +
>> +int spec_ctrl_ibrs;
>> +EXPORT_SYMBOL(spec_ctrl_ibrs);
> 
> Why is this exported?  What module will ever need this?
> 
> And horrid global symbol name, it doesn't say what it is in an obvious
> way just by looking at it.
> 
>> +
>> +/* mutex to serialize IBRS control changes */
>> +DEFINE_MUTEX(spec_ctrl_mutex);
>> +EXPORT_SYMBOL(spec_ctrl_mutex);
> 
> Why export this?  You only ever use it in one place?  Again, what module
> needs it?

It is needed for places that potentially have more than one cpu
touching spec_ctrl_ibrs values.  I'll see if I can consolidate
them to spec_ctrl.c to avoid the export.

> 
> And I have to ask, EXPORT_SYMBOL_GPL() for this and spec_ctrl_ibrs as
> well?
> 
>> +unsigned int ibrs_enabled __read_mostly;
>> +EXPORT_SYMBOL(ibrs_enabled);
> 
> Again, what module needs this?
> 
> And did __read_mostly really matter in performance tests?
> 
>> +
>> +static int __init noibrs(char *str)
>> +{
>> +        set_ibrs_disabled();
>> +
>> +        return 0;
>> +}
>> +early_param("noibrs", ex);
>> +
>> +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);
>> +}
> 
> It's a single variable, why such complex debugfs for that?  This should
> be handled by a helper macro already, right?
> 
>> +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;
> 
> 3 value "flags" are horrid :(

Unfortunately, IBRS can have 3 different usage scenarios:
1. disabled all the time
2. used only in kernel and privileged code
3. enabled all the time in both user and kernel space.

> 
>> +
>> +        if (enable > IBRS_MAX)
>> +                return -EINVAL;
>> +
>> +	mutex_lock(&spec_ctrl_mutex);
> 
> Always run checkpatch.pl so you don't get grumpy kernel maintainers
> telling you to run checkpatch.pl :(
> 
>> +
>> +	if (enable == IBRS_DISABLED) {
>> +		/* disable IBRS usage */
>> +		set_ibrs_disabled();
>> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> +			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
>> +	} else if (enable == IBRS_ENABLED) {
>> +		/* enable IBRS usage in kernel */
>> +		clear_ibrs_disabled();
>> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> +			set_ibrs_inuse();
>> +		else
>> +			/* Platform don't support IBRS */
>> +			enable = IBRS_DISABLED;
>> +	} else if (enable == IBRS_ENABLED_USER) {
>> +		/* enable IBRS usage in both userspace and kernel */
>> +		clear_ibrs_disabled();
>> +		/* don't change IBRS value once we set it to always on */
>> +		clear_ibrs_inuse();
>> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> +			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> +		else
>> +			/* Platform don't support IBRS */
>> +			enable = IBRS_DISABLED;
>> +	}
>> +
>> +	WRITE_ONCE(ibrs_enabled, enable);
> 
> It's a debugfs write callback, why do you care about WRITE_ONCE()?

Probably not necessary now. It is serialized by spec_ctrl_mutex.

> 
> thanks,
> 
> greg k-h
>