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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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. 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.

Jan

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