Re: [Patch v4 17/18] x86/speculation: Update SPEC_CTRL MSRs of remote CPUs

From: Thomas Gleixner <tglx@linutronix.de>
To: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	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>,
	LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Kees Cook <keescook@chromium.org>
Subject: Re: [Patch v4 17/18] x86/speculation: Update SPEC_CTRL MSRs of remote CPUs
Date: Tue, 6 Nov 2018 08:46:35 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1811060139090.1659@nanos.tec.linutronix.de> (raw)
In-Reply-To: <81398b26-e1c3-aac3-b44a-2a0982ae74e0@linux.intel.com>

Tim,

On Mon, 5 Nov 2018, Tim Chen wrote:
> > Aside of the condition being pointless in that case, that issues an IPI
> > whether the task is running or not. So this allows a task to issue tons of
> > async IPIs disturbing others by toggling the control.
> 
> I'm not crazy about sending IPIs too.  Hence the original implementation
> using TIF_UPDATE_SPEC_CTRL flag.

which has it's own problems of bloating the context switch code.

> I've thought a bit about the options you suggested and was unclear
> on a few things.
> 
> > 1) Restrict the PRCTL control so it is only possible to modify it at the
> >    point where the application is still single threaded.
> 
> My understanding is PRCTL applied on the task only. Should it be extended
> to other task threads?  In that case, it seems like we didn't do that for SSBD?

Right, we did not.

This was discussed back then already and we agreed on avoiding the
complexity for this and only allow task scope. Though the child tasks will
inherit the TIF flag.

> > 2) Add _TIF_UPDATE_SPEC_CTRL to the SYSCALL_EXIT_WORK_FLAGS and handle it
> >    in the slow work path.
> 
> There can be tasks that don't do any syscalls, and it seems like we can
> have MSRs getting out of sync?

Setting the TIF flag directly in a remote task is wrong. It needs to be
handled when the _TIF_UPDATE_SPEC_CTRL is evaluated, i.e. the information
needs to be stored process wide e.g. in task->mm.

But yes, if the remote task runs in user space forever, it won't
help. Though the point is that dumpable is usually set when the process
starts, so it's probably mostly a theoretical issue.

> > I'm less and less convinced that piggybacking this on dumpable is a good
> > idea. 
> 
> There are daemons like sshd that are non-dumpable.  So I think protecting
> them is desired.  Otherwise all those daemons will need to be updated to
> use PRCTL.  In the original implementation, IBPB is issued for
> non-dumpable task.  It will be nice to retain that.

A lot of things are nice to have, but you really have to carefully weigh
the tradeoff between the conveniance and the complexity plus the fast path
bloat it creates.

IBPB is a different thing as it writes into a different MSR and there is no
requirement to have the MSR access coordinated with the SSB handling in
switch_to(). 

As dumpable is evaluated in switch_mm() already it might be worthwhile to
evaluate whether the STIBP propagation can be tied to that. That does not
solve the problem of tasks running in user space forever, but we don't care
about that for IBPB either and as I said above it's mostly a theoretical
problem.

Thanks,

	tglx