Messages in this thread | | | Subject | Re: [PATCH V2 9/9] x86/speculation: Remove all ANNOTATE_NOSPEC_ALTERNATIVE directives | From | Alexandre Chartre <> | Date | Tue, 7 Apr 2020 19:01:43 +0200 |
| |
On 4/7/20 6:28 PM, Josh Poimboeuf wrote: > On Tue, Apr 07, 2020 at 06:18:51PM +0200, Alexandre Chartre wrote: >> >> On 4/7/20 4:32 PM, Alexandre Chartre wrote: >>> >>> On 4/7/20 3:34 PM, Josh Poimboeuf wrote: >>>> On Tue, Apr 07, 2020 at 03:28:37PM +0200, Peter Zijlstra wrote: >>>>> Josh, we should probably have objtool verify it doesn't emit ORC entries >>>>> in alternative ranges. >>>> >>>> Agreed, it might be as simple as checking for insn->alt_group in the >>>> INSN_STACK check or in update_insn_state(). >>>> >>> >>> We could do that only for the "objtool orc generate" command. That way >>> "objtool check" would still check the alternative, but "objtool orc generate" >>> will just use the first half of the alternative (like it does today with >>> ANNOTATE_NOSPEC_ALTERNATIVE). We can even keep all ANNOTATE_NOSPEC_ALTERNATIVE >>> but only use them for "objtool orc generate". >>> >> >> I have checked and objtool doesn't emit ORC entries for alternative: >> decode_instructions() doesn't mark such section with sec->text = true >> so create_orc_sections() doesn't emit corresponding ORC entries. >> >> So I think we can remove the ANNOTATE_NOSPEC_ALTERNATIVE directives, >> this will allow objtool to check the instructions but it still won't >> emit ORC entries (same behavior as today). In the future, if ORC >> eventually supports alternative we will be ready to have objtool emit >> ORC entries. > > What's the benefit of removing ANNOTATE_NOSPEC_ALTERNATIVE if there's no > ORC support to go along with it?
To have the code validated by objtool like any other alternative code (which is not tagged with ANNOTATE_NOSPEC_ALTERNATIVE).
> Also I want to avoid adding "ORC alternatives". ORC is nice and simple > and we should keep it that way as much as possible. > > Again, we should warn on stack changes inside alternatives, and then > look at converting RSB and retpolines to use static branches so they > have deterministic stacks. > objtool doesn't currently warn on stack changes inside alternatives. The RSB/retpoline alternatives have warning because objtool doesn't support retpoline ret and intra-function calls. If you have an alternative doing stack changes that objtool understand (like push/pop, add/remove to sp) then you won't have a warning.
I think that's the case with smap_save:
static __always_inline unsigned long smap_save(void) { unsigned long flags;
asm volatile (ALTERNATIVE("", "pushf; pop %0; " __ASM_CLAC, X86_FEATURE_SMAP) : "=rm" (flags) : : "memory", "cc");
return flags; }
The alternative does change the stack but objtool won't complain because it handles the pushf and pop instruction.
alex.
| |