Messages in this thread Patch in this message | | | From | Nick Desaulniers <> | Date | Fri, 25 May 2018 09:27:40 -0700 | Subject | Re: [clang] stack protector and f1f029c7bf |
| |
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. -- Thanks, ~Nick Desaulniers
| |