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

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:

"Add ... mitigation" or so.

See

git log -p arch/x86/kernel/cpu/bugs.c

output for inspiration.

> 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?

> 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...?

> This patch:

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> @@ -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?

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...

> +
> +	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.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
--