Re: [patch 23/24] x86/speculation: Enable PRCTL mode for spectre_v2_app2app

From: Ingo Molnar <mingo@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jiri Kosina <jkosina@suse.cz>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	David Woodhouse <dwmw@amazon.co.uk>,
	Andi Kleen <ak@linux.intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Casey Schaufler <casey.schaufler@intel.com>,
	Asit Mallick <asit.k.mallick@intel.com>,
	Arjan van de Ven <arjan@linux.intel.com>,
	Jon Masters <jcm@redhat.com>, Waiman Long <longman9394@gmail.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Dave Stewart <david.c.stewart@intel.com>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [patch 23/24] x86/speculation: Enable PRCTL mode for spectre_v2_app2app
Date: Thu, 22 Nov 2018 08:17:55 +0100	[thread overview]
Message-ID: <20181122071755.GB41788@gmail.com> (raw)
In-Reply-To: <20181121201724.508280006@linutronix.de>

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Now that all prerequisites are in place:
> 
>  - Add the prctl command line option
> 
>  - Default the 'auto' mode to 'prctl'
> 
>  - When SMT state changes, update the static key which controls the
>    conditional STIBP evaluation on context switch.
> 
>  - At init update the static key which controls the conditional IBPB
>    evaluation on context switch.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |    5 ++
>  arch/x86/kernel/cpu/bugs.c                      |   46 +++++++++++++++++++++---
>  2 files changed, 45 insertions(+), 6 deletions(-)
> 
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4246,7 +4246,10 @@
>  				  by spectre_v2=off
>  			auto    - Kernel selects the mitigation depending on
>  				  the available CPU features and vulnerability.
> -				  Default is off.
> +				  Default is prctl.
> +			prctl   - Indirect branch speculation is enabled, but
> +				  mitigation can be enabled via prctl per thread.
> +				  The mitigation control state is inherited on fork.

Please change the order of the last two entries, i.e. make 'auto' the 
last one. This is the pattern we use in other places plus we refer to 
'prctl' before we document it.

>  static const struct {
> @@ -270,6 +272,7 @@ static const struct {
>  	{ "auto",	SPECTRE_V2_APP2APP_CMD_AUTO,	false },
>  	{ "off",	SPECTRE_V2_APP2APP_CMD_NONE,	false },
>  	{ "on",		SPECTRE_V2_APP2APP_CMD_FORCE,	true  },
> +	{ "prctl",	SPECTRE_V2_APP2APP_CMD_PRCTL,	false },

Might make sense to order them in a consistent fashion as well.

> +	/*
> +	 * If STIBP is not available or SMT is not possible clear the STIPB
> +	 * mode.
> +	 */
> +	if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> +		mode = SPECTRE_V2_APP2APP_NONE;

Another nit: please match order of the comments to how the condition is 
written in the code.

> +/* Update the static key controlling the evaluation of TIF_SPEC_IB */
> +static void update_indir_branch_cond(void)
> +{
> +	if (!IS_ENABLED(CONFIG_SMP))
> +		return;
> +
> +	if (sched_smt_active())
> +		static_branch_enable(&switch_to_cond_stibp);
> +	else
> +		static_branch_disable(&switch_to_cond_stibp);

So in the !SMP case sched_smt_active() is already doing the right thing:

  static inline bool sched_smt_active(void) { return false; }

I.e. couldn't we just remove the extra CONFIG_SMP condition?
This would simplify the code with some very minor expense on !SMP.

Thanks,

	Ingo