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.coop...@citrix.com> >> --- >> CC: George Dunlap <george.dun...@eu.citrix.com> >> CC: Ian Jackson <ian.jack...@citrix.com> >> CC: Jan Beulich <jbeul...@suse.com> >> CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> >> CC: Stefano Stabellini <sstabell...@kernel.org> >> CC: Wei Liu <w...@xen.org> >> CC: Julien Grall <jul...@xen.org> >> CC: Juergen Gross <jgr...@suse.com> >> >> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
