[PATCH] x86/asm: improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[PATCH] x86/asm: improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[Jan Beulich]

Commit df3405245a ("x86/asm: Add suffix macro for GEN_*_RMWcc()")
introduced "suffix" RMWcc operations, adding bogus clobber specifiers:
For one, on x86 there's no point explicitly clobbering "cc". In fact,
with gcc properly fixed, this results in an overlap being detected by
the compiler between outputs and clobbers. Further more it seems bad
practice to me to have clobber specification and use of the clobbered
register(s) disconnected - it should rather be at the invocation place
of that GEN_{UN,BIN}ARY_SUFFIXED_RMWcc() macros that the clobber is
specified which this particular invocation needs.

Drop the "cc" clobber altogether and move the "cx" one to refcount.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/refcount.h |    4 ++--
 arch/x86/include/asm/rmwcc.h    |   16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

--- 4.16-rc2/arch/x86/include/asm/refcount.h
+++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/refcount.h
@@ -67,13 +67,13 @@ 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,
-				  r->refs.counter, "er", i, "%0", e);
+				  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,
-				 r->refs.counter, "%0", e);
+				 r->refs.counter, "%0", e, "cx");
 }
 
 static __always_inline __must_check
--- 4.16-rc2/arch/x86/include/asm/rmwcc.h
+++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/rmwcc.h
@@ -2,8 +2,7 @@
 #ifndef _ASM_X86_RMWcc
 #define _ASM_X86_RMWcc
 
-#define __CLOBBERS_MEM		"memory"
-#define __CLOBBERS_MEM_CC_CX	"memory", "cc", "cx"
+#define __CLOBBERS_MEM(clb...)	"memory", ## clb
 
 #if !defined(__GCC_ASM_FLAG_OUTPUTS__) && defined(CC_HAVE_ASM_GOTO)
 
@@ -40,18 +39,19 @@ do {									\
 #endif /* defined(__GCC_ASM_FLAG_OUTPUTS__) || !defined(CC_HAVE_ASM_GOTO) */
 
 #define GEN_UNARY_RMWcc(op, var, arg0, cc)				\
-	__GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM)
+	__GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM())
 
-#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc)		\
+#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc, clobbers...)\
 	__GEN_RMWcc(op " " arg0 "\n\t" suffix, var, cc,			\
-		    __CLOBBERS_MEM_CC_CX)
+		    __CLOBBERS_MEM(clobbers))
 
 #define GEN_BINARY_RMWcc(op, var, vcon, val, arg0, cc)			\
 	__GEN_RMWcc(op __BINARY_RMWcc_ARG arg0, var, cc,		\
-		    __CLOBBERS_MEM, vcon (val))
+		    __CLOBBERS_MEM(), vcon (val))
 
-#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc)	\
+#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc,	\
+				  clobbers...)				\
 	__GEN_RMWcc(op __BINARY_RMWcc_ARG arg0 "\n\t" suffix, var, cc,	\
-		    __CLOBBERS_MEM_CC_CX, vcon (val))
+		    __CLOBBERS_MEM(clobbers), vcon (val))
 
 #endif /* _ASM_X86_RMWcc */

[tip:x86/pti] x86/asm: Improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[tip-bot for Jan Beulich]

Commit-ID:  700b7c5409c3e9da279fbea78cf28a78fbc176cd
Gitweb:     https://git.kernel.org/tip/700b7c5409c3e9da279fbea78cf28a78fbc176cd
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Mon, 19 Feb 2018 07:49:12 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 20 Feb 2018 09:33:39 +0100

x86/asm: Improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

Commit:

  df3405245a ("x86/asm: Add suffix macro for GEN_*_RMWcc()")

... introduced "suffix" RMWcc operations, adding bogus clobber specifiers:
For one, on x86 there's no point explicitly clobbering "cc".

In fact, with GCC properly fixed, this results in an overlap being detected by
the compiler between outputs and clobbers.

Furthermore it seems bad practice to me to have clobber specification
and use of the clobbered register(s) disconnected - it should rather be
at the invocation place of that GEN_{UN,BIN}ARY_SUFFIXED_RMWcc() macros
that the clobber is specified which this particular invocation needs.

Drop the "cc" clobber altogether and move the "cx" one to refcount.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/5A8AF1F802000078001A91E1@prv-mh.provo.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/refcount.h |  4 ++--
 arch/x86/include/asm/rmwcc.h    | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
index 4e44250..d651711 100644
--- a/arch/x86/include/asm/refcount.h
+++ b/arch/x86/include/asm/refcount.h
@@ -67,13 +67,13 @@ 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,
-				  r->refs.counter, "er", i, "%0", e);
+				  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,
-				 r->refs.counter, "%0", e);
+				 r->refs.counter, "%0", e, "cx");
 }
 
 static __always_inline __must_check
diff --git a/arch/x86/include/asm/rmwcc.h b/arch/x86/include/asm/rmwcc.h
index f91c365..4914a3e 100644
--- a/arch/x86/include/asm/rmwcc.h
+++ b/arch/x86/include/asm/rmwcc.h
@@ -2,8 +2,7 @@
 #ifndef _ASM_X86_RMWcc
 #define _ASM_X86_RMWcc
 
-#define __CLOBBERS_MEM		"memory"
-#define __CLOBBERS_MEM_CC_CX	"memory", "cc", "cx"
+#define __CLOBBERS_MEM(clb...)	"memory", ## clb
 
 #if !defined(__GCC_ASM_FLAG_OUTPUTS__) && defined(CC_HAVE_ASM_GOTO)
 
@@ -40,18 +39,19 @@ do {									\
 #endif /* defined(__GCC_ASM_FLAG_OUTPUTS__) || !defined(CC_HAVE_ASM_GOTO) */
 
 #define GEN_UNARY_RMWcc(op, var, arg0, cc)				\
-	__GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM)
+	__GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM())
 
-#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc)		\
+#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc, clobbers...)\
 	__GEN_RMWcc(op " " arg0 "\n\t" suffix, var, cc,			\
-		    __CLOBBERS_MEM_CC_CX)
+		    __CLOBBERS_MEM(clobbers))
 
 #define GEN_BINARY_RMWcc(op, var, vcon, val, arg0, cc)			\
 	__GEN_RMWcc(op __BINARY_RMWcc_ARG arg0, var, cc,		\
-		    __CLOBBERS_MEM, vcon (val))
+		    __CLOBBERS_MEM(), vcon (val))
 
-#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc)	\
+#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc,	\
+				  clobbers...)				\
 	__GEN_RMWcc(op __BINARY_RMWcc_ARG arg0 "\n\t" suffix, var, cc,	\
-		    __CLOBBERS_MEM_CC_CX, vcon (val))
+		    __CLOBBERS_MEM(clobbers), vcon (val))
 
 #endif /* _ASM_X86_RMWcc */

Re: [PATCH] x86/asm: improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[Kees Cook]

On Mon, Feb 19, 2018 at 6:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
> Commit df3405245a ("x86/asm: Add suffix macro for GEN_*_RMWcc()")
> introduced "suffix" RMWcc operations, adding bogus clobber specifiers:
> For one, on x86 there's no point explicitly clobbering "cc". In fact,

Do you have more details on this? It seems from the GCC clobbers
docs[1] that "cc" is needed for asm that changes the flags register.
Given the explicit subl and decl being used for these macros, it seems
needed to me.

> with gcc properly fixed, this results in an overlap being detected by
> the compiler between outputs and clobbers. Further more it seems bad

Do you mean the flags register is already being included as an
implicit clobber because there is an output of any kind? I can't find
documentation that says this is true. If anything it looks like it
could be "improved" from a full "cc" clobber to an output operand
flag, like =@ccs.

> practice to me to have clobber specification and use of the clobbered
> register(s) disconnected - it should rather be at the invocation place
> of that GEN_{UN,BIN}ARY_SUFFIXED_RMWcc() macros that the clobber is
> specified which this particular invocation needs.
>
> Drop the "cc" clobber altogether and move the "cx" one to refcount.h.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  arch/x86/include/asm/refcount.h |    4 ++--
>  arch/x86/include/asm/rmwcc.h    |   16 ++++++++--------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> --- 4.16-rc2/arch/x86/include/asm/refcount.h
> +++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/refcount.h
> @@ -67,13 +67,13 @@ 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,
> -                                 r->refs.counter, "er", i, "%0", e);
> +                                 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,
> -                                r->refs.counter, "%0", e);
> +                                r->refs.counter, "%0", e, "cx");
>  }
>
>  static __always_inline __must_check
> --- 4.16-rc2/arch/x86/include/asm/rmwcc.h
> +++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/rmwcc.h
> @@ -2,8 +2,7 @@
>  #ifndef _ASM_X86_RMWcc
>  #define _ASM_X86_RMWcc
>
> -#define __CLOBBERS_MEM         "memory"
> -#define __CLOBBERS_MEM_CC_CX   "memory", "cc", "cx"
> +#define __CLOBBERS_MEM(clb...) "memory", ## clb

This leaves a trailing comma in the non-cx case. I thought that caused
me problems in the past, but maybe that's GCC version-specific?

>  #if !defined(__GCC_ASM_FLAG_OUTPUTS__) && defined(CC_HAVE_ASM_GOTO)
>
> @@ -40,18 +39,19 @@ do {                                                                        \
>  #endif /* defined(__GCC_ASM_FLAG_OUTPUTS__) || !defined(CC_HAVE_ASM_GOTO) */
>
>  #define GEN_UNARY_RMWcc(op, var, arg0, cc)                             \
> -       __GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM)
> +       __GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM())
>
> -#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc)            \
> +#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc, clobbers...)\
>         __GEN_RMWcc(op " " arg0 "\n\t" suffix, var, cc,                 \
> -                   __CLOBBERS_MEM_CC_CX)
> +                   __CLOBBERS_MEM(clobbers))
>
>  #define GEN_BINARY_RMWcc(op, var, vcon, val, arg0, cc)                 \
>         __GEN_RMWcc(op __BINARY_RMWcc_ARG arg0, var, cc,                \
> -                   __CLOBBERS_MEM, vcon (val))
> +                   __CLOBBERS_MEM(), vcon (val))
>
> -#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc)        \
> +#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0, cc,        \
> +                                 clobbers...)                          \
>         __GEN_RMWcc(op __BINARY_RMWcc_ARG arg0 "\n\t" suffix, var, cc,  \
> -                   __CLOBBERS_MEM_CC_CX, vcon (val))
> +                   __CLOBBERS_MEM(clobbers), vcon (val))
>
>  #endif /* _ASM_X86_RMWcc */

Anyway, I'm fine to clean it up, sure, but I'd like more details on
why this doesn't break things. :)

Thanks!

-Kees

[1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

-- 
Kees Cook
Pixel Security

Re: [PATCH] x86/asm: improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[hpa]

On February 21, 2018 1:39:18 PM PST, Kees Cook <keescook@chromium.org> wrote:
>On Mon, Feb 19, 2018 at 6:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Commit df3405245a ("x86/asm: Add suffix macro for GEN_*_RMWcc()")
>> introduced "suffix" RMWcc operations, adding bogus clobber
>specifiers:
>> For one, on x86 there's no point explicitly clobbering "cc". In fact,
>
>Do you have more details on this? It seems from the GCC clobbers
>docs[1] that "cc" is needed for asm that changes the flags register.
>Given the explicit subl and decl being used for these macros, it seems
>needed to me.
>
>> with gcc properly fixed, this results in an overlap being detected by
>> the compiler between outputs and clobbers. Further more it seems bad
>
>Do you mean the flags register is already being included as an
>implicit clobber because there is an output of any kind? I can't find
>documentation that says this is true. If anything it looks like it
>could be "improved" from a full "cc" clobber to an output operand
>flag, like =@ccs.
>
>> practice to me to have clobber specification and use of the clobbered
>> register(s) disconnected - it should rather be at the invocation
>place
>> of that GEN_{UN,BIN}ARY_SUFFIXED_RMWcc() macros that the clobber is
>> specified which this particular invocation needs.
>>
>> Drop the "cc" clobber altogether and move the "cx" one to refcount.h.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> ---
>>  arch/x86/include/asm/refcount.h |    4 ++--
>>  arch/x86/include/asm/rmwcc.h    |   16 ++++++++--------
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> --- 4.16-rc2/arch/x86/include/asm/refcount.h
>> +++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/refcount.h
>> @@ -67,13 +67,13 @@ 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,
>> -                                 r->refs.counter, "er", i, "%0", e);
>> +                                 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,
>> -                                r->refs.counter, "%0", e);
>> +                                r->refs.counter, "%0", e, "cx");
>>  }
>>
>>  static __always_inline __must_check
>> --- 4.16-rc2/arch/x86/include/asm/rmwcc.h
>> +++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/rmwcc.h
>> @@ -2,8 +2,7 @@
>>  #ifndef _ASM_X86_RMWcc
>>  #define _ASM_X86_RMWcc
>>
>> -#define __CLOBBERS_MEM         "memory"
>> -#define __CLOBBERS_MEM_CC_CX   "memory", "cc", "cx"
>> +#define __CLOBBERS_MEM(clb...) "memory", ## clb
>
>This leaves a trailing comma in the non-cx case. I thought that caused
>me problems in the past, but maybe that's GCC version-specific?
>
>>  #if !defined(__GCC_ASM_FLAG_OUTPUTS__) && defined(CC_HAVE_ASM_GOTO)
>>
>> @@ -40,18 +39,19 @@ do {                                             
>                          \
>>  #endif /* defined(__GCC_ASM_FLAG_OUTPUTS__) ||
>!defined(CC_HAVE_ASM_GOTO) */
>>
>>  #define GEN_UNARY_RMWcc(op, var, arg0, cc)                          
>  \
>> -       __GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM)
>> +       __GEN_RMWcc(op " " arg0, var, cc, __CLOBBERS_MEM())
>>
>> -#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc)         
>  \
>> +#define GEN_UNARY_SUFFIXED_RMWcc(op, suffix, var, arg0, cc,
>clobbers...)\
>>         __GEN_RMWcc(op " " arg0 "\n\t" suffix, var, cc,              
>  \
>> -                   __CLOBBERS_MEM_CC_CX)
>> +                   __CLOBBERS_MEM(clobbers))
>>
>>  #define GEN_BINARY_RMWcc(op, var, vcon, val, arg0, cc)              
>  \
>>         __GEN_RMWcc(op __BINARY_RMWcc_ARG arg0, var, cc,             
>  \
>> -                   __CLOBBERS_MEM, vcon (val))
>> +                   __CLOBBERS_MEM(), vcon (val))
>>
>> -#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0,
>cc)        \
>> +#define GEN_BINARY_SUFFIXED_RMWcc(op, suffix, var, vcon, val, arg0,
>cc,        \
>> +                                 clobbers...)                       
>  \
>>         __GEN_RMWcc(op __BINARY_RMWcc_ARG arg0 "\n\t" suffix, var,
>cc,  \
>> -                   __CLOBBERS_MEM_CC_CX, vcon (val))
>> +                   __CLOBBERS_MEM(clobbers), vcon (val))
>>
>>  #endif /* _ASM_X86_RMWcc */
>
>Anyway, I'm fine to clean it up, sure, but I'd like more details on
>why this doesn't break things. :)
>
>Thanks!
>
>-Kees
>
>[1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

On x86, gcc always assumes the flags are clobbered; technically it is because the x86 flags aren't implemented using the gcc "cc" internal.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] x86/asm: improve how GEN_*_SUFFIXED_RMWcc() specify clobbers

[Jan Beulich]

>>> On 21.02.18 at 22:39, <keescook@chromium.org> wrote:
> On Mon, Feb 19, 2018 at 6:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Commit df3405245a ("x86/asm: Add suffix macro for GEN_*_RMWcc()")
>> introduced "suffix" RMWcc operations, adding bogus clobber specifiers:
>> For one, on x86 there's no point explicitly clobbering "cc". In fact,
> 
> Do you have more details on this? It seems from the GCC clobbers
> docs[1] that "cc" is needed for asm that changes the flags register.
> Given the explicit subl and decl being used for these macros, it seems
> needed to me.
> 
>> with gcc properly fixed, this results in an overlap being detected by
>> the compiler between outputs and clobbers. Further more it seems bad
> 
> Do you mean the flags register is already being included as an
> implicit clobber because there is an output of any kind? I can't find
> documentation that says this is true. If anything it looks like it
> could be "improved" from a full "cc" clobber to an output operand
> flag, like =@ccs.

As hpa has already said, "cc" has been automatically included as a
clobber forever on x86 (until the condition code outputs appeared,
at which point the clobber became added only if no such output
was present).

>> --- 4.16-rc2/arch/x86/include/asm/rmwcc.h
>> +++ 4.16-rc2-x86-rmwcc-clobbers/arch/x86/include/asm/rmwcc.h
>> @@ -2,8 +2,7 @@
>>  #ifndef _ASM_X86_RMWcc
>>  #define _ASM_X86_RMWcc
>>
>> -#define __CLOBBERS_MEM         "memory"
>> -#define __CLOBBERS_MEM_CC_CX   "memory", "cc", "cx"
>> +#define __CLOBBERS_MEM(clb...) "memory", ## clb
> 
> This leaves a trailing comma in the non-cx case. I thought that caused
> me problems in the past, but maybe that's GCC version-specific?

No, it's formally specified to drop the comma. Note this is using a
gcc extension, not the C99 way of having a macro with variable
number of arguments (where - without again some gcc special
handling - this indeed would be a problem).

Jan