All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
	Nick Desaulniers <ndesaulniers@google.com>,
	 Masahiro Yamada <masahiroy@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	kvm@vger.kernel.org,  Sean Christopherson <seanjc@google.com>
Subject: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)
Date: Thu,  8 Feb 2024 14:06:04 -0800	[thread overview]
Message-ID: <20240208220604.140859-1-seanjc@google.com> (raw)

Explicitly require gcc-12+ to enable asm goto with outputs on gcc to avoid
what is effectively a data corruption bug on gcc-11.  As per
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html, "asm goto" is
*supposed* be implicitly volatile, but gcc-11 fails to treat it as such.
When compiling with -O2, failure to treat the asm block as volatile can
result in the entire block being discarded during optimization.

Even worse, forcing "asm volatile goto" keeps the block, but generates
completely bogus code.

Hardcode the gcc-12 or later requirement as trying to pipe the assembled
output to stdout, e.g. to query the generated code via objdump, doesn't
work due to the assembler wanting to seek throughout the output file.

Note, gcc-11 is the first gcc version that supports goto w/ outputs
(obviously with a loose definition of "supports").

E.g. given KVM's code sequence:

  vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
  vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
  vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
  vmcs12->guest_pdptr3 = vmcs_read64(GUEST_PDPTR3);

where vmcs_read64() eventually becomes:

	asm_volatile_goto("1: vmread %[field], %[output]\n\t"
			  "jna %l[do_fail]\n\t"

			  _ASM_EXTABLE(1b, %l[do_exception])

			  : [output] "=r" (value)
			  : [field] "r" (field)
			  : "cc"
			  : do_fail, do_exception);

	return value;

  do_fail:
	instrumentation_begin();
	vmread_error(field);
	instrumentation_end();
	return 0;

  do_exception:
	kvm_spurious_fault();
	return 0;

the sequence of VMREADs should generate:

   nopl   0x0(%rax,%rax,1)
   mov    $0x280a,%eax
   vmread %rax,%rax
   jbe    0xffffffff81099849 <sync_vmcs02_to_vmcs12+1929>
   mov    %rax,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109982c <sync_vmcs02_to_vmcs12+1900>
   mov    %rax,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109980f <sync_vmcs02_to_vmcs12+1871>
   mov    %rax,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x2810,%eax
   vmread %rax,%rax
   jbe    0xffffffff810997f2 <sync_vmcs02_to_vmcs12+1842>
   mov    %rax,0xf0(%rbx)
   jmp    0xffffffff81099297 <sync_vmcs02_to_vmcs12+471>

but gcc-11 will omit the asm block for the VMREAD to GUEST_PDPTR3 and skip
straight to one of the "return 0" statements:

   nopl   0x0(%rax,%rax,1)
   mov    $0x280a,%r13d
   vmread %r13,%r13
   jbe    0xffffffff810996cd <sync_vmcs02_to_vmcs12+1949>
   mov    %r13,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%r13d
   vmread %r13,%r13
   jbe    0xffffffff810996ae <sync_vmcs02_to_vmcs12+1918>
   mov    %r13,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%r13d
   vmread %r13,%r13
   jbe    0xffffffff8109968f <sync_vmcs02_to_vmcs12+1887>
   mov    %r13,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   xor    %r12d,%r12d      <= return 0
   mov    %r12,0xf0(%rbx)  <= store result to vmcs12->guest_pdptr3
   jmp    0xffffffff8109912c <sync_vmcs02_to_vmcs12+508>

and with "volatile" forced, gcc-11 generates the correct-at-first-glance,
but terribly broken sequence of:

   nopl   0x0(%rax,%rax,1)
   mov    $0x280a,%r13d
   vmread %r13,%r13
   jbe    0xffffffff810999a4 <sync_vmcs02_to_vmcs12+1988>
   mov    %r13,0xd8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280c,%r13d
   vmread %r13,%r13
   jbe    0xffffffff81099985 <sync_vmcs02_to_vmcs12+1957>
   mov    %r13,0xe0(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x280e,%r13d
   vmread %r13,%r13
   jbe    0xffffffff81099966 <sync_vmcs02_to_vmcs12+1926>
   mov    %r13,0xe8(%rbx)
   nopl   0x0(%rax,%rax,1)
   mov    $0x2810,%eax
   vmread %rax,%rax
   jbe    0xffffffff8109994a <sync_vmcs02_to_vmcs12+1898>
   xor    %r12d,%r12d     <= WTF gcc!?!?!
   mov    %r12,0xf0(%rbx)

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
Fixes: 587f17018a2c ("Kconfig: add config option for asm goto w/ outputs")
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: kvm@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Linus, I'm sending to you directly as this seems urgent enough to apply
straightaway, and this obviously affects much more than the build system.

 init/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index deda3d14135b..f4e46d64c1e7 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -82,6 +82,11 @@ config CC_CAN_LINK_STATIC
 	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
 
 config CC_HAS_ASM_GOTO_OUTPUT
+	# gcc-11 has a nasty bug where it doesn't treat asm goto as volatile,
+	# which can result in asm blocks being dropped when compiling with -02.
+	# Note, explicitly forcing volatile doesn't entirely fix the bug!
+	# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
+	depends on !CC_IS_GCC || GCC_VERSION >= 120000
 	def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
 
 config CC_HAS_ASM_GOTO_TIED_OUTPUT

base-commit: 047371968ffc470769f541d6933e262dc7085456
-- 
2.43.0.687.g38aa6559b0-goog


             reply	other threads:[~2024-02-08 22:06 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-08 22:06 Sean Christopherson [this message]
2024-02-09 17:03 ` [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier) Nick Desaulniers
2024-02-09 17:14   ` Andrew Pinski (QUIC)
2024-02-09 17:55     ` Linus Torvalds
2024-02-09 18:43       ` Sean Christopherson
2024-02-09 18:55         ` Nick Desaulniers
2024-02-09 19:01           ` Linus Torvalds
2024-02-09 19:20             ` Nick Desaulniers
2024-02-09 20:39             ` Linus Torvalds
2024-02-09 21:46               ` Sean Christopherson
2024-02-10 17:21                 ` David Laight
2024-02-11 11:12               ` Uros Bizjak
2024-02-11 19:59                 ` Linus Torvalds
2024-02-11 20:12                   ` Jakub Jelinek
2024-02-13  0:15                   ` Sean Christopherson
2024-02-14 17:20                     ` Sean Christopherson
2024-02-14 18:43                 ` Linus Torvalds
2024-02-15  0:11                   ` Linus Torvalds
2024-02-15  8:39                     ` Jakub Jelinek
2024-02-15 18:25                       ` Linus Torvalds
2024-02-15 19:17                         ` Linus Torvalds
2024-02-15 19:26                           ` Jakub Jelinek
2024-02-09 18:56         ` Linus Torvalds
2024-02-09 19:26           ` Andrew Pinski (QUIC)

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=20240208220604.140859-1-seanjc@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=masahiroy@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=peterz@infradead.org \
    --cc=torvalds@linux-foundation.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.