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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>  There's no "optimiser decides
> to be clever" in this case imo - it all is a result of not inlining, when the
> construct in evaluate_nospec() is specifically assuming this wouldn't happen.
> Therefore I continue to think that the description is misleading.
>
>> Any use of evaluate_nospec() needs all static inline predicates which use it
>> to be declared always_inline to prevent the optimiser having the flexibility
>> to generate unsafe code.
> I agree with this part.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>>
>> 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.

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.

~Andrew

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