Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec
Date: Fri, 25 Oct 2019 14:34:00 +0200	[thread overview]
Message-ID: <c12afdd1-c561-bc79-5c36-22ac2e994953@suse.com> (raw)
In-Reply-To: <782089aa-7994-f471-3c52-2afb30f95812@citrix.com>

On 25.10.2019 14:10, Andrew Cooper wrote:
> On 25/10/2019 13:03, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>>
>>> To correctly protect jumps, the generated code needs to be of the form:
>>>
>>>     cmp/test <cond>
>>>     jcc 1f
>>>     lfence
>>>     ...
>>>  1: lfence
>>>     ...
>>>
>>> Critically, the lfence must be at the head of both basic blocks, later in the
>>> instruction stream than the conditional jump in need of protection.
>>>
>>> When a static inline is involved, the optimiser decides to be clever and
>>> rearranges the code as:
>>>
>>>  pred:
>>>     lfence
>>>     <calculate cond>
>>>     ret
>>>
>>>     call pred
>>>     cmp $0, %eax
>>>     jcc 1f
>>>     ...
>>>  1: ...
>>>
>>> which breaks the speculative safety.
>> Aiui "pred" is a non-inlined static inline here.
> 
> Correct, although it actually applies to anything which the compiler
> chose to out of line, perhaps even as a side effect of CSE pass.

Not sure if you're alluding to such, but I've never seen the compiler
out-of-line something that wasn't a function (or perhaps a specialization
of one) at the source level.

>>> This is the transitive set of predicates which I can spot which need
>>> protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
>>> approach, but the only other option for 4.13 is to revert it all to unbreak
>>> livepatching.
>> To unbreak livepatching, aiui what you need is symbol disambiguation,
>> a patch for which has been sent.
> 
> Correct, but..
> 
>> With this I think we should focus on
>> code generation aspects here. I'm fine ack-ing the code changes with
>> a modified description. But since you're -1 for this, I'm not sure in
>> the first place that we want to go this route.
> 
> ... without this change, l1tf-barrier/branch-hardening is still broken,
> and a performance overhead.

Well, it has less of an effect, but it's still better than without any
of this altogether. In some cases code generation is correct, and in
some other cases code generation is at least such that the window size
gets shrunk.

> The two choices to unblock 4.13 are this patch, or the previous version
> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
> disliked.

Option 3 is to have just the config option, for people to turn this
off if they feel like doing so.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel