request (x86 + stack protector + paravirt + clang)

request (x86 + stack protector + paravirt + clang)

[Nick Desaulniers]

Please cherry-pick upstream commits:

d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
declarations"
0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"
d0a8d93 "x86/paravirt: Make native_save_fl() extern inline"

To stable branches 4.4+.  They will allow 4.4+ x86 kernels compiled
with Clang and have the configs CONFIG_STACK_PROTECTOR_STRONG and
CONFIG_PARAVIRT to boot.  They also allow gcc 5.1+ users to have
consistent `extern inline` semantics.

In response to:
https://android-review.googlesource.com/c/kernel/common/+/716477#message-291621a3a8592fc5b4c2b8ae0f73c376ebd1a7b4

One of these days I'll remember to cc stable in the commit message
properly...sorry!
-- 
Thanks,
~Nick Desaulniers

Re: request (x86 + stack protector + paravirt + clang)

[Greg KH]

On Tue, Jul 17, 2018 at 04:48:01PM -0700, Nick Desaulniers wrote:
> Please cherry-pick upstream commits:
> 
> d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> declarations"

This took a bunch of work to get it to work for 4.9, I didn't mess with
it for 4.4.y, can you provide a patch series for that tree?  Hm, I
wonder if the same commits I did for 4.9.y would work there, let me try
that first...

> 0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"

This didn't work for 4.4.y either :(

> d0a8d93 "x86/paravirt: Make native_save_fl() extern inline"

This worked for all.

> To stable branches 4.4+.  They will allow 4.4+ x86 kernels compiled
> with Clang and have the configs CONFIG_STACK_PROTECTOR_STRONG and
> CONFIG_PARAVIRT to boot.  They also allow gcc 5.1+ users to have
> consistent `extern inline` semantics.
> 
> In response to:
> https://android-review.googlesource.com/c/kernel/common/+/716477#message-291621a3a8592fc5b4c2b8ae0f73c376ebd1a7b4
> 
> One of these days I'll remember to cc stable in the commit message
> properly...sorry!

Not a problem.

Also, a minor thing, when doing kernel work you should have:
	[core]
	       abbrev = 12

in your .gitconfig as our sha1 numbers are usually longer than the 7 you
sent above.

Also, here's a handy alias lots of us use:
	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"
to output the sha1 and commit description.

It would do this for one of your commits above:
	d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern inline")

thanks,

greg k-h

Re: request (x86 + stack protector + paravirt + clang)

[Greg KH]

On Wed, Jul 18, 2018 at 10:48:37AM +0200, Greg KH wrote:
> On Tue, Jul 17, 2018 at 04:48:01PM -0700, Nick Desaulniers wrote:
> > Please cherry-pick upstream commits:
> > 
> > d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> > declarations"
> 
> This took a bunch of work to get it to work for 4.9, I didn't mess with
> it for 4.4.y, can you provide a patch series for that tree?  Hm, I
> wonder if the same commits I did for 4.9.y would work there, let me try
> that first...

Ok, nevermind, I got all of the above to work for 4.4.y now as well.
Hopefully it passes my test builds...

> > 0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"
> 
> This didn't work for 4.4.y either :(

I got this to apply as well now, so all should be good.

thanks,

greg k-h

Re: request (x86 + stack protector + paravirt + clang)

[Greg KH]

On Wed, Jul 18, 2018 at 10:48:37AM +0200, Greg KH wrote:
> On Tue, Jul 17, 2018 at 04:48:01PM -0700, Nick Desaulniers wrote:
> > Please cherry-pick upstream commits:
> > 
> > d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> > declarations"
> 
> This took a bunch of work to get it to work for 4.9, I didn't mess with
> it for 4.4.y, can you provide a patch series for that tree?  Hm, I
> wonder if the same commits I did for 4.9.y would work there, let me try
> that first...
> 
> > 0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"
> 
> This didn't work for 4.4.y either :(
> 
> > d0a8d93 "x86/paravirt: Make native_save_fl() extern inline"
> 
> This worked for all.

Oops, nope, it broke the build on 4.4.y, can you provide a working
backport for there?

thanks,

greg k-h

[4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[Nick Desaulniers]

native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.

paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.

Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.

Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.

The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.

Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16

Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371

Thanks to the many folks that participated in the discussion.

Acked-by: Juergen Gross <jgross@suse.com>
Debugged-by: Alistair Strachan <astrachan@google.com>
Debugged-by: Matthias Kaehlcke <mka@chromium.org>
Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Suggested-by: Tom Stellar <tstellar@redhat.com>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
---
Backport for 4.4. 4.4 is missing commit 784d5699eddc "x86: move exports to
actual definitions" which doesn't apply cleanly, and not really worth
backporting IMO. It's simpler to change this patch from upstream:
+ #include <asm-generic/export.h>
rather than
+ #include <asm/export.h>

 arch/x86/include/asm/irqflags.h |  2 +-
 arch/x86/kernel/Makefile        |  1 +
 arch/x86/kernel/irqflags.S      | 26 ++++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/irqflags.S

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index b77f5edb03b0..0056bc945cd1 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -8,7 +8,7 @@
  * Interrupt control:
  */
 
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
 {
 	unsigned long flags;
 
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index b1b78ffe01d0..7947cee61f61 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -41,6 +41,7 @@ obj-y			+= alternative.o i8253.o pci-nommu.o hw_breakpoint.o
 obj-y			+= tsc.o tsc_msr.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
+obj-y			+= irqflags.o
 
 obj-y				+= process.o
 obj-y				+= fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index 000000000000..3817eb748eb4
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/asm.h>
+#include <asm-generic/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)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+	push %_ASM_ARG1
+	popf
+	ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
-- 
2.18.0.233.g985f88cf7e-goog

Re: request (x86 + stack protector + paravirt + clang)

[Nick Desaulniers]

On Wed, Jul 18, 2018 at 1:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> Also, a minor thing, when doing kernel work you should have:
>         [core]
>                abbrev = 12
>
> in your .gitconfig as our sha1 numbers are usually longer than the 7 you
> sent above.
>
> Also, here's a handy alias lots of us use:
>         git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"
> to output the sha1 and commit description.
>
> It would do this for one of your commits above:
>         d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern inline")

Neat, thanks for the tip, I was wondering if folks were always
counting out 12 characters when copying SHAs...

On Wed, Jul 18, 2018 at 2:07 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 18, 2018 at 10:48:37AM +0200, Greg KH wrote:
> > On Tue, Jul 17, 2018 at 04:48:01PM -0700, Nick Desaulniers wrote:
> > > Please cherry-pick upstream commits:
> > >
> > > d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> > > declarations"
> >
> > This took a bunch of work to get it to work for 4.9, I didn't mess with
> > it for 4.4.y, can you provide a patch series for that tree?  Hm, I
> > wonder if the same commits I did for 4.9.y would work there, let me try
> > that first...
> >
> > > 0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"
> >
> > This didn't work for 4.4.y either :(
> >
> > > d0a8d93 "x86/paravirt: Make native_save_fl() extern inline"
> >
> > This worked for all.
>
> Oops, nope, it broke the build on 4.4.y, can you provide a working
> backport for there?

Was there anything special I should have put in the commit message to
show it was changed from upstream?  I sent it in reply to this email
I'm responding to, but gmail doesn't display it as a child.  It can
also be found: https://lkml.org/lkml/2018/7/20/1142 but please let me
know if there's a better way I should send it.

-- 
Thanks,
~Nick Desaulniers

Re: [4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

Hi Nick,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/x86/core]
[cannot apply to v4.18-rc5 next-20180720]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nick-Desaulniers/x86-paravirt-make-native_save_fl-extern-inline/20180721-073112
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/irqflags.S: Assembler messages:
>> arch/x86/kernel/irqflags.S:22: Error: bad register name `%_ASM_ARG1'

vim +22 arch/x86/kernel/irqflags.S

    16	
    17	/*
    18	 * void native_restore_fl(unsigned long flags)
    19	 * %eax/%rdi: flags
    20	 */
    21	ENTRY(native_restore_fl)
  > 22		push %_ASM_ARG1

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6567 bytes --]

Re: [4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[Nick Desaulniers]

On Fri, Jul 20, 2018 at 5:04 PM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Nick,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tip/x86/core]
> [cannot apply to v4.18-rc5 next-20180720]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Nick-Desaulniers/x86-paravirt-make-native_save_fl-extern-inline/20180721-073112
> config: i386-tinyconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>    arch/x86/kernel/irqflags.S: Assembler messages:
> >> arch/x86/kernel/irqflags.S:22: Error: bad register name `%_ASM_ARG1'
>
> vim +22 arch/x86/kernel/irqflags.S
>
>     16
>     17  /*
>     18   * void native_restore_fl(unsigned long flags)
>     19   * %eax/%rdi: flags
>     20   */
>     21  ENTRY(native_restore_fl)
>   > 22          push %_ASM_ARG1
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

oops, probably should not have cc'ed vger.kernel.org for a patch meant
for stable.

-- 
Thanks,
~Nick Desaulniers

Re: request (x86 + stack protector + paravirt + clang)

[Greg KH]

On Fri, Jul 20, 2018 at 03:45:29PM -0700, Nick Desaulniers wrote:
> On Wed, Jul 18, 2018 at 1:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Also, a minor thing, when doing kernel work you should have:
> >         [core]
> >                abbrev = 12
> >
> > in your .gitconfig as our sha1 numbers are usually longer than the 7 you
> > sent above.
> >
> > Also, here's a handy alias lots of us use:
> >         git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"
> > to output the sha1 and commit description.
> >
> > It would do this for one of your commits above:
> >         d0a8d9378d16 ("x86/paravirt: Make native_save_fl() extern inline")
> 
> Neat, thanks for the tip, I was wondering if folks were always
> counting out 12 characters when copying SHAs...
> 
> On Wed, Jul 18, 2018 at 2:07 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 18, 2018 at 10:48:37AM +0200, Greg KH wrote:
> > > On Tue, Jul 17, 2018 at 04:48:01PM -0700, Nick Desaulniers wrote:
> > > > Please cherry-pick upstream commits:
> > > >
> > > > d03db2b "compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> > > > declarations"
> > >
> > > This took a bunch of work to get it to work for 4.9, I didn't mess with
> > > it for 4.4.y, can you provide a patch series for that tree?  Hm, I
> > > wonder if the same commits I did for 4.9.y would work there, let me try
> > > that first...
> > >
> > > > 0e2e160 "x86/asm: Add _ASM_ARG* constants for argument registers to <asm/asm.h>"
> > >
> > > This didn't work for 4.4.y either :(
> > >
> > > > d0a8d93 "x86/paravirt: Make native_save_fl() extern inline"
> > >
> > > This worked for all.
> >
> > Oops, nope, it broke the build on 4.4.y, can you provide a working
> > backport for there?
> 
> Was there anything special I should have put in the commit message to
> show it was changed from upstream?  I sent it in reply to this email
> I'm responding to, but gmail doesn't display it as a child.  It can
> also be found: https://lkml.org/lkml/2018/7/20/1142 but please let me
> know if there's a better way I should send it.

Well, gmail sucks for mailing lists :)

And no, you did it great, but it would be nice to have the git commit id
of the patch itself somewhere in there so I know what it is in Linus's
tree so I don't have to go dig it up manually.  Not a big deal, I can
find it easily this time, but for the next time you send stuff in.

thanks,

greg k-h

Re: [4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[Greg KH]

On Fri, Jul 20, 2018 at 05:06:07PM -0700, Nick Desaulniers wrote:
> On Fri, Jul 20, 2018 at 5:04 PM kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Nick,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on tip/x86/core]
> > [cannot apply to v4.18-rc5 next-20180720]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Nick-Desaulniers/x86-paravirt-make-native_save_fl-extern-inline/20180721-073112
> > config: i386-tinyconfig (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=i386
> >
> > All errors (new ones prefixed by >>):
> >
> >    arch/x86/kernel/irqflags.S: Assembler messages:
> > >> arch/x86/kernel/irqflags.S:22: Error: bad register name `%_ASM_ARG1'
> >
> > vim +22 arch/x86/kernel/irqflags.S
> >
> >     16
> >     17  /*
> >     18   * void native_restore_fl(unsigned long flags)
> >     19   * %eax/%rdi: flags
> >     20   */
> >     21  ENTRY(native_restore_fl)
> >   > 22          push %_ASM_ARG1
> >
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
> oops, probably should not have cc'ed vger.kernel.org for a patch meant
> for stable.

stable@vger.kernel.org is the right address, you did it correct.  I
think 0-day just got a bit trigger-happy on this one for some reason,
don't worry about it.

thanks,

greg k-h

Re: [4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[Greg KH]

On Fri, Jul 20, 2018 at 03:36:41PM -0700, Nick Desaulniers wrote:
> native_save_fl() is marked static inline, but by using it as
> a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
> 
> paravirt's use of native_save_fl() also requires that no GPRs other than
> %rax are clobbered.
> 
> Compilers have different heuristics which they use to emit stack guard
> code, the emittance of which can break paravirt's callee saved assumption
> by clobbering %rcx.
> 
> Marking a function definition extern inline means that if this version
> cannot be inlined, then the out-of-line version will be preferred. By
> having the out-of-line version be implemented in assembly, it cannot be
> instrumented with a stack protector, which might violate custom calling
> conventions that code like paravirt rely on.
> 
> The semantics of extern inline has changed since gnu89. This means that
> folks using GCC versions >= 5.1 may see symbol redefinition errors at
> link time for subdirs that override KBUILD_CFLAGS (making the C standard
> used implicit) regardless of this patch. This has been cleaned up
> earlier in the patch set, but is left as a note in the commit message
> for future travelers.
> 
> Reports:
> https://lkml.org/lkml/2018/5/7/534
> https://github.com/ClangBuiltLinux/linux/issues/16
> 
> Discussion:
> https://bugs.llvm.org/show_bug.cgi?id=37512
> https://lkml.org/lkml/2018/5/24/1371
> 
> Thanks to the many folks that participated in the discussion.
> 
> Acked-by: Juergen Gross <jgross@suse.com>
> Debugged-by: Alistair Strachan <astrachan@google.com>
> Debugged-by: Matthias Kaehlcke <mka@chromium.org>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Suggested-by: Tom Stellar <tstellar@redhat.com>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
> Backport for 4.4. 4.4 is missing commit 784d5699eddc "x86: move exports to
> actual definitions" which doesn't apply cleanly, and not really worth
> backporting IMO. It's simpler to change this patch from upstream:
> + #include <asm-generic/export.h>
> rather than
> + #include <asm/export.h>

Yeah, that makes sense, thanks for the backport.  I'll queue it up after
the next round of stable kernels comes out in a few days.

greg k-h

Re: [4.4 STABLE BACKPORT] x86: paravirt: make native_save_fl extern inline

[Greg KH]

On Fri, Jul 20, 2018 at 03:36:41PM -0700, Nick Desaulniers wrote:
> native_save_fl() is marked static inline, but by using it as
> a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
> 
> paravirt's use of native_save_fl() also requires that no GPRs other than
> %rax are clobbered.
> 
> Compilers have different heuristics which they use to emit stack guard
> code, the emittance of which can break paravirt's callee saved assumption
> by clobbering %rcx.
> 
> Marking a function definition extern inline means that if this version
> cannot be inlined, then the out-of-line version will be preferred. By
> having the out-of-line version be implemented in assembly, it cannot be
> instrumented with a stack protector, which might violate custom calling
> conventions that code like paravirt rely on.
> 
> The semantics of extern inline has changed since gnu89. This means that
> folks using GCC versions >= 5.1 may see symbol redefinition errors at
> link time for subdirs that override KBUILD_CFLAGS (making the C standard
> used implicit) regardless of this patch. This has been cleaned up
> earlier in the patch set, but is left as a note in the commit message
> for future travelers.
> 
> Reports:
> https://lkml.org/lkml/2018/5/7/534
> https://github.com/ClangBuiltLinux/linux/issues/16
> 
> Discussion:
> https://bugs.llvm.org/show_bug.cgi?id=37512
> https://lkml.org/lkml/2018/5/24/1371
> 
> Thanks to the many folks that participated in the discussion.
> 
> Acked-by: Juergen Gross <jgross@suse.com>
> Debugged-by: Alistair Strachan <astrachan@google.com>
> Debugged-by: Matthias Kaehlcke <mka@chromium.org>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Suggested-by: Tom Stellar <tstellar@redhat.com>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
> Backport for 4.4. 4.4 is missing commit 784d5699eddc "x86: move exports to
> actual definitions" which doesn't apply cleanly, and not really worth
> backporting IMO. It's simpler to change this patch from upstream:
> + #include <asm-generic/export.h>
> rather than
> + #include <asm/export.h>

Now applied, thanks.

greg k-h