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

[Borislav Petkov <bp@alien8.de>]

On Mon, Oct 18, 2021 at 09:06:07PM +0100, Andrew Cooper wrote:
> ... this is 0x18 for Hygon, and ...

Sure, whatever :)

> >
> > 	/* All the remaining ones are affected */
> > 	set_cpu_bug(c, X86_BUG_NULL_SEG);
> 
> ... hypervisor && !ncsb still needs to set BUG_NULL_SEG, so you really
> can't exit early.

Yeah, we had a session on IRC, we came up with this rough version, more
polishing tomorrow:

static void early_probe_null_seg_clearing_base(struct cpuinfo_x86 *c)
{

        /* Zen3 CPUs advertise Null Selector Clears Base in CPUID. */
        if (c->extended_cpuid_level >= 0x80000021 && cpuid_eax(0x80000021) & BIT(6))
                return;

	/*
	 * CPUID bit above wasn't set. If this kernel is still running as a HV guest,
	 * then the HV has decided not to advertize that CPUID bit for whatever reason.
	 * For example, one member of the migration pool might be vulnerable.
	 * Which means, the bug is present: set the BUG flag and return.
	 */
        if (cpu_has(c, X86_FEATURE_HYPERVISOR)) {
		set_cpu_bug(c, X86_BUG_NULL_SEG);
                return;
	}

        /* Zen2 CPUs also have this behaviour, but no CPUID bit. 0x18 for Hygon. */
        if ((c->x86 == 0x17 || c->x86 == 0x18) && check_null_seg_clears_base(c))
                return;

        /* All the remaining ones are affected */
        set_cpu_bug(c, X86_BUG_NULL_SEG);
}

So I really want to have those comments explaining each step in the
complex check because we will forget why this crazy dance is being done
and as I said in a previous thread, we're not all virtualizers. :)

> No other CPU vendors are known to have this issue.

How do you know? Or should there be a comment along the lines of "Cooper
says that..."

:-)

> (And by "issue", even this is complicated.  Back in the 32bit
> days, it was a plausible perf improvement, but it backfired massively
> for AMD64 where there was a possibility/expectation to use NULL
> segments.)
> 
> Andy only put the check in unilaterally just in case, and even that was
> fine-ish until AMD went and fixed it silently in Zen2.

Yeah, there's the context switch overhead too but that's for another
thread.

> > Because if this null seg behavior detection should happen on all
> > CPUs - and I think it should, because, well, it has been that way
> > until now - then the vendor specific identification minus what
> > detect_null_seg_behavior() does should run first and then after
> > ->c_identify() is done, you should do something like:
> >
> >  if (!cpu_has_bug(c, X86_BUG_NULL_SEG)) { if
> >  (!check_null_seg_clears_base(c)) set_cpu_bug(c, X86_BUG_NULL_SEG);
> >  }
> >
> > so that it still takes place on all CPUs.
>
> This would only really work for boot cpu and setup_force_cap(),
> because no CPU is going to have X86_BUG_NULL_SEG set by
> default, but this still misses the point of the bugfix which
> is "check_null_seg_clears_base() must not be called when
> cpu_has_hypervisor".
>
> In practice, the BSP is good enough.  The behaviour predates the
> K8, which was the first CPU where it became observable without
> SMM/PUSHALL/etc, and quite possibly goes back to the dawn of time, and
> you can't mix a Zen1 and Zen2 in a 2-socket system.

Oh, I didn't express myself properly "should happen on all CPUs" was
supposed to mean, if this detection should happen on all vendors like it
does now. Not BSP vs AP.

> It is made unused by this patch, so can't be pulled out earlier, but
> should be adjusted.

Right.

-- 
Regards/Gruss,
    Boris.

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