Messages in this thread | | | From | Nick Desaulniers <> | Date | Fri, 25 May 2018 09:46:42 -0700 | Subject | Re: [clang] stack protector and f1f029c7bf |
| |
On Fri, May 25, 2018 at 9:33 AM <hpa@zytor.com> wrote: > On May 25, 2018 9:27:40 AM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote: > >On Thu, May 24, 2018 at 3:43 PM <hpa@zytor.com> wrote: > >> On May 24, 2018 3:31:05 PM PDT, Nick Desaulniers > ><ndesaulniers@google.com> > >wrote: > >> >On Thu, May 24, 2018 at 3:05 PM H. Peter Anvin <hpa@zytor.com> > >wrote: > >> >> COMPILER AR: "=rm" should NEVER generate worse code than "=r". > >That > >> >is > >> >> unequivocally a compiler bug. > >> > > >> >Filed: https://bugs.llvm.org/show_bug.cgi?id=37583 > >> > > >> >> >> You are claiming it doesn't buy us anything, but you are only > >> >looking > >> >at > >> >> > the paravirt case which is kind of "special" (in the short bus > >kind > >> >of > >> >way), > >> >> > > >> >> > That's fair. Is another possible solution to have paravirt > >maybe > >> >not > >> >use > >> >> > native_save_fl() then, but its own > >> >non-static-inline-without-m-constraint > >> >> > implementation? > >> > > >> >> KERNEL AR: change native_save_fl() to an extern inline with an > >> >assembly > >> >> out-of-line implementation, to satisfy the paravirt requirement > >that > >> >no > >> >> GPRs other than %rax are clobbered. > >> > > >> >i'm happy to add that, do you have a recommendation if it should go > >in > >> >an > >> >existing .S file or a new one (and if so where/what shall I call > >it?). > > > >> How about irqflags.c since that is what the .h file is called. > > > >> It should simply be: > > > >> push %rdi > >> popf > >> ret > > > >> pushf > >> pop %rax > >> ret > > > >> ... but with all the regular assembly decorations, of course. > > > >Something like the following? > > > > > >diff --git a/arch/x86/kernel/irqflags.c b/arch/x86/kernel/irqflags.c > >new file mode 100644 > >index 000000000000..59dc21bd3327 > >--- /dev/null > >+++ b/arch/x86/kernel/irqflags.c > >@@ -0,0 +1,24 @@ > >+#include <asm/asm.h> > >+ > >+extern unsigned long native_save_fl(void); > >+extern void native_restore_fl(unsigned long flags); > >+ > >+asm( > >+".pushsection .text;" > >+".global native_save_fl;" > >+".type native_save_fl, @function;" > >+"native_save_fl:" > >+"pushf;" > >+"pop %" _ASM_AX ";" > >+"ret;" > >+".popsection"); > >+ > >+asm( > >+".pushsection .text;" > >+".global native_restore_fl;" > >+".type native_restore_fl, @function;" > >+"native_restore_fl:" > >+"push %" _ASM_DI ";" > >+"popf;" > >+"ret;" > >+".popsection"); > > > >And change the declaration in arch/x86/include/asm/irqflags.h to: > >+extern inline unsigned long native_save_fl(void); > >+extern inline void native_restore_fl(unsigned long flags); > > > >This seems to work, but > >1. arch/x86/boot/compressed/misc.o warns that native_save_fl() is never > >defined (arch_local_save_flags() uses it). Does that mean > >arch_local_save_flags(), and friends would also have to move to the > >newly > >created .c file as well? > >2. `extern inline` doesn't inline any instances (from what I can tell > >from > >disassembling vmlinux). I think this is strictly worse. Don't we only > >want > >pv_irq_ops.save_fl to be non-inlined in a way that no stack protector > >can > >be added? If that's the case, should my assembly based implementation > >have > >a different identifier (`native_save_fl_paravirt` or something). That > >would > >also fix point #1 above. But now the paravirt code has its own copy of > >the > >function.
> Sorry, I meant irqflags.S.
> It still should be available as as inline, however, but now "extern inline".
Heh, ok I was confused. But in testing, I had also created:
arch/x86/lib/irqflags.S /* SPDX-License-Identifier: GPL-2.0 */
#include <asm/asm.h> #include <asm/export.h> #include <linux/linkage.h>
/* * unsigned long native_save_fl(void) */ ENTRY(native_save_fl) pushf pop %_ASM_AX ret ENDPROC(native_save_fl) EXPORT_SYMBOL(native_save_fl)
/* * void native_restore_fl(unsigned long flags) * %rdi: flags */ ENTRY(native_restore_fl) push %_ASM_DI popf ret ENDPROC(native_restore_fl) EXPORT_SYMBOL(native_restore_fl)
The issue is that this still has issues 1 & 2 listed earlier (and the disassembly has a lot more trailing nops added).
When you say
> It still should be available as as inline, however, but now "extern inline".
Am I understanding correctly that native_save_fl should be inlined into all call sites (modulo the problematic pv_irq_ops.save_fl case)? Because for these two assembly implementations, it's not, but maybe there's something missing in my implementation? -- Thanks, ~Nick Desaulniers
| |