Re: [clang] stack protector and f1f029c7bf

[Nick Desaulniers]

On Thu, May 24, 2018 at 3:43 PM <hpa@xxxxxxxxx> wrote:
> On May 24, 2018 3:31:05 PM PDT, Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
wrote:
> >On Thu, May 24, 2018 at 3:05 PM H. Peter Anvin <hpa@xxxxxxxxx> 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