LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Nadav Amit <namit@vmware.com>
Cc: LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Jan Beulich <JBeulich@suse.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH v2 3/9] x86: refcount: prevent gcc distortions
Date: Mon, 4 Jun 2018 15:06:15 -0700	[thread overview]
Message-ID: <CAGXu5jJFUMsQFhxeZivbQZwQRPwdJr6gD8EkKfzasWr2SBoggQ@mail.gmail.com> (raw)
In-Reply-To: <20180604112131.59100-4-namit@vmware.com>

On Mon, Jun 4, 2018 at 4:21 AM, Nadav Amit <namit@vmware.com> wrote:
> GCC considers the number of statements in inlined assembly blocks,
> according to new-lines and semicolons, as an indication to the cost of
> the block in time and space. This data is distorted by the kernel code,
> which puts information in alternative sections. As a result, the
> compiler may perform incorrect inlining and branch optimizations.
>
> The solution is to set an assembly macro and call it from the inlined
> assembly block. As a result GCC considers the inline assembly block as
> a single instruction.
>
> This patch allows to inline functions such as __get_seccomp_filter().
> Interestingly, this allows more aggressive inlining while reducing the
> kernel size.
>
>    text    data     bss     dec     hex filename
> 18140970 10225412 2957312 31323694 1ddf62e ./vmlinux before
> 18140140 10225284 2957312 31322736 1ddf270 ./vmlinux after (-958)
>
> Static text symbols:
> Before: 40302
> After:  40286   (-16)
>
> Functions such as kref_get(), free_user(), fuse_file_get() now get
> inlined.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arch/x86/include/asm/refcount.h | 73 ++++++++++++++++++++-------------
>  arch/x86/kernel/macros.S        |  1 +
>  2 files changed, 45 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> index 4cf11d88d3b3..53462f32b58e 100644
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -4,6 +4,9 @@
>   * x86-specific implementation of refcount_t. Based on PAX_REFCOUNT from
>   * PaX/grsecurity.
>   */
> +
> +#ifndef __ASSEMBLY__

Can you swap the order here, so that the asm macros are visible first
in the file?

#ifdef __ASSEMBLY__
...macros
#else
....C
#endif

-Kees

> +
>  #include <linux/refcount.h>
>
>  /*
> @@ -14,34 +17,11 @@
>   * central refcount exception. The fixup address for the exception points
>   * back to the regular execution flow in .text.
>   */
> -#define _REFCOUNT_EXCEPTION                            \
> -       ".pushsection .text..refcount\n"                \
> -       "111:\tlea %[counter], %%" _ASM_CX "\n"         \
> -       "112:\t" ASM_UD2 "\n"                           \
> -       ASM_UNREACHABLE                                 \
> -       ".popsection\n"                                 \
> -       "113:\n"                                        \
> -       _ASM_EXTABLE_REFCOUNT(112b, 113b)
> -
> -/* Trigger refcount exception if refcount result is negative. */
> -#define REFCOUNT_CHECK_LT_ZERO                         \
> -       "js 111f\n\t"                                   \
> -       _REFCOUNT_EXCEPTION
> -
> -/* Trigger refcount exception if refcount result is zero or negative. */
> -#define REFCOUNT_CHECK_LE_ZERO                         \
> -       "jz 111f\n\t"                                   \
> -       REFCOUNT_CHECK_LT_ZERO
> -
> -/* Trigger refcount exception unconditionally. */
> -#define REFCOUNT_ERROR                                 \
> -       "jmp 111f\n\t"                                  \
> -       _REFCOUNT_EXCEPTION
>
>  static __always_inline void refcount_add(unsigned int i, refcount_t *r)
>  {
>         asm volatile(LOCK_PREFIX "addl %1,%0\n\t"
> -               REFCOUNT_CHECK_LT_ZERO
> +               "REFCOUNT_CHECK_LT_ZERO counter=\"%[counter]\""
>                 : [counter] "+m" (r->refs.counter)
>                 : "ir" (i)
>                 : "cc", "cx");
> @@ -50,7 +30,7 @@ static __always_inline void refcount_add(unsigned int i, refcount_t *r)
>  static __always_inline void refcount_inc(refcount_t *r)
>  {
>         asm volatile(LOCK_PREFIX "incl %0\n\t"
> -               REFCOUNT_CHECK_LT_ZERO
> +               "REFCOUNT_CHECK_LT_ZERO counter=\"%[counter]\""
>                 : [counter] "+m" (r->refs.counter)
>                 : : "cc", "cx");
>  }
> @@ -58,7 +38,7 @@ static __always_inline void refcount_inc(refcount_t *r)
>  static __always_inline void refcount_dec(refcount_t *r)
>  {
>         asm volatile(LOCK_PREFIX "decl %0\n\t"
> -               REFCOUNT_CHECK_LE_ZERO
> +               "REFCOUNT_CHECK_LE_ZERO counter=\"%[counter]\""
>                 : [counter] "+m" (r->refs.counter)
>                 : : "cc", "cx");
>  }
> @@ -66,13 +46,15 @@ static __always_inline void refcount_dec(refcount_t *r)
>  static __always_inline __must_check
>  bool refcount_sub_and_test(unsigned int i, refcount_t *r)
>  {
> -       GEN_BINARY_SUFFIXED_RMWcc(LOCK_PREFIX "subl", REFCOUNT_CHECK_LT_ZERO,
> +       GEN_BINARY_SUFFIXED_RMWcc(LOCK_PREFIX "subl",
> +                                 "REFCOUNT_CHECK_LT_ZERO counter=\"%0\"",
>                                   r->refs.counter, "er", i, "%0", e, "cx");
>  }
>
>  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
>  {
> -       GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl", REFCOUNT_CHECK_LT_ZERO,
> +       GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> +                                "REFCOUNT_CHECK_LT_ZERO counter=\"%0\"",
>                                  r->refs.counter, "%0", e, "cx");
>  }
>
> @@ -90,7 +72,7 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
>
>                 /* Did we try to increment from/to an undesirable state? */
>                 if (unlikely(c < 0 || c == INT_MAX || result < c)) {
> -                       asm volatile(REFCOUNT_ERROR
> +                       asm volatile("REFCOUNT_ERROR counter=\"%[counter]\""
>                                      : : [counter] "m" (r->refs.counter)
>                                      : "cc", "cx");
>                         break;
> @@ -106,4 +88,37 @@ static __always_inline __must_check bool refcount_inc_not_zero(refcount_t *r)
>         return refcount_add_not_zero(1, r);
>  }
>
> +#else /* __ASSEMBLY__ */
> +#include <asm/asm.h>
> +#include <asm/bug.h>
> +
> +.macro REFCOUNT_EXCEPTION counter:req
> +       .pushsection .text..refcount
> +111:   lea \counter, %_ASM_CX
> +112:   ud2
> +       ASM_UNREACHABLE
> +       .popsection
> +113:   _ASM_EXTABLE_REFCOUNT(112b, 113b)
> +.endm
> +
> +/* Trigger refcount exception if refcount result is negative. */
> +.macro REFCOUNT_CHECK_LT_ZERO counter:req
> +       js 111f
> +       REFCOUNT_EXCEPTION \counter
> +.endm
> +
> +/* Trigger refcount exception if refcount result is zero or negative. */
> +.macro REFCOUNT_CHECK_LE_ZERO counter:req
> +       jz 111f
> +       REFCOUNT_CHECK_LT_ZERO counter=\counter
> +.endm
> +
> +/* Trigger refcount exception unconditionally. */
> +.macro REFCOUNT_ERROR counter:req
> +       jmp 111f
> +       REFCOUNT_EXCEPTION counter=\counter
> +.endm
> +
> +#endif /* __ASSEMBLY__ */
> +
>  #endif
> diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S
> index cee28c3246dc..f1fe1d570365 100644
> --- a/arch/x86/kernel/macros.S
> +++ b/arch/x86/kernel/macros.S
> @@ -7,3 +7,4 @@
>   */
>
>  #include <linux/compiler.h>
> +#include <asm/refcount.h>
> --
> 2.17.0
>



-- 
Kees Cook
Pixel Security

  reply	other threads:[~2018-06-04 22:06 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-04 11:21 [PATCH v2 0/9] x86: macrofying inline asm for better compilation Nadav Amit
2018-06-04 11:21 ` [PATCH v2 1/9] Makefile: Prepare for using macros for inline asm Nadav Amit
2018-06-04 11:21 ` [PATCH v2 2/9] x86: objtool: use asm macro for better compiler decisions Nadav Amit
2018-06-04 19:04   ` Josh Poimboeuf
2018-06-05  5:41   ` kbuild test robot
2018-06-04 11:21 ` [PATCH v2 3/9] x86: refcount: prevent gcc distortions Nadav Amit
2018-06-04 22:06   ` Kees Cook [this message]
2018-06-04 22:20     ` Nadav Amit
2018-06-05  8:26   ` kbuild test robot
2018-06-04 11:21 ` [PATCH v2 4/9] x86: alternatives: macrofy locks for better inlining Nadav Amit
2018-06-05  5:36   ` kbuild test robot
2018-06-05 14:07   ` kbuild test robot
2018-06-07  3:05   ` [lkp-robot] [x86] 1a39381d70: WARNING:at_kernel/locking/mutex.c:#__mutex_unlock_slowpath kernel test robot
2018-06-04 11:21 ` [PATCH v2 5/9] x86: bug: prevent gcc distortions Nadav Amit
2018-06-05  7:34   ` kbuild test robot
2018-06-04 11:21 ` [PATCH v2 6/9] x86: prevent inline distortion by paravirt ops Nadav Amit
2018-06-04 11:21 ` [PATCH v2 7/9] x86: extable: use macros instead of inline assembly Nadav Amit
2018-06-04 11:21 ` [PATCH v2 8/9] x86: cpufeature: " Nadav Amit
2018-06-04 11:21 ` [PATCH v2 9/9] x86: jump-labels: " Nadav Amit
2018-06-04 19:05 ` [PATCH v2 0/9] x86: macrofying inline asm for better compilation Josh Poimboeuf
2018-06-04 19:56   ` Nadav Amit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXu5jJFUMsQFhxeZivbQZwQRPwdJr6gD8EkKfzasWr2SBoggQ@mail.gmail.com \
    --to=keescook@chromium.org \
    --cc=JBeulich@suse.com \
    --cc=hpa@zytor.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=namit@vmware.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v2 3/9] x86: refcount: prevent gcc distortions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).