Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Borislav Petkov <bp@alien8.de>]

On Fri, Oct 01, 2021 at 02:33:49PM +0100, Jane Malalane wrote:
> Subject: Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL
> ...
> Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
> makes it unsafe to migrate in a virtualised environment as the
> properties across the migration pool might differ.

Sorry but you need to explain "migration safety" in greater detail -
we're not all virtualizers.

> Zen3 adds the NullSelectorClearsBase bit to indicate that loading
> a NULL segment selector zeroes the base and limit fields, as well as
> just attributes. Zen2 also has this behaviour but doesn't have the
> NSCB bit.

I'm guessing you can detect Zen2 too, without the CPUID bit?

> When virtualised, NSCB might be cleared for migration safety,
> therefore we must not probe. Always honour the NSCB bit in this case,

Who is "we"?

> as the hypervisor is expected to synthesize it in the Zen2 case.
> 
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: <x86@kernel.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Pu Wen <puwen@hygon.cn>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Sean Christopherson <seanjc@google.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Yazen Ghannam <Yazen.Ghannam@amd.com>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> CC: Huang Rui <ray.huang@amd.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Kim Phillips <kim.phillips@amd.com>
> CC: <stable@vger.kernel.org>
> ---
>  arch/x86/include/asm/cpufeatures.h |  2 +-
>  arch/x86/kernel/cpu/amd.c          | 23 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/common.c       |  6 ++----
>  arch/x86/kernel/cpu/cpu.h          |  1 +
>  arch/x86/kernel/cpu/hygon.c        | 23 +++++++++++++++++++++++
>  5 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index d0ce5cfd3ac1..f571e4f6fe83 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -96,7 +96,7 @@
>  #define X86_FEATURE_SYSCALL32		( 3*32+14) /* "" syscall in IA32 userspace */
>  #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
>  #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
> -/* FREE!                                ( 3*32+17) */
> +#define X86_FEATURE_NSCB		( 3*32+17) /* Null Selector Clears Base */

You don't really need that bit definition - you check CPUID in the
early_init_amd/hygon() functions and that is enough. IOW, based on that
CPUID bit, you can call detect_null_seg_behavior() and set (or not)
X86_BUG_NULL_SEG.

...

> diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
> index 6d50136f7ab9..765f1556d964 100644
> --- a/arch/x86/kernel/cpu/hygon.c
> +++ b/arch/x86/kernel/cpu/hygon.c
> @@ -264,6 +264,29 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
>  	if (c->x86_power & BIT(14))
>  		set_cpu_cap(c, X86_FEATURE_RAPL);
>  
> +	/*
> +	 * Zen1 and earlier CPUs don't clear segment base/limits when
> +	 * loading a NULL selector.  This has been designated
> +	 * X86_BUG_NULL_SEG.
> +	 *
> +	 * Zen3 CPUs advertise Null Selector Clears Base in CPUID.
> +	 * Zen2 CPUs also have this behaviour, but no CPUID bit.
> +	 *
> +	 * A hypervisor may sythesize the bit, but may also hide it
> +	 * for migration safety, so we must not probe for model
> +	 * specific behaviour when virtualised.
> +	 */
> +	if (c->extended_cpuid_level >= 0x80000021 &&
> +	    cpuid_eax(0x80000021) & BIT(6))
> +	        set_cpu_cap(c, X86_FEATURE_NSCB);
> +
> +	if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_NSCB) &&
> +	    c->x86 == 0x18)
> +                detect_null_seg_behavior(c);
> +
> +	if (!cpu_has(c, X86_FEATURE_NSCB))
> +		set_cpu_bug(c, X86_BUG_NULL_SEG);

Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

I'll show you that there's whitespace damage here:

ERROR: code indent should use tabs where possible
#238: FILE: arch/x86/kernel/cpu/hygon.c:281:
+^I        set_cpu_cap(c, X86_FEATURE_NSCB);$

ERROR: code indent should use tabs where possible
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

WARNING: please, no spaces at the start of a line
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette