[MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2

[mark gross <mgross@linux.intel.com>]

On Fri, Mar 27, 2020 at 12:37:10PM -0500, speck for Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 09:20:41AM -0700, speck for mark gross wrote:
> > > > +
> > > > +			off:	disable mitigation and remove performance
> > > > +				impact to rdrand and rdseed
> > > > +
> > > >  	srcutree.counter_wrap_check [KNL]
> > > >  			Specifies how frequently to check for
> > > >  			grace-period sequence counter wrap for the
> > > > @@ -397,6 +399,103 @@ static int __init tsx_async_abort_parse_cmdline(char *str)
> > > >  }
> > > >  early_param("tsx_async_abort", tsx_async_abort_parse_cmdline);
> > > >  
> > > > +#undef pr_fmt
> > > > +#define pr_fmt(fmt)	"SRBDS: " fmt
> > > > +
> > > > +enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL;
> > > > +static const char * const srbds_strings[] = {
> > > > +	[SRBDS_NOT_AFFECTED]		= "Not affected",
> > > 
> > > I don't think the SRBDS_NOT_AFFECTED enum is needed.
> > > 
> > > The "Not affected" string is already printed by cpu_show_common().
> > > 
> > > And srbds_configure_mitigation() can just check for
> > > boot_cpu_has_bug(X86_BUG_SRBDS) instead of this enum value.
> > 
> > True.  I prefer having the complete state machine represented by the
> > enumeration but, if you feel strongly I'll try to remove it.
> 
> If the state isn't actually used by the sysfs printing code then it just
> serves to add confusion for a reader of the code.
> 
> And notice none of the other mitigations have this.  It's a subtle
> inconsistency, which is a recipe for future bugs.
I'll remove it.


> > > > +	[SRBDS_MITIGATION_OFF]		= "Vulnerable",
> > > > +	[SRBDS_MITIGATION_UCODE_NEEDED]	= "Vulnerable: no microcode",
> > > > +	[SRBDS_MITIGATION_FULL]		= "Mitigated",
> > > > +	[SRBDS_NOT_AFFECTED_TSX_OFF]	= "Not affected (TSX disabled)",
> > > > +	[SRBDS_HYPERVISOR]		= "Unknown",
> > > > +};
> > > > +
> > > > +static bool srbds_off;
> > > > +
> > > > +void srbds_configure_mitigation(void)
> > > 
> > > To me, "configure" sounds like it could be a software thing.
> > We are configuring the meditation by honoring the kernel command line in this
> > function.
> > 
> > > 
> > > How about update_srbds_msr(), which is also more consistent with some of
> > > the other naming in the file.
> > 
> > If you like but, I was mimicking the TAA implementation when choosing the name
> > of this function.
> 
> Hm, which TAA function name were you mimicking?

I thought taa_select_mitigation but, I see what you are talkng about.  I split
out the mitication selection and the application of the policy.

I'll change the name to update_srbds_msr.


> 
> > > > +static void __init srbds_select_mitigation(void)
> > > > +{
> > > > +	u64 ia32_cap;
> > > > +
> > > > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > > > +		srbds_mitigation = SRBDS_HYPERVISOR;
> > > > +		return;
> > > > +	}
> > > 
> > > It's confusing that this comes before the X86_BUG_SRBDS check.  Is that
> > > on purpose?
> > VM vendors on keybase wanted it so.
> > 
> > > 
> > > If so, I presume it doesn't have the intended effect, because if booting
> > > in a guest on a non-vulnerable CPU, srbds_mitigation will get set to
> > > SRBDS_HYPERVISOR, but the shown state will still be "Not affected"
> > > because of how cpu_show_common() checks for the bug at the beginning.
> > 
> > The intended effect is to report "Unknown" for the vulnerability status of
> > SRBDS for all guest VM's running this code and to never touch the MSR
> > controlling the mitigation.
> > 
> > I think this is what happens.  I'll double check and confirm with testing.
> 
> Maybe I'm blind, but given how cpu_show_common() works, I don't see how
> that could possibly happen when boot_cpu_has_bug(X86_BUG_SRBDS) is
> false.  If I'm wrong please enlighten me.
Maybe I'm blind.  Your right.  my qemu test boot on an old server part
workstation that is not affected by SRBDS is reporting "not affected" when it
I intended it to be reporting "Unknow"  (which if you saw Tony's email is also
not what we want now that I ahve clarification from Tony.)

> > > > +
> > > > +	if (!boot_cpu_has_bug(X86_BUG_SRBDS)) {
> > > > +		srbds_mitigation = SRBDS_NOT_AFFECTED;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL)) {
> > > > +		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (boot_cpu_has_bug(X86_BUG_SRBDS)) {
> > > > +		srbds_mitigation = SRBDS_MITIGATION_FULL;
> > > > +		/*
> > > > +		 * Check to see if this is one of the MDS_NO systems supporting
> > > > +		 * TSX that are only exposed to SRBDS when TSX is enabled.
> > > > +		 */
> > > > +		ia32_cap = x86_read_arch_cap_msr();
> > > > +		if (ia32_cap & ARCH_CAP_MDS_NO) {
> > > > +			if (!boot_cpu_has(X86_FEATURE_RTM))
> > > > +				srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF;
> > > > +		}
> > > 
> > > I wonder if this would be equivalent?
> > > 
> > > 		if (!boot_cpu_has_bug(X86_BUG_MDS) && boot_cpu_has(X86_BUG_TAA) &&
> > > 		    !boot_cpu_has(X86_FEATURE_RTM))
> > > 		    	srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF;
> > I have no clue if its equivalent but, I find your version much harder to parse
> > and my version matches more closely to what the white paper states.
> 
> Ok.
> 
> > > > +	}
> > > > +
> > > > +	if (cpu_mitigations_off() || srbds_off) {
> > > > +		if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF)
> > > > +			srbds_mitigation = SRBDS_MITIGATION_OFF;
> > > > +	}
> > > > +
> > > > +	srbds_configure_mitigation();
> > > > +}
> > > 
> > > The other mitigations do a printk with the mitigation status here.
> > > Should we not do the same here?
> > Perhaps but, I was attempting to limit the extra chatter in the kernel logs.
> > If you feel it adds value to the code I'll put it in.  FWIW I think the chatter
> > from the other places should be removed.
> 
> It should at least be consistent with the others.
ok.


> > > > +ssize_t cpu_show_special_register_data_sampling(struct device *dev, struct device_attribute *attr, char *buf)
> > > > +{
> > > 
> > > This function name can be renamed to match whatever the filename turns
> > > out to be:
> > > 
> > >   cpu_show_srbds()
> > > 
> > > or
> > > 
> > >   cpu_show_special_register_data_buffer_sampling()
> > 
> > I got some early review feedback to not use the shorter name "srbds" for the
> > sysfs inodes so I was trying to avoid the full spelled out the full names.
> > 
> > "srbds" and "special_register_data_buffer_sampling" do not imply HWRNG security
> > risk so IMO I'd like to just use srbds.  What do others think?
> 
> +1 for "srbds" for both file name and function name.
thanks!


> > > > +void srbds_configure_mitigation(void);
> > > > +
> > > >  #ifdef CONFIG_CPU_SUP_INTEL
> > > > +
> > > >  enum tsx_ctrl_states {
> > > >  	TSX_CTRL_ENABLE,
> > > >  	TSX_CTRL_DISABLE,
> > > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > > > index be82cd5841c3..1b083a2a415b 100644
> > > > --- a/arch/x86/kernel/cpu/intel.c
> > > > +++ b/arch/x86/kernel/cpu/intel.c
> > > > @@ -684,6 +684,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> > > >  		tsx_enable();
> > > >  	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
> > > >  		tsx_disable();
> > > > +
> > > > +	srbds_configure_mitigation();
> > > >  }
> > > 
> > > I'm not sure this is the right place to call this.  On the boot CPU,
> > > this gets called *before* the mitigation is selected, and then again
> > > from srbds_select_mitigation() itself.
> > > 
> > > Maybe it should instead be called from identify_secondary_cpu(), while
> > > keeping the existing call from srbds_select_mitigation() so the boot CPU
> > > also gets updated.
> > The SRBDS MSR needs to be poked from each CPU thread if you want to disable the
> > mitigation.  Does identify_secondary_cpu get called per HW thread?
> 
> identify_secondary_cpu() gets called for every HW thread, other than the
> boot one.
> 
> The boot one already does it in srbds_select_mitigation().
> 
> > I felt it would be nice to keep the call close to other mitigation setup
> > logic, in this case the TAA logic.
> 
> That's not the TAA logic, but rather the TSX logic (though yes I realize
> they're related).  And anyway SRBDS and TSX have different requirements.
> 
> > What do you think it will be more readable if I move it to
> > identify_secondary_cpu?
> 
> As I said - it gets called *twice* on the boot CPU.  The first time is
> before srbds_mitigation has even been configured.  That's confusing.
I'll move it to identify_secondary_cpu.

--mark


> -- 
> Josh