Messages in this thread | | | Subject | Re: [PATCH] x86/retpoline: Avoid return buffer underflows on context switch | Date | Tue, 9 Jan 2018 00:44:33 +0000 | From | "Woodhouse, David" <> |
| |
On Mon, 2018-01-08 at 16:15 -0800, Paul Turner wrote: > On Mon, Jan 8, 2018 at 2:11 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Jan 08, 2018 at 12:15:31PM -0800, Andi Kleen wrote: > >> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h > >> index b8c8eeacb4be..e84e231248c2 100644 > >> --- a/arch/x86/include/asm/nospec-branch.h > >> +++ b/arch/x86/include/asm/nospec-branch.h > >> @@ -53,6 +53,35 @@ > >> #endif > >> .endm > >> > >> +/* > >> + * We use 32-N: 32 is the max return buffer size, > >> + * but there should have been at a minimum two > >> + * controlled calls already: one into the kernel > >> + * from entry*.S and another into the function > >> + * containing this macro. So N=2, thus 30. > >> + */ > >> +#define NUM_BRANCHES_TO_FILL 30 > >> + > >> +/* > >> + * Fill the CPU return branch buffer to prevent > >> + * indirect branch prediction on underflow. > >> + * Caller should check for X86_FEATURE_SMEP and X86_FEATURE_RETPOLINE > >> + */ > >> +.macro FILL_RETURN_BUFFER > >> +#ifdef CONFIG_RETPOLINE > >> + .rept NUM_BRANCHES_TO_FILL > >> + call 1221f > >> + pause /* stop speculation */ > >> +1221: > >> + .endr > >> +#ifdef CONFIG_64BIT > >> + addq $8*NUM_BRANCHES_TO_FILL, %rsp > >> +#else > >> + addl $4*NUM_BRANCHES_TO_FILL, %esp > >> +#endif > >> +#endif > >> +.endm > > > > So pjt did alignment, a single unroll and per discussion earlier today > > (CET) or late last night (PST), he only does 16. > > > > Why is none of that done here? Also, can we pretty please stop using > > those retarded number labels, they make this stuff unreadable. > > > > Also, pause is unlikely to stop speculation, that comment doesn't make > > sense. Looking at PJT's version there used to be a speculation trap in > > there, but I can't see that here. > > > > You definitely want the speculation traps.. these entries are > potentially consumed. > Worse: The first entry that will be consumed is the last call in your > linear chain, meaning that it immediately gets to escape into > alternative execution. > (When I was experimenting with icache-minimizing constructions here I > actually used intentional backwards jumps in linear chains to avoid > this.) > > The sequence I reported is what ended up seeming optimal.
On IRC, Arjan assures me that 'pause' here really is sufficient as a speculation trap. If we do end up returning back here as a misprediction, that 'pause' will stop the speculative execution on affected CPUs even though it isn't *architecturally* documented to do so.
Arjan, can you confirm that in email please?[unhandled content-type:application/x-pkcs7-signature] | |