Re: [clang] stack protector and f1f029c7bf

[Nick Desaulniers]

On Fri, May 25, 2018 at 9:33 AM <hpa@xxxxxxxxx> wrote:
> On May 25, 2018 9:27:40 AM PDT, Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
wrote:
> >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.

> 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