[MODERATED] Re: [PATCH v2 2/2] v2: more sampling fun 2

On Wed, Feb 26, 2020 at 12:46:41PM +0100, speck for Borislav Petkov wrote:
> On Thu, Jan 16, 2020 at 02:16:07PM -0800, speck for mark gross wrote:
> > From: mark gross <[email protected]>
> > Subject: [PATCH v2 2/2] WIP SRBDS mitigation enabling.
> > 
> > From: mark gross <[email protected]>
> > Subject: [PATCH v2 2/2] WIP SRBDS mitigation enabling.
> 
> In addition to the notes in the previous mail, your subject needs a
> verb:
ok

> "Add ... mitigation" or so.
> 
> See
> 
> git log -p arch/x86/kernel/cpu/bugs.c
> 
> output for inspiration.
ok I'll take any insperation I can get.
thanks

> > SRBDS is an MDS-like speculative side channel that can leak bits from
> > the RNG across cores and threads. New microcode serializes the processor
> > access during the execution of RDRAND and RDSEED ensures that the shared
> > buffer is overwritten before it is released for reuse.
> > 
> > We subdivide processors that are vulnerable to SRBDS into two classes:
> 
> "We" is?
its just me for that instance of the word.

> > X86_BUG_SRBDS:          models that are vulnerable
> > X86_BUG_SRBDS_TSX:      models only vulnerable when TSX is enabled.
> > 
> > The latter are not vulnerable to SRBDS if TSX is disabled on all cores.
> > 
> > The mitigation is activated by default on affected processors and it slows
> > down /dev/urandom.  The latency of RDRAND and RDSEED instructions is
> > increased by 10x.  We don't expect this to be noticeable in most cases.
> 
> This "We" sounds like you mean Intel...?
yes, this instance of "we" is an intel statement from that white paper I'm
waiting on for the documentation patch.

> > This patch:
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
good point.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
"imperative mood"  ok.

> 
> > @@ -397,6 +399,69 @@ 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_MITIGATION_OFF]		= "Vulnerable",
> > +	[SRBDS_MITIGATION_UCODE_NEEDED]	= "Vulnerable: no microcode",
> > +	[SRBDS_MITIGATION_FULL]		= "Mitigation: bus lock when using RDRAND or RDSEED",
> > +};
> > +
> > +void srbds_configure_mitigation(void)
> > +{
> > +	u64 mcu_ctrl;
> > +
> > +	if (!boot_cpu_has_bug(X86_BUG_SRBDS) && !boot_cpu_has_bug(X86_BUG_SRBDS_TSX))
> > +		return;
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
> > +		return;
> > +
> > +	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> 
> What's up with virtualization? Is that MSR going to be exported or what?
Myself and a few Intel folks I talked to about virtualization aspects of this
don't expect it will be but, I think that is up to the VMM vendors.

> 
> If so, you need the _safe() accessors here.
> 
> > +	if (srbds_mitigation == SRBDS_MITIGATION_FULL)
> > +		mcu_ctrl &= ~SRBDS_MITG_DIS;
> > +	else if (srbds_mitigation == SRBDS_MITIGATION_OFF)
> > +		mcu_ctrl |= SRBDS_MITG_DIS;
> > +
> > +	if (boot_cpu_has_bug(X86_BUG_SRBDS_TSX) && !boot_cpu_has(X86_FEATURE_RTM))
> > +		mcu_ctrl |= SRBDS_MITG_DIS;
> > +
> > +	wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> > +}
> > +
> > +static void __init srbds_select_mitigation(void)
> > +{
> > +	if (!boot_cpu_has_bug(X86_BUG_SRBDS) &&
> > +	    !boot_cpu_has_bug(X86_BUG_SRBDS_TSX))
> > +		return;
> 
> Why is that check here again if you do it in
> srbds_configure_mitigation() above? I'm guessing you can remove the one
> above...
I wrote it this way initially because at the time I expected there to be more
dynamic support for managing this mitigation WRT post boot changes to TSX
availability.  The expectation I was later given was to make it a boot
commandline control only.  I kept it this way mostly out of laziness and
simplification of the number of mitigation states to track.


> 
> > +
> > +	if (cpu_mitigations_off()) {
> > +		srbds_mitigation = SRBDS_MITIGATION_OFF;
> > +		goto out;
> > +	}
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
> > +		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> > +
> > +out:
> > +	srbds_configure_mitigation();
> > +}
> > +
> > +static int __init srbds_parse_cmdline(char *str)
> > +{
> > +	if (!str)
> > +		return -EINVAL;
> > +
> > +	if (!strcmp(str, "off"))
> > +		srbds_mitigation = SRBDS_MITIGATION_OFF;
> > +
> > +	return 0;
> > +}
> > +
> > +early_param("srbds_mitigation", srbds_parse_cmdline);
> 
> "srb_sampling=" or something more readable pls.
srb_sampling is no more readable to me and doesn't convay what the command line
is for.  But, more readable is always better.

Perhaps changing into a single value, with any '=' like:
disable_srbs_mitigation or srbds_mitigation_off would be better?

Thanks for the feedback,

-mark