linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86_64: fix jiffies ODR violation
@ 2020-05-15 18:05 Bob Haarman
  2020-05-19  3:17 ` Andi Kleen
  2020-06-02 13:27 ` Josh Poimboeuf
  0 siblings, 2 replies; 7+ messages in thread
From: Bob Haarman @ 2020-05-15 18:05 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Fangrui Song, Sami Tolvanen, Bob Haarman, stable,
	Nathan Chancellor, Alistair Delva, Nick Desaulniers, x86,
	H. Peter Anvin, Masami Hiramatsu, Josh Poimboeuf,
	Vincenzo Frascino, Kyung Min Park, afzal mohammed,
	Peter Zijlstra, Kees Cook, Heiko Carstens, Dave Hansen,
	Baoquan He, Thomas Lendacky, Ross Zwisler, Arvind Sankar,
	Dmitry Safonov, Andi Kleen, linux-kernel, clang-built-linux

`jiffies` and `jiffies_64` are meant to alias (two different symbols
that share the same address).  Most architectures make the symbols alias
to the same address via linker script assignment in their
arch/<arch>/kernel/vmlinux.lds.S:

jiffies = jiffies_64;

which is effectively a definition of jiffies.

jiffies and jiffies_64 are both forward declared for all arch's in:
include/linux/jiffies.h.

jiffies_64 is defined in kernel/time/timer.c for all arch's.

x86_64 was peculiar in that it wasn't doing the above linker script
assignment, but rather was:
1. defining jiffies in arch/x86/kernel/time.c instead via linker script.
2. overriding the symbol jiffies_64 from kernel/time/timer.c in
arch/x86/kernel/vmlinux.lds.s via `jiffies_64 = jiffies;`.

As Fangrui notes:
```
In LLD, symbol assignments in linker scripts override definitions in
object files. GNU ld appears to have the same behavior. It would
probably make sense for LLD to error "duplicate symbol" but GNU ld is
unlikely to adopt for compatibility reasons.
```

So we have an ODR violation (UB), which we seem to have gotten away
with thus far. Where it becomes harmful is when we:

1. Use -fno-semantic-interposition.

As Fangrui notes:
```
Clang after LLVM
commit 5b22bcc2b70d ("[X86][ELF] Prefer to lower MC_GlobalAddress
operands to .Lfoo$local")
defaults to -fno-semantic-interposition similar semantics which help
-fpic/-fPIC code avoid GOT/PLT when the referenced symbol is defined
within the same translation unit. Unlike GCC
-fno-semantic-interposition, Clang emits such relocations referencing
local symbols for non-pic code as well.
```

This causes references to jiffies to refer to `.Ljiffies$local` when
jiffies is defined in the same translation unit. Likewise, references
to jiffies_64 become references to `.Ljiffies_64$local` in translation
units that define jiffies_64.  Because these differ from the names
used in the linker script, they will not be rewritten to alias one
another.

Combined with ...

2. Full LTO effectively treats all source files as one translation
unit, causing these local references to be produced everywhere.  When
the linker processes the linker script, there are no longer any
references to `jiffies_64` anywhere to replace with `jiffies`.  And
thus `.Ljiffies$local` and `.Ljiffies_64$local` no longer alias
at all.

In the process of porting patches enabling Full LTO from arm64 to
x86_64, we observe spooky bugs where the kernel appeared to boot, but
init doesn't get scheduled.

Instead, we can avoid the ODR violation by matching other arch's by
defining jiffies only by linker script.  For -fno-semantic-interposition
+ Full LTO, there is no longer a global definition of jiffies for the
compiler to produce a local symbol which the linker script won't ensure
aliases to jiffies_64.

Link: https://github.com/ClangBuiltLinux/linux/issues/852
Fixes: 40747ffa5aa8 ("asmlinkage: Make jiffies visible")
Cc: stable@vger.kernel.org
Reported-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: Alistair Delva <adelva@google.com>
Suggested-by: Fangrui Song <maskray@google.com>
Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
Debugged-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Bob Haarman <inglorion@google.com>
---
 arch/x86/kernel/time.c        | 4 ----
 arch/x86/kernel/vmlinux.lds.S | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 371a6b348e44..e42faa792c07 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -25,10 +25,6 @@
 #include <asm/hpet.h>
 #include <asm/time.h>
 
-#ifdef CONFIG_X86_64
-__visible volatile unsigned long jiffies __cacheline_aligned_in_smp = INITIAL_JIFFIES;
-#endif
-
 unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 1bf7e312361f..7c35556c7827 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -40,13 +40,13 @@ OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
 #ifdef CONFIG_X86_32
 OUTPUT_ARCH(i386)
 ENTRY(phys_startup_32)
-jiffies = jiffies_64;
 #else
 OUTPUT_ARCH(i386:x86-64)
 ENTRY(phys_startup_64)
-jiffies_64 = jiffies;
 #endif
 
+jiffies = jiffies_64;
+
 #if defined(CONFIG_X86_64)
 /*
  * On 64-bit, align RODATA to 2MB so we retain large page mappings for
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86_64: fix jiffies ODR violation
  2020-05-15 18:05 [PATCH] x86_64: fix jiffies ODR violation Bob Haarman
@ 2020-05-19  3:17 ` Andi Kleen
  2020-06-01 23:47   ` Bob Haarman
  2020-06-02 13:27 ` Josh Poimboeuf
  1 sibling, 1 reply; 7+ messages in thread
From: Andi Kleen @ 2020-05-19  3:17 UTC (permalink / raw)
  To: Bob Haarman
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Fangrui Song,
	Sami Tolvanen, stable, Nathan Chancellor, Alistair Delva,
	Nick Desaulniers, x86, H. Peter Anvin, Masami Hiramatsu,
	Josh Poimboeuf, Vincenzo Frascino, Kyung Min Park,
	afzal mohammed, Peter Zijlstra, Kees Cook, Heiko Carstens,
	Dave Hansen, Baoquan He, Thomas Lendacky, Ross Zwisler,
	Arvind Sankar, Dmitry Safonov, linux-kernel, clang-built-linux

> Instead, we can avoid the ODR violation by matching other arch's by
> defining jiffies only by linker script.  For -fno-semantic-interposition
> + Full LTO, there is no longer a global definition of jiffies for the
> compiler to produce a local symbol which the linker script won't ensure
> aliases to jiffies_64.

I guess it was an historical accident.

Reviewed-by: Andi Kleen <ak@linux.intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86_64: fix jiffies ODR violation
  2020-05-19  3:17 ` Andi Kleen
@ 2020-06-01 23:47   ` Bob Haarman
  0 siblings, 0 replies; 7+ messages in thread
From: Bob Haarman @ 2020-06-01 23:47 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Andrew Morton
  Cc: Andi Kleen, Fangrui Song, Sami Tolvanen, stable,
	Nathan Chancellor, Alistair Delva, Nick Desaulniers, x86,
	H. Peter Anvin, Masami Hiramatsu, Josh Poimboeuf,
	Vincenzo Frascino, Kyung Min Park, afzal mohammed,
	Peter Zijlstra, Kees Cook, Heiko Carstens, Dave Hansen,
	Baoquan He, Thomas Lendacky, Ross Zwisler, Arvind Sankar,
	Dmitry Safonov, linux-kernel, clang-built-linux

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

On Mon, May 18, 2020 at 08:17:42PM -0700, Andi Kleen wrote:
> > Instead, we can avoid the ODR violation by matching other arch's by
> > defining jiffies only by linker script.  For -fno-semantic-interposition
> > + Full LTO, there is no longer a global definition of jiffies for the
> > compiler to produce a local symbol which the linker script won't ensure
> > aliases to jiffies_64.
> 
> I guess it was an historical accident.
> 
> Reviewed-by: Andi Kleen <ak@linux.intel.com>

Thank you, Andi. Do any other reviewers have comments?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3856 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86_64: fix jiffies ODR violation
  2020-05-15 18:05 [PATCH] x86_64: fix jiffies ODR violation Bob Haarman
  2020-05-19  3:17 ` Andi Kleen
@ 2020-06-02 13:27 ` Josh Poimboeuf
  2020-06-02 19:30   ` [PATCH v2] " Bob Haarman
  1 sibling, 1 reply; 7+ messages in thread
From: Josh Poimboeuf @ 2020-06-02 13:27 UTC (permalink / raw)
  To: Bob Haarman
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Fangrui Song,
	Sami Tolvanen, stable, Nathan Chancellor, Alistair Delva,
	Nick Desaulniers, x86, H. Peter Anvin, Masami Hiramatsu,
	Vincenzo Frascino, Kyung Min Park, afzal mohammed,
	Peter Zijlstra, Kees Cook, Heiko Carstens, Dave Hansen,
	Baoquan He, Thomas Lendacky, Ross Zwisler, Arvind Sankar,
	Dmitry Safonov, Andi Kleen, linux-kernel, clang-built-linux

On Fri, May 15, 2020 at 11:05:40AM -0700, Bob Haarman wrote:
> `jiffies`
[...]
> `jiffies_64`
[...]
> ```
> In LLD, symbol assignments in linker scripts override definitions in
> object files. GNU ld appears to have the same behavior. It would
> probably make sense for LLD to error "duplicate symbol" but GNU ld is
> unlikely to adopt for compatibility reasons.
> ```

Kernel commit logs shouldn't be in Markdown.

Symbol names can just be in single quotes (not back-quotes!) like
'jiffies'.

Quotes can be indented by a few spaces for visual separation, like

  In LLD, symbol assignments in linker scripts override definitions in
  object files. GNU ld appears to have the same behavior. It would
  probably make sense for LLD to error "duplicate symbol" but GNU ld is
  unlikely to adopt for compatibility reasons.

or can be formatting like an email quote:

> In LLD, symbol assignments in linker scripts override definitions in
> object files. GNU ld appears to have the same behavior. It would
> probably make sense for LLD to error "duplicate symbol" but GNU ld is
> unlikely to adopt for compatibility reasons.


With Markdown-isms removed from the patch description:

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] x86_64: fix jiffies ODR violation
  2020-06-02 13:27 ` Josh Poimboeuf
@ 2020-06-02 19:30   ` Bob Haarman
  2020-06-04  7:30     ` Sedat Dilek
  2020-06-09  8:53     ` [tip: x86/urgent] x86_64: Fix " tip-bot2 for Bob Haarman
  0 siblings, 2 replies; 7+ messages in thread
From: Bob Haarman @ 2020-06-02 19:30 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Andrew Morton,
	Josh Poimboeuf
  Cc: Bob Haarman, stable, Nathan Chancellor, Alistair Delva,
	Fangrui Song, Nick Desaulniers, Sami Tolvanen, Andi Kleen, x86,
	H. Peter Anvin, afzal mohammed, Kyung Min Park, Peter Zijlstra,
	Kees Cook, Heiko Carstens, Baoquan He, Thomas Lendacky, H.J. Lu,
	Ross Zwisler, Arvind Sankar, Dmitry Safonov, linux-kernel,
	clang-built-linux

'jiffies' and 'jiffies_64' are meant to alias (two different symbols
that share the same address).  Most architectures make the symbols alias
to the same address via linker script assignment in their
arch/<arch>/kernel/vmlinux.lds.S:

jiffies = jiffies_64;

which is effectively a definition of jiffies.

jiffies and jiffies_64 are both forward declared for all arch's in:
include/linux/jiffies.h.

jiffies_64 is defined in kernel/time/timer.c for all arch's.

x86_64 was peculiar in that it wasn't doing the above linker script
assignment, but rather was:
1. defining jiffies in arch/x86/kernel/time.c instead via linker script.
2. overriding the symbol jiffies_64 from kernel/time/timer.c in
arch/x86/kernel/vmlinux.lds.s via 'jiffies_64 = jiffies;'.

As Fangrui notes:

  In LLD, symbol assignments in linker scripts override definitions in
  object files. GNU ld appears to have the same behavior. It would
  probably make sense for LLD to error "duplicate symbol" but GNU ld
  is unlikely to adopt for compatibility reasons.

So we have an ODR violation (UB), which we seem to have gotten away
with thus far. Where it becomes harmful is when we:

1. Use -fno-semantic-interposition.

As Fangrui notes:

  Clang after LLVM commit 5b22bcc2b70d
  ("[X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local")
  defaults to -fno-semantic-interposition similar semantics which help
  -fpic/-fPIC code avoid GOT/PLT when the referenced symbol is defined
  within the same translation unit. Unlike GCC
  -fno-semantic-interposition, Clang emits such relocations referencing
  local symbols for non-pic code as well.

This causes references to jiffies to refer to '.Ljiffies$local' when
jiffies is defined in the same translation unit. Likewise, references
to jiffies_64 become references to '.Ljiffies_64$local' in translation
units that define jiffies_64.  Because these differ from the names
used in the linker script, they will not be rewritten to alias one
another.

Combined with ...

2. Full LTO effectively treats all source files as one translation
unit, causing these local references to be produced everywhere.  When
the linker processes the linker script, there are no longer any
references to jiffies_64' anywhere to replace with 'jiffies'.  And
thus '.Ljiffies$local' and '.Ljiffies_64$local' no longer alias
at all.

In the process of porting patches enabling Full LTO from arm64 to
x86_64, we observe spooky bugs where the kernel appeared to boot, but
init doesn't get scheduled.

Instead, we can avoid the ODR violation by matching other arch's by
defining jiffies only by linker script.  For -fno-semantic-interposition
+ Full LTO, there is no longer a global definition of jiffies for the
compiler to produce a local symbol which the linker script won't ensure
aliases to jiffies_64.

Link: https://github.com/ClangBuiltLinux/linux/issues/852
Fixes: 40747ffa5aa8 ("asmlinkage: Make jiffies visible")
Cc: stable@vger.kernel.org
Reported-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: Alistair Delva <adelva@google.com>
Suggested-by: Fangrui Song <maskray@google.com>
Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
Debugged-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Bob Haarman <inglorion@google.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
v2:
* Changed commit message as requested by Josh Poimboeuf
  (no code change)

---
 arch/x86/kernel/time.c        | 4 ----
 arch/x86/kernel/vmlinux.lds.S | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 371a6b348e44..e42faa792c07 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -25,10 +25,6 @@
 #include <asm/hpet.h>
 #include <asm/time.h>
 
-#ifdef CONFIG_X86_64
-__visible volatile unsigned long jiffies __cacheline_aligned_in_smp = INITIAL_JIFFIES;
-#endif
-
 unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 1bf7e312361f..7c35556c7827 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -40,13 +40,13 @@ OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
 #ifdef CONFIG_X86_32
 OUTPUT_ARCH(i386)
 ENTRY(phys_startup_32)
-jiffies = jiffies_64;
 #else
 OUTPUT_ARCH(i386:x86-64)
 ENTRY(phys_startup_64)
-jiffies_64 = jiffies;
 #endif
 
+jiffies = jiffies_64;
+
 #if defined(CONFIG_X86_64)
 /*
  * On 64-bit, align RODATA to 2MB so we retain large page mappings for
-- 
2.27.0.rc2.251.g90737beb825-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] x86_64: fix jiffies ODR violation
  2020-06-02 19:30   ` [PATCH v2] " Bob Haarman
@ 2020-06-04  7:30     ` Sedat Dilek
  2020-06-09  8:53     ` [tip: x86/urgent] x86_64: Fix " tip-bot2 for Bob Haarman
  1 sibling, 0 replies; 7+ messages in thread
From: Sedat Dilek @ 2020-06-04  7:30 UTC (permalink / raw)
  To: Bob Haarman
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Andrew Morton,
	Josh Poimboeuf, stable, Nathan Chancellor, Alistair Delva,
	Fangrui Song, Nick Desaulniers, Sami Tolvanen, Andi Kleen, x86,
	H. Peter Anvin, afzal mohammed, Kyung Min Park, Peter Zijlstra,
	Kees Cook, Heiko Carstens, Baoquan He, Thomas Lendacky, H.J. Lu,
	Ross Zwisler, Arvind Sankar, Dmitry Safonov, linux-kernel,
	Clang-Built-Linux ML

On Tue, Jun 2, 2020 at 9:31 PM 'Bob Haarman' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> 'jiffies' and 'jiffies_64' are meant to alias (two different symbols
> that share the same address).  Most architectures make the symbols alias
> to the same address via linker script assignment in their
> arch/<arch>/kernel/vmlinux.lds.S:
>
> jiffies = jiffies_64;
>
> which is effectively a definition of jiffies.
>
> jiffies and jiffies_64 are both forward declared for all arch's in:
> include/linux/jiffies.h.
>
> jiffies_64 is defined in kernel/time/timer.c for all arch's.
>
> x86_64 was peculiar in that it wasn't doing the above linker script
> assignment, but rather was:
> 1. defining jiffies in arch/x86/kernel/time.c instead via linker script.
> 2. overriding the symbol jiffies_64 from kernel/time/timer.c in
> arch/x86/kernel/vmlinux.lds.s via 'jiffies_64 = jiffies;'.
>
> As Fangrui notes:
>
>   In LLD, symbol assignments in linker scripts override definitions in
>   object files. GNU ld appears to have the same behavior. It would
>   probably make sense for LLD to error "duplicate symbol" but GNU ld
>   is unlikely to adopt for compatibility reasons.
>
> So we have an ODR violation (UB), which we seem to have gotten away
> with thus far. Where it becomes harmful is when we:
>
> 1. Use -fno-semantic-interposition.
>
> As Fangrui notes:
>
>   Clang after LLVM commit 5b22bcc2b70d
>   ("[X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local")
>   defaults to -fno-semantic-interposition similar semantics which help
>   -fpic/-fPIC code avoid GOT/PLT when the referenced symbol is defined
>   within the same translation unit. Unlike GCC
>   -fno-semantic-interposition, Clang emits such relocations referencing
>   local symbols for non-pic code as well.
>
> This causes references to jiffies to refer to '.Ljiffies$local' when
> jiffies is defined in the same translation unit. Likewise, references
> to jiffies_64 become references to '.Ljiffies_64$local' in translation
> units that define jiffies_64.  Because these differ from the names
> used in the linker script, they will not be rewritten to alias one
> another.
>
> Combined with ...
>
> 2. Full LTO effectively treats all source files as one translation
> unit, causing these local references to be produced everywhere.  When
> the linker processes the linker script, there are no longer any
> references to jiffies_64' anywhere to replace with 'jiffies'.  And
> thus '.Ljiffies$local' and '.Ljiffies_64$local' no longer alias
> at all.
>
> In the process of porting patches enabling Full LTO from arm64 to
> x86_64, we observe spooky bugs where the kernel appeared to boot, but
> init doesn't get scheduled.
>
> Instead, we can avoid the ODR violation by matching other arch's by
> defining jiffies only by linker script.  For -fno-semantic-interposition
> + Full LTO, there is no longer a global definition of jiffies for the
> compiler to produce a local symbol which the linker script won't ensure
> aliases to jiffies_64.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/852
> Fixes: 40747ffa5aa8 ("asmlinkage: Make jiffies visible")
> Cc: stable@vger.kernel.org
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: Alistair Delva <adelva@google.com>
> Suggested-by: Fangrui Song <maskray@google.com>
> Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
> Debugged-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Bob Haarman <inglorion@google.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> v2:
> * Changed commit message as requested by Josh Poimboeuf
>   (no code change)
>

Hi,

I have tested v2 with my local patch-series together.

Feel free to add my...

Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # build+boot on
Debian/testing AMD64 with selfmade llvm-toolchain v10.0.1-rc1+

Thanks.

Regards,
- Sedat -

> ---
>  arch/x86/kernel/time.c        | 4 ----
>  arch/x86/kernel/vmlinux.lds.S | 4 ++--
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
> index 371a6b348e44..e42faa792c07 100644
> --- a/arch/x86/kernel/time.c
> +++ b/arch/x86/kernel/time.c
> @@ -25,10 +25,6 @@
>  #include <asm/hpet.h>
>  #include <asm/time.h>
>
> -#ifdef CONFIG_X86_64
> -__visible volatile unsigned long jiffies __cacheline_aligned_in_smp = INITIAL_JIFFIES;
> -#endif
> -
>  unsigned long profile_pc(struct pt_regs *regs)
>  {
>         unsigned long pc = instruction_pointer(regs);
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index 1bf7e312361f..7c35556c7827 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -40,13 +40,13 @@ OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
>  #ifdef CONFIG_X86_32
>  OUTPUT_ARCH(i386)
>  ENTRY(phys_startup_32)
> -jiffies = jiffies_64;
>  #else
>  OUTPUT_ARCH(i386:x86-64)
>  ENTRY(phys_startup_64)
> -jiffies_64 = jiffies;
>  #endif
>
> +jiffies = jiffies_64;
> +
>  #if defined(CONFIG_X86_64)
>  /*
>   * On 64-bit, align RODATA to 2MB so we retain large page mappings for
> --
> 2.27.0.rc2.251.g90737beb825-goog
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200602193100.229287-1-inglorion%40google.com.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip: x86/urgent] x86_64: Fix jiffies ODR violation
  2020-06-02 19:30   ` [PATCH v2] " Bob Haarman
  2020-06-04  7:30     ` Sedat Dilek
@ 2020-06-09  8:53     ` tip-bot2 for Bob Haarman
  1 sibling, 0 replies; 7+ messages in thread
From: tip-bot2 for Bob Haarman @ 2020-06-09  8:53 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Nathan Chancellor, Alistair Delva, Fangrui Song, Bob Haarman,
	Thomas Gleixner, Sedat Dilek, Andi Kleen, Josh Poimboeuf, stable,
	x86, LKML

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     d8ad6d39c35d2b44b3d48b787df7f3359381dcbf
Gitweb:        https://git.kernel.org/tip/d8ad6d39c35d2b44b3d48b787df7f3359381dcbf
Author:        Bob Haarman <inglorion@google.com>
AuthorDate:    Tue, 02 Jun 2020 12:30:59 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 09 Jun 2020 10:50:56 +02:00

x86_64: Fix jiffies ODR violation

'jiffies' and 'jiffies_64' are meant to alias (two different symbols that
share the same address).  Most architectures make the symbols alias to the
same address via a linker script assignment in their
arch/<arch>/kernel/vmlinux.lds.S:

jiffies = jiffies_64;

which is effectively a definition of jiffies.

jiffies and jiffies_64 are both forward declared for all architectures in
include/linux/jiffies.h. jiffies_64 is defined in kernel/time/timer.c.

x86_64 was peculiar in that it wasn't doing the above linker script
assignment, but rather was:
1. defining jiffies in arch/x86/kernel/time.c instead via the linker script.
2. overriding the symbol jiffies_64 from kernel/time/timer.c in
arch/x86/kernel/vmlinux.lds.s via 'jiffies_64 = jiffies;'.

As Fangrui notes:

  In LLD, symbol assignments in linker scripts override definitions in
  object files. GNU ld appears to have the same behavior. It would
  probably make sense for LLD to error "duplicate symbol" but GNU ld
  is unlikely to adopt for compatibility reasons.

This results in an ODR violation (UB), which seems to have survived
thus far. Where it becomes harmful is when;

1. -fno-semantic-interposition is used:

As Fangrui notes:

  Clang after LLVM commit 5b22bcc2b70d
  ("[X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local")
  defaults to -fno-semantic-interposition similar semantics which help
  -fpic/-fPIC code avoid GOT/PLT when the referenced symbol is defined
  within the same translation unit. Unlike GCC
  -fno-semantic-interposition, Clang emits such relocations referencing
  local symbols for non-pic code as well.

This causes references to jiffies to refer to '.Ljiffies$local' when
jiffies is defined in the same translation unit. Likewise, references to
jiffies_64 become references to '.Ljiffies_64$local' in translation units
that define jiffies_64.  Because these differ from the names used in the
linker script, they will not be rewritten to alias one another.

2. Full LTO

Full LTO effectively treats all source files as one translation
unit, causing these local references to be produced everywhere.  When
the linker processes the linker script, there are no longer any
references to jiffies_64' anywhere to replace with 'jiffies'.  And
thus '.Ljiffies$local' and '.Ljiffies_64$local' no longer alias
at all.

In the process of porting patches enabling Full LTO from arm64 to x86_64,
spooky bugs have been observed where the kernel appeared to boot, but init
doesn't get scheduled.

Avoid the ODR violation by matching other architectures and define jiffies
only by linker script.  For -fno-semantic-interposition + Full LTO, there
is no longer a global definition of jiffies for the compiler to produce a
local symbol which the linker script won't ensure aliases to jiffies_64.

Fixes: 40747ffa5aa8 ("asmlinkage: Make jiffies visible")
Reported-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: Alistair Delva <adelva@google.com>
Debugged-by: Nick Desaulniers <ndesaulniers@google.com>
Debugged-by: Sami Tolvanen <samitolvanen@google.com>
Suggested-by: Fangrui Song <maskray@google.com>
Signed-off-by: Bob Haarman <inglorion@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # build+boot on
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Link: https://github.com/ClangBuiltLinux/linux/issues/852
Link: https://lkml.kernel.org/r/20200602193100.229287-1-inglorion@google.com
---
 arch/x86/kernel/time.c        | 4 ----
 arch/x86/kernel/vmlinux.lds.S | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 106e7f8..f395729 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -25,10 +25,6 @@
 #include <asm/hpet.h>
 #include <asm/time.h>
 
-#ifdef CONFIG_X86_64
-__visible volatile unsigned long jiffies __cacheline_aligned_in_smp = INITIAL_JIFFIES;
-#endif
-
 unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 1bf7e31..7c35556 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -40,13 +40,13 @@ OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
 #ifdef CONFIG_X86_32
 OUTPUT_ARCH(i386)
 ENTRY(phys_startup_32)
-jiffies = jiffies_64;
 #else
 OUTPUT_ARCH(i386:x86-64)
 ENTRY(phys_startup_64)
-jiffies_64 = jiffies;
 #endif
 
+jiffies = jiffies_64;
+
 #if defined(CONFIG_X86_64)
 /*
  * On 64-bit, align RODATA to 2MB so we retain large page mappings for

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-09  8:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15 18:05 [PATCH] x86_64: fix jiffies ODR violation Bob Haarman
2020-05-19  3:17 ` Andi Kleen
2020-06-01 23:47   ` Bob Haarman
2020-06-02 13:27 ` Josh Poimboeuf
2020-06-02 19:30   ` [PATCH v2] " Bob Haarman
2020-06-04  7:30     ` Sedat Dilek
2020-06-09  8:53     ` [tip: x86/urgent] x86_64: Fix " tip-bot2 for Bob Haarman

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).