On Wed, Apr 29, 2020 at 6:18 AM Peter Zijlstra <[email protected]> wrote: > > On Wed, Apr 29, 2020 at 10:30:53AM +0200, Peter Zijlstra wrote: > > > POPF is an expensive instruction that should be avoided if possible. > > > A better solution would be to have the alternative jump over the > > > push/pop when SMAP is disabled. > > > > Yeah. I think I had that, but then confused myself again. I don't think > > it matters much if you look at where it's used though. > > > > Still, let me try the jmp thing again.. > > Here goes.. > > --- > Subject: x86,smap: Fix smap_{save,restore}() alternatives > From: Peter Zijlstra <[email protected]> > Date: Tue Apr 28 19:57:59 CEST 2020 > > As reported by objtool: > > lib/ubsan.o: warning: objtool: .altinstr_replacement+0x0: alternative > modifies stack > lib/ubsan.o: warning: objtool: .altinstr_replacement+0x7: alternative > modifies stack > > the smap_{save,restore}() alternatives violate (the newly enforced) > rule on stack invariance. That is, due to there only being a single > ORC table it must be valid to any alternative. These alternatives > violate this with the direct result that unwinds will not be correct > when it hits between the PUSH and POP instructions. > > Rewrite the functions to only have a conditional jump. > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]> > --- > arch/x86/include/asm/smap.h | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > --- a/arch/x86/include/asm/smap.h > +++ b/arch/x86/include/asm/smap.h > @@ -57,8 +57,10 @@ static __always_inline unsigned long sma > { > unsigned long flags; > > - asm volatile (ALTERNATIVE("", "pushf; pop %0; " __ASM_CLAC, > - X86_FEATURE_SMAP) > + asm volatile ("# smap_save\n\t" > + ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP) > + "pushf; pop %0; " __ASM_CLAC "\n\t" > + "1:" > : "=rm" (flags) : : "memory", "cc"); > > return flags; > @@ -66,7 +68,10 @@ static __always_inline unsigned long sma > > static __always_inline void smap_restore(unsigned long flags) > { > - asm volatile (ALTERNATIVE("", "push %0; popf", X86_FEATURE_SMAP) > + asm volatile ("# smap_restore\n\t" > + ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP) > + "push %0; popf\n\t" > + "1:" > : : "g" (flags) : "memory", "cc"); > } >
Looks good. Alternatively, you could use static_cpu_has(X86_FEATURE_SMAP). -- Brian Gerst
