Date:   Tue, 9 Jan 2018 01:29:43 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable
 SPEC_CTRL feature

On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.chen@...ux.intel.com>
> From: Andrea Arcangeli <aarcange@...hat.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@...ux.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.

Powered by blists - more mailing lists