Re: [PATCH] x86/retpoline: Also fill return buffer after idle

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Tue, 2018-01-09 at 10:37 +0100, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 03:51:26PM -0800, Andi Kleen wrote:
> 
> > 
> > @@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
> >  		}
> >  
> >  		__monitor((void *)¤t_thread_info()->flags, 0, 0);
> > -		if (!need_resched())
> > +		if (!need_resched()) {
> >  			__mwait(eax, ecx);
> > +			/*
> > +			 * idle could have cleared the return buffer,
> > +			 * so fill it to prevent uncontrolled
> > +			 * speculation.
> > +			 */
> > +			fill_return_buffer();
> wouldn't something like:
> 
> 			if (static_cpu_has(X86_FEATURE_RETPOLINE))
> 				fill_return_buffer();
> 
> be much saner? Then we avoid the entire call when not needed and you
> don't have to muck with the asm either.

Hm...

The background, of course, that that we need to be careful when doing
things like this. If you end up with a conditional branch there, then
processor can speculate right past it. There's a reason a lot of the
IBRS-setting code has, effectively, an 'else lfence' in the cases where
it isn't being done with ALTERNATIVEs.

We had a *beautiful* case of that in the early IBRS patch set, on the
syscall path, where the conditional branch opened up a path for
speculative execution all the way to the jmp *sys_call_table(…).

Now, as discussed on IRC, we can see that the current implementation of
static_cpu_has using asm goto *is* generally doing the right thing and
turning it into a straight unconditional jump over the
fill_return_buffer() code. Clever GCC, have biscuit.

However, you are suggesting that we turn the static_cpu_has() trick
from a "nice to have" optimisation which is all very well when it pans
out, to something we *rely* on for secure operation of the system.

It never ends well when we rely on all versions of GCC optimising
things precisely how we want.

If you can build in a sanity check to ensure that the build will *fail*
when GCC doesn't do what we want, I suppose we could live with that.
But we don't have such a sanity check at the moment, do we?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]