All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/entry: use STB_GLOBAL for register restoring thunk
@ 2020-12-23 23:21 Nick Desaulniers
  2020-12-24  4:55 ` Josh Poimboeuf
  0 siblings, 1 reply; 33+ messages in thread
From: Nick Desaulniers @ 2020-12-23 23:21 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Nick Desaulniers, Fangrui Song, Arnd Bergmann, Josh Poimboeuf,
	x86, H. Peter Anvin, Nathan Chancellor, linux-kernel,
	clang-built-linux

Arnd found a randconfig that produces the warning:

arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
offset 0x3e

when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
notes:

  With the LLVM assembler stripping the .text section symbol, objtool
  has no way to reference this code when it generates ORC unwinder
  entries, because this code is outside of any ELF function.

This behavior was implemented as an optimization in LLVM 5 years ago,
but it's not the first time this has caused issues for objtool.  A patch
has been authored against LLVM to revert the behavior, which may or may
not be accepted.  Until then use a global symbol for the thunk that way
objtool can generate proper unwind info here with LLVM_IAS=1.

Cc: Fangrui Song <maskray@google.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/entry/thunk_64.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..878816034a73 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
-- 
2.29.2.729.g45daf8777d-goog


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

* Re: [PATCH] x86/entry: use STB_GLOBAL for register restoring thunk
  2020-12-23 23:21 [PATCH] x86/entry: use STB_GLOBAL for register restoring thunk Nick Desaulniers
@ 2020-12-24  4:55 ` Josh Poimboeuf
  2021-01-06  0:43   ` [PATCH v2] " Nick Desaulniers
  0 siblings, 1 reply; 33+ messages in thread
From: Josh Poimboeuf @ 2020-12-24  4:55 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Fangrui Song, Arnd Bergmann, x86, H. Peter Anvin,
	Nathan Chancellor, linux-kernel, clang-built-linux

On Wed, Dec 23, 2020 at 03:21:26PM -0800, Nick Desaulniers wrote:
> Arnd found a randconfig that produces the warning:
> 
> arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> offset 0x3e
> 
> when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler stripping the .text section symbol, objtool
>   has no way to reference this code when it generates ORC unwinder
>   entries, because this code is outside of any ELF function.
> 
> This behavior was implemented as an optimization in LLVM 5 years ago,
> but it's not the first time this has caused issues for objtool.  A patch
> has been authored against LLVM to revert the behavior, which may or may
> not be accepted.  Until then use a global symbol for the thunk that way
> objtool can generate proper unwind info here with LLVM_IAS=1.

As Fangrui pointed out, the section symbol stripping is useful for when
there are a ton of sections like '-ffunction-sections' and
'-fdata-sections'.  Maybe add that justification to the patch
description.

We can try to support it, though I suspect other tools may also end up
getting surprised.

> Cc: Fangrui Song <maskray@google.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1209
> Link: https://reviews.llvm.org/D93783
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Code looks familiar ;-)

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

-- 
Josh


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

* [PATCH v2] x86/entry: use STB_GLOBAL for register restoring thunk
  2020-12-24  4:55 ` Josh Poimboeuf
@ 2021-01-06  0:43   ` Nick Desaulniers
  2021-01-06  1:58     ` Josh Poimboeuf
  0 siblings, 1 reply; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-06  0:43 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Nick Desaulniers, Fangrui Song, Arnd Bergmann, Josh Poimboeuf,
	x86, H. Peter Anvin, Nathan Chancellor, linux-kernel,
	clang-built-linux

Arnd found a randconfig that produces the warning:

arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
offset 0x3e

when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
notes:

  With the LLVM assembler stripping the .text section symbol, objtool
  has no way to reference this code when it generates ORC unwinder
  entries, because this code is outside of any ELF function.

Fangrui notes that this is helpful for reducing images size when
compiling with -ffunction-sections and -fdata-sections. I have observerd
on the order of tens of thousands of symbols for the kernel images built
with those flags. A patch has been authored against GNU binutils to
match this behavior, with a new flag
--generate-unused-section-symbols=[yes|no].

Use a global symbol for the thunk that way
objtool can generate proper unwind info here with LLVM_IAS=1.

Cc: Fangrui Song <maskray@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v1 -> v2:
* Pick up Josh's Ack.
* Add commit message info about -ffunction-sections/-fdata-sections, and
  link to binutils patch.

 arch/x86/entry/thunk_64.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..878816034a73 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
-- 
2.29.2.729.g45daf8777d-goog


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

* Re: [PATCH v2] x86/entry: use STB_GLOBAL for register restoring thunk
  2021-01-06  0:43   ` [PATCH v2] " Nick Desaulniers
@ 2021-01-06  1:58     ` Josh Poimboeuf
  2021-01-11 20:38       ` [PATCH v3] x86/entry: emit a symbol " Nick Desaulniers
  0 siblings, 1 reply; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-06  1:58 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Fangrui Song, Arnd Bergmann, x86, H. Peter Anvin,
	Nathan Chancellor, linux-kernel, clang-built-linux

On Tue, Jan 05, 2021 at 04:43:51PM -0800, Nick Desaulniers wrote:
> Arnd found a randconfig that produces the warning:
> 
> arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> offset 0x3e
> 
> when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler stripping the .text section symbol, objtool
>   has no way to reference this code when it generates ORC unwinder
>   entries, because this code is outside of any ELF function.
> 
> Fangrui notes that this is helpful for reducing images size when
> compiling with -ffunction-sections and -fdata-sections. I have observerd
> on the order of tens of thousands of symbols for the kernel images built
> with those flags. A patch has been authored against GNU binutils to
> match this behavior, with a new flag
> --generate-unused-section-symbols=[yes|no].
> 
> Use a global symbol for the thunk that way
> objtool can generate proper unwind info here with LLVM_IAS=1.

On second thought, there's no need to make the symbol global.  Just
getting rid of the '.L' local label symbol prefix should be enough to
make an ELF symbol:

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..c9a9fbf1655f 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif


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

* [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-06  1:58     ` Josh Poimboeuf
@ 2021-01-11 20:38       ` Nick Desaulniers
  2021-01-11 20:58         ` Fangrui Song
                           ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-11 20:38 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Nick Desaulniers, Fangrui Song, Arnd Bergmann, Josh Poimboeuf,
	x86, H. Peter Anvin, Nathan Chancellor, linux-kernel,
	clang-built-linux

Arnd found a randconfig that produces the warning:

arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
offset 0x3e

when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
notes:

  With the LLVM assembler stripping the .text section symbol, objtool
  has no way to reference this code when it generates ORC unwinder
  entries, because this code is outside of any ELF function.

Fangrui notes that this optimization is helpful for reducing images size
when compiling with -ffunction-sections and -fdata-sections. I have
observerd on the order of tens of thousands of symbols for the kernel
images built with those flags. A patch has been authored against GNU
binutils to match this behavior, with a new flag
--generate-unused-section-symbols=[yes|no].

We can omit the .L prefix on a label to emit an entry into the symbol
table for the label, with STB_LOCAL binding.  This enables objtool to
generate proper unwind info here with LLVM_IAS=1.

Cc: Fangrui Song <maskray@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v2 -> v3:
* rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
  as per Josh.
* rename oneline to drop STB_GLOBAL in commit message.
* add link to GAS docs on .L prefix.
* drop Josh's ack since patch changed.

Changes v1 -> v2:
* Pick up Josh's Ack.
* Add commit message info about -ffunction-sections/-fdata-sections, and
  link to binutils patch.


 arch/x86/entry/thunk_64.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..c9a9fbf1655f 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
-- 
2.30.0.284.gd98b1dd5eaa7-goog


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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-11 20:38       ` [PATCH v3] x86/entry: emit a symbol " Nick Desaulniers
@ 2021-01-11 20:58         ` Fangrui Song
  2021-01-11 22:09           ` Josh Poimboeuf
  2021-01-11 22:12         ` Josh Poimboeuf
  2021-01-12  0:38         ` Borislav Petkov
  2 siblings, 1 reply; 33+ messages in thread
From: Fangrui Song @ 2021-01-11 20:58 UTC (permalink / raw)
  To: Nick Desaulniers, Josh Poimboeuf
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Arnd Bergmann, x86, H. Peter Anvin, Nathan Chancellor,
	linux-kernel, clang-built-linux


On 2021-01-11, Nick Desaulniers wrote:
>Arnd found a randconfig that produces the warning:
>
>arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
>offset 0x3e
>
>when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
>notes:
>
>  With the LLVM assembler stripping the .text section symbol, objtool
>  has no way to reference this code when it generates ORC unwinder
>  entries, because this code is outside of any ELF function.
>
>Fangrui notes that this optimization is helpful for reducing images size
>when compiling with -ffunction-sections and -fdata-sections. I have
>observerd on the order of tens of thousands of symbols for the kernel
>images built with those flags. A patch has been authored against GNU
>binutils to match this behavior, with a new flag
>--generate-unused-section-symbols=[yes|no].

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d1bcae833b32f1408485ce69f844dcd7ded093a8
has been committed. The patch should be included in binutils 2.37.
The maintainers are welcome to the idea, but fixing all the arch-specific tests is tricky.

H.J. fixed the x86 tests and enabled this for x86. When binutils 2.37
come out, some other architectures may follow as well.

>We can omit the .L prefix on a label to emit an entry into the symbol
>table for the label, with STB_LOCAL binding.  This enables objtool to
>generate proper unwind info here with LLVM_IAS=1.

Josh, I think objtool orc generate needs to synthesize STT_SECTION
symbols even if they do not exist in object files.

rg 'SYM_CODE.*\.L' reveals a few other .S files which may have similar problems.

>Cc: Fangrui Song <maskray@google.com>
>Link: https://github.com/ClangBuiltLinux/linux/issues/1209
>Link: https://reviews.llvm.org/D93783
>Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
>Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
>Reported-by: Arnd Bergmann <arnd@arndb.de>
>Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
>Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>---
>Changes v2 -> v3:
>* rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
>  as per Josh.
>* rename oneline to drop STB_GLOBAL in commit message.
>* add link to GAS docs on .L prefix.
>* drop Josh's ack since patch changed.
>
>Changes v1 -> v2:
>* Pick up Josh's Ack.
>* Add commit message info about -ffunction-sections/-fdata-sections, and
>  link to binutils patch.
>
>
> arch/x86/entry/thunk_64.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
>index ccd32877a3c4..c9a9fbf1655f 100644
>--- a/arch/x86/entry/thunk_64.S
>+++ b/arch/x86/entry/thunk_64.S
>@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
> 	.endif
>
> 	call \func
>-	jmp  .L_restore
>+	jmp  __thunk_restore
> SYM_FUNC_END(\name)
> 	_ASM_NOKPROBE(\name)
> 	.endm
>@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
> #endif
>
> #ifdef CONFIG_PREEMPTION
>-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
>+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
> 	popq %r11
> 	popq %r10
> 	popq %r9
>@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
> 	popq %rdi
> 	popq %rbp
> 	ret
>-	_ASM_NOKPROBE(.L_restore)
>-SYM_CODE_END(.L_restore)
>+	_ASM_NOKPROBE(__thunk_restore)
>+SYM_CODE_END(__thunk_restore)
> #endif
>-- 
>2.30.0.284.gd98b1dd5eaa7-goog
>

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-11 20:58         ` Fangrui Song
@ 2021-01-11 22:09           ` Josh Poimboeuf
  2021-01-11 22:16             ` Fāng-ruì Sòng
  0 siblings, 1 reply; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-11 22:09 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Arnd Bergmann, x86, H. Peter Anvin,
	Nathan Chancellor, linux-kernel, clang-built-linux

On Mon, Jan 11, 2021 at 12:58:14PM -0800, Fangrui Song wrote:
> On 2021-01-11, Nick Desaulniers wrote:
> > Arnd found a randconfig that produces the warning:
> > 
> > arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> > offset 0x3e
> > 
> > when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> > notes:
> > 
> >  With the LLVM assembler stripping the .text section symbol, objtool
> >  has no way to reference this code when it generates ORC unwinder
> >  entries, because this code is outside of any ELF function.
> > 
> > Fangrui notes that this optimization is helpful for reducing images size
> > when compiling with -ffunction-sections and -fdata-sections. I have
> > observerd on the order of tens of thousands of symbols for the kernel
> > images built with those flags. A patch has been authored against GNU
> > binutils to match this behavior, with a new flag
> > --generate-unused-section-symbols=[yes|no].
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d1bcae833b32f1408485ce69f844dcd7ded093a8
> has been committed. The patch should be included in binutils 2.37.
> The maintainers are welcome to the idea, but fixing all the arch-specific tests is tricky.
> 
> H.J. fixed the x86 tests and enabled this for x86. When binutils 2.37
> come out, some other architectures may follow as well.
> 
> > We can omit the .L prefix on a label to emit an entry into the symbol
> > table for the label, with STB_LOCAL binding.  This enables objtool to
> > generate proper unwind info here with LLVM_IAS=1.
> 
> Josh, I think objtool orc generate needs to synthesize STT_SECTION
> symbols even if they do not exist in object files.

I'm guessing you don't mean re-adding *all* missing STT_SECTIONs, as
that would just be undoing these new assembler features.

We could re-add STT_SECTION only when there's no other corresponding
symbol associated with the code, but then objtool would have to start
updating the symbol table (which right now it manages to completely
avoid).  But that would only be for the niche cases, like
'SYM_CODE.*\.L' as you mentioned.

I'd rather avoid making doing something so pervasive for such a small
number of edge cases.  It's hopefully easier and more robust to just say
"all code must be associated with a symbol".  I suspect we're already
~99.99% there anyway.

  $ git grep -e 'SYM_CODE.*\.L'
  arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
  arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
  arch/x86/entry/thunk_64.S:SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
  arch/x86/entry/thunk_64.S:SYM_CODE_END(.L_restore)
  arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
  arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
  arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
  arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
  arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
  arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
  arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
  arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)

Alternatively, the assemblers could add an option to only strip
-ffunction-sections and -fdata-sections STT_SECTION symbols, e.g. leave
".text" and friends alone.

-- 
Josh


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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-11 20:38       ` [PATCH v3] x86/entry: emit a symbol " Nick Desaulniers
  2021-01-11 20:58         ` Fangrui Song
@ 2021-01-11 22:12         ` Josh Poimboeuf
  2021-01-12  0:38         ` Borislav Petkov
  2 siblings, 0 replies; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-11 22:12 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Fangrui Song, Arnd Bergmann, x86, H. Peter Anvin,
	Nathan Chancellor, linux-kernel, clang-built-linux

On Mon, Jan 11, 2021 at 12:38:06PM -0800, Nick Desaulniers wrote:
> Arnd found a randconfig that produces the warning:
> 
> arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> offset 0x3e
> 
> when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler stripping the .text section symbol, objtool
>   has no way to reference this code when it generates ORC unwinder
>   entries, because this code is outside of any ELF function.
> 
> Fangrui notes that this optimization is helpful for reducing images size

"image"

> when compiling with -ffunction-sections and -fdata-sections. I have
> observerd on the order of tens of thousands of symbols for the kernel

"observed"

> images built with those flags. A patch has been authored against GNU
> binutils to match this behavior, with a new flag
> --generate-unused-section-symbols=[yes|no].
> 
> We can omit the .L prefix on a label to emit an entry into the symbol
> table for the label, with STB_LOCAL binding.  This enables objtool to
> generate proper unwind info here with LLVM_IAS=1.
> 
> Cc: Fangrui Song <maskray@google.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1209
> Link: https://reviews.llvm.org/D93783
> Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
> Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes v2 -> v3:
> * rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
>   as per Josh.
> * rename oneline to drop STB_GLOBAL in commit message.
> * add link to GAS docs on .L prefix.
> * drop Josh's ack since patch changed.

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

-- 
Josh


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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-11 22:09           ` Josh Poimboeuf
@ 2021-01-11 22:16             ` Fāng-ruì Sòng
  0 siblings, 0 replies; 33+ messages in thread
From: Fāng-ruì Sòng @ 2021-01-11 22:16 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Arnd Bergmann, X86 ML, H. Peter Anvin,
	Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 2:09 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Jan 11, 2021 at 12:58:14PM -0800, Fangrui Song wrote:
> > On 2021-01-11, Nick Desaulniers wrote:
> > > Arnd found a randconfig that produces the warning:
> > >
> > > arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> > > offset 0x3e
> > >
> > > when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> > > notes:
> > >
> > >  With the LLVM assembler stripping the .text section symbol, objtool
> > >  has no way to reference this code when it generates ORC unwinder
> > >  entries, because this code is outside of any ELF function.
> > >
> > > Fangrui notes that this optimization is helpful for reducing images size
> > > when compiling with -ffunction-sections and -fdata-sections. I have
> > > observerd on the order of tens of thousands of symbols for the kernel
> > > images built with those flags. A patch has been authored against GNU
> > > binutils to match this behavior, with a new flag
> > > --generate-unused-section-symbols=[yes|no].
> >
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d1bcae833b32f1408485ce69f844dcd7ded093a8
> > has been committed. The patch should be included in binutils 2.37.
> > The maintainers are welcome to the idea, but fixing all the arch-specific tests is tricky.
> >
> > H.J. fixed the x86 tests and enabled this for x86. When binutils 2.37
> > come out, some other architectures may follow as well.
> >
> > > We can omit the .L prefix on a label to emit an entry into the symbol
> > > table for the label, with STB_LOCAL binding.  This enables objtool to
> > > generate proper unwind info here with LLVM_IAS=1.
> >
> > Josh, I think objtool orc generate needs to synthesize STT_SECTION
> > symbols even if they do not exist in object files.
>
> I'm guessing you don't mean re-adding *all* missing STT_SECTIONs, as
> that would just be undoing these new assembler features.
>
> We could re-add STT_SECTION only when there's no other corresponding
> symbol associated with the code, but then objtool would have to start
> updating the symbol table (which right now it manages to completely
> avoid).  But that would only be for the niche cases, like
> 'SYM_CODE.*\.L' as you mentioned.
>
> I'd rather avoid making doing something so pervasive for such a small
> number of edge cases.  It's hopefully easier and more robust to just say
> "all code must be associated with a symbol".  I suspect we're already
> ~99.99% there anyway.
>
>   $ git grep -e 'SYM_CODE.*\.L'
>   arch/x86/entry/entry_64.S:SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
>   arch/x86/entry/entry_64.S:SYM_CODE_END(.Lbad_gs)
>   arch/x86/entry/thunk_64.S:SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
>   arch/x86/entry/thunk_64.S:SYM_CODE_END(.L_restore)
>   arch/x86/lib/copy_user_64.S:SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
>   arch/x86/lib/copy_user_64.S:SYM_CODE_END(.Lcopy_user_handle_tail)
>   arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
>   arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_clac)
>   arch/x86/lib/getuser.S:SYM_CODE_START_LOCAL(.Lbad_get_user_8_clac)
>   arch/x86/lib/getuser.S:SYM_CODE_END(.Lbad_get_user_8_clac)
>   arch/x86/lib/putuser.S:SYM_CODE_START_LOCAL(.Lbad_put_user_clac)
>   arch/x86/lib/putuser.S:SYM_CODE_END(.Lbad_put_user_clac)

I'd prefer that the assembly can continue using .L and does not know
the objtool limitation.
Assemblers normally drop .L symbols. These symbols are otherwise not useful.

However, if as you said, teaching objtool about synthesizing
STT_SECTION from section header table is difficult,
this patch looks fine to me.

Reviewed-by: Fangrui Song <maskray@google.com>

> Alternatively, the assemblers could add an option to only strip
> -ffunction-sections and -fdata-sections STT_SECTION symbols, e.g. leave
> ".text" and friends alone.

I forgot to mention that --generate-unused-section-symbols=[yes|no] is
not added to GNU as.
Making the assembler behavior dependent on -ffunction-sections is not
an option in both LLVM integrated assembler and GNU as.

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-11 20:38       ` [PATCH v3] x86/entry: emit a symbol " Nick Desaulniers
  2021-01-11 20:58         ` Fangrui Song
  2021-01-11 22:12         ` Josh Poimboeuf
@ 2021-01-12  0:38         ` Borislav Petkov
  2021-01-12  0:41           ` Fāng-ruì Sòng
  2 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2021-01-12  0:38 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Fangrui Song,
	Arnd Bergmann, Josh Poimboeuf, x86, H. Peter Anvin,
	Nathan Chancellor, linux-kernel, clang-built-linux

On Mon, Jan 11, 2021 at 12:38:06PM -0800, Nick Desaulniers wrote:
> Arnd found a randconfig that produces the warning:
> 
> arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> offset 0x3e
> 
> when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler stripping the .text section symbol, objtool
>   has no way to reference this code when it generates ORC unwinder
>   entries, because this code is outside of any ELF function.
> 
> Fangrui notes that this optimization is helpful for reducing images size
> when compiling with -ffunction-sections and -fdata-sections. I have
> observerd on the order of tens of thousands of symbols for the kernel
> images built with those flags. A patch has been authored against GNU
> binutils to match this behavior, with a new flag
> --generate-unused-section-symbols=[yes|no].
> 
> We can omit the .L prefix on a label to emit an entry into the symbol
> table for the label, with STB_LOCAL binding.  This enables objtool to
> generate proper unwind info here with LLVM_IAS=1.
> 
> Cc: Fangrui Song <maskray@google.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1209
> Link: https://reviews.llvm.org/D93783
> Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
> Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes v2 -> v3:
> * rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
>   as per Josh.

Ok so I read a bit around those links above...

Are you trying to tell me here that we can't use .L-prefixed local
labels anymore because, well, clang's assembler is way too overzealous
when stripping symbols to save whopping KiBs of memory?!

Btw Josh made sense to me when asking for a flag or so to keep .text.

And I see --generate-unused-section-symbols=[yes|no] for binutils.

So why isn't there a patch using that switch on clang too instead of the
kernel having to dance yet again for some tool?

:-\

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  0:38         ` Borislav Petkov
@ 2021-01-12  0:41           ` Fāng-ruì Sòng
  2021-01-12  1:00             ` Borislav Petkov
  0 siblings, 1 reply; 33+ messages in thread
From: Fāng-ruì Sòng @ 2021-01-12  0:41 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Arnd Bergmann, Josh Poimboeuf, X86 ML, H. Peter Anvin,
	Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 4:38 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Jan 11, 2021 at 12:38:06PM -0800, Nick Desaulniers wrote:
> > Arnd found a randconfig that produces the warning:
> >
> > arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
> > offset 0x3e
> >
> > when building with LLVM_IAS=1 (use Clang's integrated assembler). Josh
> > notes:
> >
> >   With the LLVM assembler stripping the .text section symbol, objtool
> >   has no way to reference this code when it generates ORC unwinder
> >   entries, because this code is outside of any ELF function.
> >
> > Fangrui notes that this optimization is helpful for reducing images size
> > when compiling with -ffunction-sections and -fdata-sections. I have
> > observerd on the order of tens of thousands of symbols for the kernel
> > images built with those flags. A patch has been authored against GNU
> > binutils to match this behavior, with a new flag
> > --generate-unused-section-symbols=[yes|no].
> >
> > We can omit the .L prefix on a label to emit an entry into the symbol
> > table for the label, with STB_LOCAL binding.  This enables objtool to
> > generate proper unwind info here with LLVM_IAS=1.
> >
> > Cc: Fangrui Song <maskray@google.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1209
> > Link: https://reviews.llvm.org/D93783
> > Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
> > Link: https://sourceware.org/pipermail/binutils/2020-December/114671.html
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> > Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > Changes v2 -> v3:
> > * rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
> >   as per Josh.
>
> Ok so I read a bit around those links above...
>
> Are you trying to tell me here that we can't use .L-prefixed local
> labels anymore because, well, clang's assembler is way too overzealous
> when stripping symbols to save whopping KiBs of memory?!

To be fair: we cannot use .L-prefixed local because of the objtool limitation.
The LLVM integrated assembler behavior is a good one and binutils
global maintainers have agreed so H.J. went ahead and implemented it
for GNU as x86.

> Btw Josh made sense to me when asking for a flag or so to keep .text.
>
> And I see --generate-unused-section-symbols=[yes|no] for binutils.
>
> So why isn't there a patch using that switch on clang too instead of the
> kernel having to dance yet again for some tool?

--generate-unused-section-symbols=[yes|no] as an assembler option has
been rejected.


> :-\
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  0:41           ` Fāng-ruì Sòng
@ 2021-01-12  1:00             ` Borislav Petkov
  2021-01-12  1:13               ` Nick Desaulniers
  0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2021-01-12  1:00 UTC (permalink / raw)
  To: Fāng-ruì Sòng
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Arnd Bergmann, Josh Poimboeuf, X86 ML, H. Peter Anvin,
	Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 04:41:52PM -0800, Fāng-ruì Sòng wrote:
> To be fair: we cannot use

Who's "we"?

> .L-prefixed local because of the objtool limitation.

What objtool limitation? I thought clang's assembler removes .text which
objtool uses. It worked fine with GNU as so far.

> The LLVM integrated assembler behavior is a good one

Please explain what "good one" means in that particular context.

> and binutils global maintainers have agreed so H.J. went ahead and
> implemented it for GNU as x86.

But they don't break old behavior, do they? Or are they removing .text
unconditionally now too?

> --generate-unused-section-symbols=[yes|no] as an assembler option has
> been rejected.

Meaning what exactly? There's no way for clang's integrated assembler to
even get a cmdline option to not strip .text?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  1:00             ` Borislav Petkov
@ 2021-01-12  1:13               ` Nick Desaulniers
  2021-01-12  1:59                 ` Josh Poimboeuf
  2021-01-12 11:47                 ` [PATCH v3] x86/entry: emit " Borislav Petkov
  0 siblings, 2 replies; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-12  1:13 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Fāng-ruì Sòng, Andy Lutomirski, Thomas Gleixner,
	Ingo Molnar, Arnd Bergmann, Josh Poimboeuf, X86 ML,
	H. Peter Anvin, Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 5:00 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Jan 11, 2021 at 04:41:52PM -0800, Fāng-ruì Sòng wrote:
> > To be fair: we cannot use
>
> Who's "we"?
>
> > .L-prefixed local because of the objtool limitation.
>
> What objtool limitation? I thought clang's assembler removes .text which
> objtool uses. It worked fine with GNU as so far.

I don't think we need to completely stop using .L prefixes in the
kernel, just this one location since tracking the control flow seems a
little tricky for objtool. Maybe Josh can clarify more if needed?

>
> > The LLVM integrated assembler behavior is a good one
>
> Please explain what "good one" means in that particular context.
>
> > and binutils global maintainers have agreed so H.J. went ahead and
> > implemented it for GNU as x86.
>
> But they don't break old behavior, do they? Or are they removing .text
> unconditionally now too?

Unconditionally. See
https://sourceware.org/pipermail/binutils/2021-January/114700.html
where that flag was rejected and the optimization was adopted as the
optimization was obvious to GNU binutils developers. So I suspect this
will become a problem for GNU binutils users as well after the latest
release that contains
https://sourceware.org/pipermail/binutils/attachments/20210105/75dd4a9d/attachment-0001.bin.

> > --generate-unused-section-symbols=[yes|no] as an assembler option has
> > been rejected.
>
> Meaning what exactly? There's no way for clang's integrated assembler to
> even get a cmdline option to not strip .text?

I can clean that up in v5; The section symbols were not generated then
stripped; they were simply never generated.
-- 
Thanks,
~Nick Desaulniers

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  1:13               ` Nick Desaulniers
@ 2021-01-12  1:59                 ` Josh Poimboeuf
  2021-01-12 11:54                   ` Borislav Petkov
  2021-01-12 11:47                 ` [PATCH v3] x86/entry: emit " Borislav Petkov
  1 sibling, 1 reply; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-12  1:59 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Borislav Petkov, Fāng-ruì Sòng, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Arnd Bergmann, X86 ML,
	H. Peter Anvin, Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 05:13:16PM -0800, Nick Desaulniers wrote:
> On Mon, Jan 11, 2021 at 5:00 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Mon, Jan 11, 2021 at 04:41:52PM -0800, Fāng-ruì Sòng wrote:
> > > To be fair: we cannot use
> >
> > Who's "we"?
> >
> > > .L-prefixed local because of the objtool limitation.
> >
> > What objtool limitation? I thought clang's assembler removes .text which
> > objtool uses. It worked fine with GNU as so far.
> 
> I don't think we need to completely stop using .L prefixes in the
> kernel, just this one location since tracking the control flow seems a
> little tricky for objtool. Maybe Josh can clarify more if needed?

Right.  In the vast majority of cases, .L symbols are totally fine.

The limitation now being imposed by objtool (due to these assembler
changes) is that all code must be contained in an ELF symbol.  And .L
symbols don't create such symbols.

So basically, you can use an .L symbol *inside* a function or a code
segment, you just can't use the .L symbol to contain the code using a
SYM_*_START/END annotation pair.

It only affects a tiny fraction of all .L usage.  Just a handful of code
sites I think.

-- 
Josh


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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  1:13               ` Nick Desaulniers
  2021-01-12  1:59                 ` Josh Poimboeuf
@ 2021-01-12 11:47                 ` Borislav Petkov
  1 sibling, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2021-01-12 11:47 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Fāng-ruì Sòng, Andy Lutomirski, Thomas Gleixner,
	Ingo Molnar, Arnd Bergmann, Josh Poimboeuf, X86 ML,
	H. Peter Anvin, Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 05:13:16PM -0800, Nick Desaulniers wrote:
> Unconditionally. See
> https://sourceware.org/pipermail/binutils/2021-January/114700.html
> where that flag was rejected and the optimization was adopted as the
> optimization was obvious to GNU binutils developers. So I suspect this
> will become a problem for GNU binutils users as well after the latest
> release that contains
> https://sourceware.org/pipermail/binutils/attachments/20210105/75dd4a9d/attachment-0001.bin.

Aha, thanks for this.

> I can clean that up in v5; The section symbols were not generated then
> stripped; they were simply never generated.

I'd appreciate a more verbose writeup explaining why this is being done,
but written for outsiders who are not necessarily toolchain developers.
So that it is clear months/years from now why this was done. Something
structured like this maybe:

  Problem is A.

  It happens because of B.

  Fix it by doing C.

  (Potentially do D).

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH v3] x86/entry: emit a symbol for register restoring thunk
  2021-01-12  1:59                 ` Josh Poimboeuf
@ 2021-01-12 11:54                   ` Borislav Petkov
  2021-01-12 19:46                     ` [PATCH v4] " Nick Desaulniers
  0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2021-01-12 11:54 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Nick Desaulniers, Fāng-ruì Sòng, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Arnd Bergmann, X86 ML,
	H. Peter Anvin, Nathan Chancellor, LKML, clang-built-linux

On Mon, Jan 11, 2021 at 07:59:52PM -0600, Josh Poimboeuf wrote:
> Right.  In the vast majority of cases, .L symbols are totally fine.
> 
> The limitation now being imposed by objtool (due to these assembler
> changes) is that all code must be contained in an ELF symbol.  And .L
> symbols don't create such symbols.
> 
> So basically, you can use an .L symbol *inside* a function or a code
> segment, you just can't use the .L symbol to contain the code using a
> SYM_*_START/END annotation pair.
> 
> It only affects a tiny fraction of all .L usage.  Just a handful of code
> sites I think.

@Nick, this belongs into the commit message too pls.

Also,

Documentation/asm-annotations.rst
include/linux/linkage.h

would need some of that blurb added explaining to users *why* they
should not use .L local symbols as SYM_* macro arguments.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-12 11:54                   ` Borislav Petkov
@ 2021-01-12 19:46                     ` Nick Desaulniers
  2021-01-12 21:01                       ` Mark Brown
                                         ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-12 19:46 UTC (permalink / raw)
  To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Nick Desaulniers, Fangrui Song, Arnd Bergmann, Josh Poimboeuf,
	Jonathan Corbet, x86, H. Peter Anvin, Nathan Chancellor,
	Mark Brown, Miguel Ojeda, Jiri Slaby, Joe Perches, linux-doc,
	linux-kernel, clang-built-linux

Arnd found a randconfig that produces the warning:

arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
offset 0x3e

when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
notes:

  With the LLVM assembler not generating section symbols, objtool has no
  way to reference this code when it generates ORC unwinder entries,
  because this code is outside of any ELF function.

  The limitation now being imposed by objtool is that all code must be
  contained in an ELF symbol.  And .L symbols don't create such symbols.

  So basically, you can use an .L symbol *inside* a function or a code
  segment, you just can't use the .L symbol to contain the code using a
  SYM_*_START/END annotation pair.

Fangrui notes that this optimization is helpful for reducing image size
when compiling with -ffunction-sections and -fdata-sections. I have
observed on the order of tens of thousands of symbols for the kernel
images built with those flags.

A patch has been authored against GNU binutils to match this behavior,
so this will also become a problem for users of GNU binutils once they
upgrade to 2.36.

We can omit the .L prefix on a label so that the assembler will emit an
entry into the symbol table for the label, with STB_LOCAL binding.  This
enables objtool to generate proper unwind info here with LLVM_IAS=1 or
GNU binutils 2.36+.

Cc: Fangrui Song <maskray@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d1bcae833b32f1408485ce69f844dcd7ded093a8
Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v3 -> v4:
* Add changes to Documentation/ and include/ as per Boris.
* Fix typos as per Josh.
* Replace link and note in commit message about
  --generate-unused-section-symbols=[yes|no] which was dropped from GNU
  binutils with link to actual commit in binutils-gdb.
* Add additional notes from Josh in commit message.
* Slightly reword commit message to indicate that section symbols are
  not emitted, rather than stripped.

Changes v2 -> v3:
* rework to use STB_LOCAL rather than STB_GLOBAL by dropping .L prefix,
  as per Josh.
* rename oneline to drop STB_GLOBAL in commit message.
* add link to GAS docs on .L prefix.
* drop Josh's ack since patch changed.

Changes v1 -> v2:
* Pick up Josh's Ack.
* Add commit message info about -ffunction-sections/-fdata-sections, and
  link to binutils patch.

 Documentation/asm-annotations.rst | 9 +++++++++
 arch/x86/entry/thunk_64.S         | 8 ++++----
 include/linux/linkage.h           | 5 ++++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index 32ea57483378..e711ff98102a 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -153,6 +153,15 @@ This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
   To some extent, this category corresponds to deprecated ``ENTRY`` and
   ``END``. Except ``END`` had several other meanings too.
 
+  Developers should avoid using local symbol names that are prefixed with
+  ``.L``, as this has special meaning for the assembler; a symbol entry will
+  not be emitted into the symbol table. This can prevent ``objtool`` from
+  generating correct unwind info. Symbols with STB_LOCAL binding may still be
+  used, and ``.L`` prefixed local symbol names are still generally useable
+  within a function, but ``.L`` prefixed local symbol names should not be used
+  to denote the beginning or end of code regions via
+  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
+
 * ``SYM_INNER_LABEL*`` is used to denote a label inside some
   ``SYM_{CODE,FUNC}_START`` and ``SYM_{CODE,FUNC}_END``.  They are very similar
   to C labels, except they can be made global. An example of use::
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..c9a9fbf1655f 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index 5bcfbd972e97..11537ba9f512 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -270,7 +270,10 @@
 	SYM_END(name, SYM_T_FUNC)
 #endif
 
-/* SYM_CODE_START -- use for non-C (special) functions */
+/*
+ * SYM_CODE_START -- use for non-C (special) functions, avoid .L prefixed local
+ * symbol names which may not emit a symbol table entry.
+ */
 #ifndef SYM_CODE_START
 #define SYM_CODE_START(name)				\
 	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)
-- 
2.30.0.284.gd98b1dd5eaa7-goog


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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-12 19:46                     ` [PATCH v4] " Nick Desaulniers
@ 2021-01-12 21:01                       ` Mark Brown
  2021-01-13 16:59                         ` Josh Poimboeuf
  2021-01-14 11:49                         ` [Linux-kernel-mentees] Fwd: " Lukas Bulwahn
  2021-01-13 11:52                       ` [tip: x86/entry] x86/entry: Emit " tip-bot2 for Nick Desaulniers
  2021-01-19 10:12                       ` tip-bot2 for Nick Desaulniers
  2 siblings, 2 replies; 33+ messages in thread
From: Mark Brown @ 2021-01-12 21:01 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Fangrui Song, Arnd Bergmann, Josh Poimboeuf, Jonathan Corbet,
	x86, H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, linux-doc, linux-kernel, clang-built-linux

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

On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:

This:

> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> notes:

>   So basically, you can use an .L symbol *inside* a function or a code
>   segment, you just can't use the .L symbol to contain the code using a
>   SYM_*_START/END annotation pair.

is a stronger statement than this:

> +  Developers should avoid using local symbol names that are prefixed with
> +  ``.L``, as this has special meaning for the assembler; a symbol entry will
> +  not be emitted into the symbol table. This can prevent ``objtool`` from
> +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
> +  used, and ``.L`` prefixed local symbol names are still generally useable
> +  within a function, but ``.L`` prefixed local symbol names should not be used
> +  to denote the beginning or end of code regions via
> +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.

and seems more what I'd expect - SYM_FUNC* is also affected for example.
Even though other usages are probably not very likely it seems better to
keep the stronger statement in case someone comes up with one, and to
stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
affected.

This also looks like a good candiate for a checkpatch rule, but that can
be done separately of course.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* [tip: x86/entry] x86/entry: Emit a symbol for register restoring thunk
  2021-01-12 19:46                     ` [PATCH v4] " Nick Desaulniers
  2021-01-12 21:01                       ` Mark Brown
@ 2021-01-13 11:52                       ` tip-bot2 for Nick Desaulniers
  2021-01-19 10:12                       ` tip-bot2 for Nick Desaulniers
  2 siblings, 0 replies; 33+ messages in thread
From: tip-bot2 for Nick Desaulniers @ 2021-01-13 11:52 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Arnd Bergmann, Josh Poimboeuf, Borislav Petkov, Nick Desaulniers,
	Borislav Petkov, x86, linux-kernel

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

Commit-ID:     bde718b7e154afc99e1956b18a848401ce8e1f8e
Gitweb:        https://git.kernel.org/tip/bde718b7e154afc99e1956b18a848401ce8e1f8e
Author:        Nick Desaulniers <ndesaulniers@google.com>
AuthorDate:    Tue, 12 Jan 2021 11:46:24 -08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Wed, 13 Jan 2021 12:23:57 +01:00

x86/entry: Emit a symbol for register restoring thunk

Arnd found a randconfig that produces the warning:

  arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
  offset 0x3e

when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
notes:

  With the LLVM assembler not generating section symbols, objtool has no
  way to reference this code when it generates ORC unwinder entries,
  because this code is outside of any ELF function.

  The limitation now being imposed by objtool is that all code must be
  contained in an ELF symbol.  And .L symbols don't create such symbols.

  So basically, you can use an .L symbol *inside* a function or a code
  segment, you just can't use the .L symbol to contain the code using a
  SYM_*_START/END annotation pair.

Fangrui notes that this optimization is helpful for reducing image size
when compiling with -ffunction-sections and -fdata-sections. I have
observed on the order of tens of thousands of symbols for the kernel
images built with those flags.

A patch has been authored against GNU binutils to match this behavior
of not generating unused section symbols ([1]), so this will
also become a problem for users of GNU binutils once they upgrade to 2.36.

Omit the .L prefix on a label so that the assembler will emit an entry
into the symbol table for the label, with STB_LOCAL binding. This
enables objtool to generate proper unwind info here with LLVM_IAS=1 or
GNU binutils 2.36+.

 [ bp: Massage commit message. ]

Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210112194625.4181814-1-ndesaulniers@google.com
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d1bcae833b32f1408485ce69f844dcd7ded093a8 [1]
---
 Documentation/asm-annotations.rst |  9 +++++++++
 arch/x86/entry/thunk_64.S         |  8 ++++----
 include/linux/linkage.h           |  5 ++++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index 32ea574..e711ff9 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -153,6 +153,15 @@ This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
   To some extent, this category corresponds to deprecated ``ENTRY`` and
   ``END``. Except ``END`` had several other meanings too.
 
+  Developers should avoid using local symbol names that are prefixed with
+  ``.L``, as this has special meaning for the assembler; a symbol entry will
+  not be emitted into the symbol table. This can prevent ``objtool`` from
+  generating correct unwind info. Symbols with STB_LOCAL binding may still be
+  used, and ``.L`` prefixed local symbol names are still generally useable
+  within a function, but ``.L`` prefixed local symbol names should not be used
+  to denote the beginning or end of code regions via
+  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
+
 * ``SYM_INNER_LABEL*`` is used to denote a label inside some
   ``SYM_{CODE,FUNC}_START`` and ``SYM_{CODE,FUNC}_END``.  They are very similar
   to C labels, except they can be made global. An example of use::
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd3287..c9a9fbf 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index 5bcfbd9..11537ba 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -270,7 +270,10 @@
 	SYM_END(name, SYM_T_FUNC)
 #endif
 
-/* SYM_CODE_START -- use for non-C (special) functions */
+/*
+ * SYM_CODE_START -- use for non-C (special) functions, avoid .L prefixed local
+ * symbol names which may not emit a symbol table entry.
+ */
 #ifndef SYM_CODE_START
 #define SYM_CODE_START(name)				\
 	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)

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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-12 21:01                       ` Mark Brown
@ 2021-01-13 16:59                         ` Josh Poimboeuf
  2021-01-13 17:46                           ` [PATCH] Documentation: asm-annotation: clarify .L local symbol names Nick Desaulniers
  2021-01-13 17:56                           ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Nick Desaulniers
  2021-01-14 11:49                         ` [Linux-kernel-mentees] Fwd: " Lukas Bulwahn
  1 sibling, 2 replies; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-13 16:59 UTC (permalink / raw)
  To: Mark Brown
  Cc: Nick Desaulniers, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Fangrui Song, Arnd Bergmann, Jonathan Corbet,
	x86, H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, linux-doc, linux-kernel, clang-built-linux

On Tue, Jan 12, 2021 at 09:01:54PM +0000, Mark Brown wrote:
> On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:
> 
> This:
> 
> > when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> > notes:
> 
> >   So basically, you can use an .L symbol *inside* a function or a code
> >   segment, you just can't use the .L symbol to contain the code using a
> >   SYM_*_START/END annotation pair.
> 
> is a stronger statement than this:
> 
> > +  Developers should avoid using local symbol names that are prefixed with
> > +  ``.L``, as this has special meaning for the assembler; a symbol entry will
> > +  not be emitted into the symbol table. This can prevent ``objtool`` from
> > +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
> > +  used, and ``.L`` prefixed local symbol names are still generally useable
> > +  within a function, but ``.L`` prefixed local symbol names should not be used
> > +  to denote the beginning or end of code regions via
> > +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
> 
> and seems more what I'd expect - SYM_FUNC* is also affected for example.
> Even though other usages are probably not very likely it seems better to
> keep the stronger statement in case someone comes up with one, and to
> stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
> affected.

Agreed, I think the comment is misleading/wrong/unclear in multiple
ways.  In most cases the use of .L symbols is still fine.  What's no
longer fine is when they're used to contain code in any kind of
START/END pair.

> This also looks like a good candiate for a checkpatch rule, but that can
> be done separately of course.

I like the idea of a checkpatch rule.

-- 
Josh


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

* [PATCH] Documentation: asm-annotation: clarify .L local symbol names
  2021-01-13 16:59                         ` Josh Poimboeuf
@ 2021-01-13 17:46                           ` Nick Desaulniers
  2021-01-13 19:56                             ` Mark Brown
  2021-01-13 17:56                           ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Nick Desaulniers
  1 sibling, 1 reply; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-13 17:46 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Nick Desaulniers,
	Mark Brown, Josh Poimboeuf, Jonathan Corbet, linux-doc,
	linux-kernel

Use more precise language and move the text to a region in the docs to
show that this constraint is not just for SYM_CODE_START*.

Suggested-by: Mark Brown <broonie@kernel.org>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Documentation/asm-annotations.rst | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index e711ff98102a..76424e0431f4 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -100,6 +100,11 @@ Instruction Macros
 ~~~~~~~~~~~~~~~~~~
 This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
 
+``objtool`` requires that all code must be contained in an ELF symbol. Symbol
+names that have a ``.L`` prefix do not emit symbol table entries. ``.L``
+prefixed symbols can be used within a code region, but should be avoided for
+denoting a range of code via ``SYM_*_START/END`` annotations.
+
 * ``SYM_FUNC_START`` and ``SYM_FUNC_START_LOCAL`` are supposed to be **the
   most frequent markings**. They are used for functions with standard calling
   conventions -- global and local. Like in C, they both align the functions to
@@ -153,15 +158,6 @@ This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
   To some extent, this category corresponds to deprecated ``ENTRY`` and
   ``END``. Except ``END`` had several other meanings too.
 
-  Developers should avoid using local symbol names that are prefixed with
-  ``.L``, as this has special meaning for the assembler; a symbol entry will
-  not be emitted into the symbol table. This can prevent ``objtool`` from
-  generating correct unwind info. Symbols with STB_LOCAL binding may still be
-  used, and ``.L`` prefixed local symbol names are still generally useable
-  within a function, but ``.L`` prefixed local symbol names should not be used
-  to denote the beginning or end of code regions via
-  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
-
 * ``SYM_INNER_LABEL*`` is used to denote a label inside some
   ``SYM_{CODE,FUNC}_START`` and ``SYM_{CODE,FUNC}_END``.  They are very similar
   to C labels, except they can be made global. An example of use::
-- 
2.30.0.284.gd98b1dd5eaa7-goog


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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-13 16:59                         ` Josh Poimboeuf
  2021-01-13 17:46                           ` [PATCH] Documentation: asm-annotation: clarify .L local symbol names Nick Desaulniers
@ 2021-01-13 17:56                           ` Nick Desaulniers
  2021-01-14 10:39                             ` Borislav Petkov
  1 sibling, 1 reply; 33+ messages in thread
From: Nick Desaulniers @ 2021-01-13 17:56 UTC (permalink / raw)
  To: Josh Poimboeuf, Mark Brown
  Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Fangrui Song, Arnd Bergmann, Jonathan Corbet,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Wed, Jan 13, 2021 at 8:59 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Tue, Jan 12, 2021 at 09:01:54PM +0000, Mark Brown wrote:
> > On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:
> >
> > This:
> >
> > > when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> > > notes:
> >
> > >   So basically, you can use an .L symbol *inside* a function or a code
> > >   segment, you just can't use the .L symbol to contain the code using a
> > >   SYM_*_START/END annotation pair.
> >
> > is a stronger statement than this:
> >
> > > +  Developers should avoid using local symbol names that are prefixed with
> > > +  ``.L``, as this has special meaning for the assembler; a symbol entry will
> > > +  not be emitted into the symbol table. This can prevent ``objtool`` from
> > > +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
> > > +  used, and ``.L`` prefixed local symbol names are still generally useable
> > > +  within a function, but ``.L`` prefixed local symbol names should not be used
> > > +  to denote the beginning or end of code regions via
> > > +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
> >
> > and seems more what I'd expect - SYM_FUNC* is also affected for example.
> > Even though other usages are probably not very likely it seems better to
> > keep the stronger statement in case someone comes up with one, and to
> > stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
> > affected.
>
> Agreed, I think the comment is misleading/wrong/unclear in multiple
> ways.  In most cases the use of .L symbols is still fine.  What's no
> longer fine is when they're used to contain code in any kind of
> START/END pair.

Apologies, that was not my intention.  I've sent a follow up in
https://lore.kernel.org/lkml/20210113174620.958429-1-ndesaulniers@google.com/T/#u
since BP picked up v3 in tip x86/entry:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/entry&id=bde718b7e154afc99e1956b18a848401ce8e1f8e

-- 
Thanks,
~Nick Desaulniers

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

* Re: [PATCH] Documentation: asm-annotation: clarify .L local symbol names
  2021-01-13 17:46                           ` [PATCH] Documentation: asm-annotation: clarify .L local symbol names Nick Desaulniers
@ 2021-01-13 19:56                             ` Mark Brown
  0 siblings, 0 replies; 33+ messages in thread
From: Mark Brown @ 2021-01-13 19:56 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Borislav Petkov, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
	Josh Poimboeuf, Jonathan Corbet, linux-doc, linux-kernel

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

On Wed, Jan 13, 2021 at 09:46:20AM -0800, Nick Desaulniers wrote:
> Use more precise language and move the text to a region in the docs to
> show that this constraint is not just for SYM_CODE_START*.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-13 17:56                           ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Nick Desaulniers
@ 2021-01-14 10:39                             ` Borislav Petkov
  2021-01-14 11:36                               ` Peter Zijlstra
  2021-01-14 15:14                               ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Josh Poimboeuf
  0 siblings, 2 replies; 33+ messages in thread
From: Borislav Petkov @ 2021-01-14 10:39 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Josh Poimboeuf, Mark Brown, Andy Lutomirski, Thomas Gleixner,
	Ingo Molnar, Fangrui Song, Arnd Bergmann, Jonathan Corbet,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Wed, Jan 13, 2021 at 09:56:04AM -0800, Nick Desaulniers wrote:
> Apologies, that was not my intention.  I've sent a follow up in
> https://lore.kernel.org/lkml/20210113174620.958429-1-ndesaulniers@google.com/T/#u
> since BP picked up v3 in tip x86/entry:
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/entry&id=bde718b7e154afc99e1956b18a848401ce8e1f8e

It is the topmost patch so I can rebase...

Also, I replicated that text into linkage.h and removed the change over
SYM_CODE_START and I've got the below.

Further complaints?

---
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Tue, 12 Jan 2021 11:46:24 -0800
Subject: [PATCH] x86/entry: Emit a symbol for register restoring thunk

Arnd found a randconfig that produces the warning:

  arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
  offset 0x3e

when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
notes:

  With the LLVM assembler not generating section symbols, objtool has no
  way to reference this code when it generates ORC unwinder entries,
  because this code is outside of any ELF function.

  The limitation now being imposed by objtool is that all code must be
  contained in an ELF symbol.  And .L symbols don't create such symbols.

  So basically, you can use an .L symbol *inside* a function or a code
  segment, you just can't use the .L symbol to contain the code using a
  SYM_*_START/END annotation pair.

Fangrui notes that this optimization is helpful for reducing image size
when compiling with -ffunction-sections and -fdata-sections. I have
observed on the order of tens of thousands of symbols for the kernel
images built with those flags.

A patch has been authored against GNU binutils to match this behavior
of not generating unused section symbols ([1]), so this will
also become a problem for users of GNU binutils once they upgrade to 2.36.

Omit the .L prefix on a label so that the assembler will emit an entry
into the symbol table for the label, with STB_LOCAL binding. This
enables objtool to generate proper unwind info here with LLVM_IAS=1 or
GNU binutils 2.36+.

 [ bp: Massage commit message. ]

Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210112194625.4181814-1-ndesaulniers@google.com
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d1bcae833b32f1408485ce69f844dcd7ded093a8 [1]
---
 Documentation/asm-annotations.rst | 5 +++++
 arch/x86/entry/thunk_64.S         | 8 ++++----
 include/linux/linkage.h           | 5 +++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index 32ea57483378..76424e0431f4 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -100,6 +100,11 @@ Instruction Macros
 ~~~~~~~~~~~~~~~~~~
 This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
 
+``objtool`` requires that all code must be contained in an ELF symbol. Symbol
+names that have a ``.L`` prefix do not emit symbol table entries. ``.L``
+prefixed symbols can be used within a code region, but should be avoided for
+denoting a range of code via ``SYM_*_START/END`` annotations.
+
 * ``SYM_FUNC_START`` and ``SYM_FUNC_START_LOCAL`` are supposed to be **the
   most frequent markings**. They are used for functions with standard calling
   conventions -- global and local. Like in C, they both align the functions to
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd32877a3c4..c9a9fbf1655f 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index 5bcfbd972e97..dbf8506decca 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -178,6 +178,11 @@
  * Objtool generates debug info for both FUNC & CODE, but needs special
  * annotations for each CODE's start (to describe the actual stack frame).
  *
+ * Objtool requires that all code must be contained in an ELF symbol. Symbol
+ * names that have a  .L prefix do not emit symbol table entries. .L
+ * prefixed symbols can be used within a code region, but should be avoided for
+ * denoting a range of code via ``SYM_*_START/END`` annotations.
+ *
  * ALIAS -- does not generate debug info -- the aliased function will
  */
 
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-14 10:39                             ` Borislav Petkov
@ 2021-01-14 11:36                               ` Peter Zijlstra
  2021-01-14 13:28                                 ` [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument Borislav Petkov
  2021-01-19 10:12                                 ` [tip: x86/entry] " tip-bot2 for Borislav Petkov
  2021-01-14 15:14                               ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Josh Poimboeuf
  1 sibling, 2 replies; 33+ messages in thread
From: Peter Zijlstra @ 2021-01-14 11:36 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nick Desaulniers, Josh Poimboeuf, Mark Brown, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Fangrui Song, Arnd Bergmann,
	Jonathan Corbet, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Thu, Jan 14, 2021 at 11:39:28AM +0100, Borislav Petkov wrote:
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Tue, 12 Jan 2021 11:46:24 -0800
> Subject: [PATCH] x86/entry: Emit a symbol for register restoring thunk
> 
> Arnd found a randconfig that produces the warning:
> 
>   arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
>   offset 0x3e
> 
> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler not generating section symbols, objtool has no
>   way to reference this code when it generates ORC unwinder entries,
>   because this code is outside of any ELF function.
> 
>   The limitation now being imposed by objtool is that all code must be
>   contained in an ELF symbol.  And .L symbols don't create such symbols.
> 
>   So basically, you can use an .L symbol *inside* a function or a code
>   segment, you just can't use the .L symbol to contain the code using a
>   SYM_*_START/END annotation pair.
> 
> Fangrui notes that this optimization is helpful for reducing image size
> when compiling with -ffunction-sections and -fdata-sections. I have
> observed on the order of tens of thousands of symbols for the kernel
> images built with those flags.
> 
> A patch has been authored against GNU binutils to match this behavior
> of not generating unused section symbols ([1]), so this will
> also become a problem for users of GNU binutils once they upgrade to 2.36.
> 
> Omit the .L prefix on a label so that the assembler will emit an entry
> into the symbol table for the label, with STB_LOCAL binding. This
> enables objtool to generate proper unwind info here with LLVM_IAS=1 or
> GNU binutils 2.36+.
> 
>  [ bp: Massage commit message. ]
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Suggested-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

And while looking, I suppose we can delete the put_ret_addr_in_rdi crud,
but that's another patch.

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

* [Linux-kernel-mentees] Fwd: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-12 21:01                       ` Mark Brown
  2021-01-13 16:59                         ` Josh Poimboeuf
@ 2021-01-14 11:49                         ` Lukas Bulwahn
  2021-01-14 14:21                           ` Aditya
  1 sibling, 1 reply; 33+ messages in thread
From: Lukas Bulwahn @ 2021-01-14 11:49 UTC (permalink / raw)
  To: Dwaipayan Ray, Aditya Srivastava; +Cc: Joe Perches, linux-kernel-mentees

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

Dwaipayan and Aditya, here is another candidate for a checkpatch rule.

Lukas

---------- Forwarded message ---------
From: Mark Brown <broonie@kernel.org>
Date: Tue, Jan 12, 2021 at 10:02 PM
Subject: Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>, Thomas Gleixner
<tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov
<bp@alien8.de>, Fangrui Song <maskray@google.com>, Arnd Bergmann
<arnd@arndb.de>, Josh Poimboeuf <jpoimboe@redhat.com>, Jonathan Corbet
<corbet@lwn.net>, <x86@kernel.org>, H. Peter Anvin <hpa@zytor.com>,
Nathan Chancellor <natechancellor@gmail.com>, Miguel Ojeda
<ojeda@kernel.org>, Jiri Slaby <jirislaby@kernel.org>, Joe Perches
<joe@perches.com>, <linux-doc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <clang-built-linux@googlegroups.com>


On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:

This:

> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> notes:

>   So basically, you can use an .L symbol *inside* a function or a code
>   segment, you just can't use the .L symbol to contain the code using a
>   SYM_*_START/END annotation pair.

is a stronger statement than this:

> +  Developers should avoid using local symbol names that are prefixed with
> +  ``.L``, as this has special meaning for the assembler; a symbol entry will
> +  not be emitted into the symbol table. This can prevent ``objtool`` from
> +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
> +  used, and ``.L`` prefixed local symbol names are still generally useable
> +  within a function, but ``.L`` prefixed local symbol names should not be used
> +  to denote the beginning or end of code regions via
> +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.

and seems more what I'd expect - SYM_FUNC* is also affected for example.
Even though other usages are probably not very likely it seems better to
keep the stronger statement in case someone comes up with one, and to
stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
affected.

This also looks like a good candiate for a checkpatch rule, but that can
be done separately of course.

--
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/20210112210154.GI4646%40sirena.org.uk.

[-- Attachment #2: signature.asc --]
[-- Type: text/plain, Size: 499 bytes --]

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl/+DkEACgkQJNaLcl1U
h9Cq7wf/T25FXiQQZ1vqQuvuSs9y7UidITjNn00or2An/9AZvjd7fNSIGitE6JZF
ulLa3Dnm6DnJ8IHxCom/f9HA0Bhqduottun00gPpRE4yYnc6I6rs4+jD3D3yaM+e
P48KYy2zaNF6Xgud3wdMdBWrO6wHbpk/FtrGu9myxKXbPoaWXCc/2JO+lQGHy2Ld
stRoPLzuNHGsqiiQyQVFUCJcva6y2q5UTYqWG21loFirQ0khEI1aHwVuifeddpjE
JamhRqQhmlTD8qRrnf8c4iVj6oexQsKzjOKkaKX2qpyYhK8bxdgvG8r0kwJfnuZM
LObeotMsynR86MF2K/fVFTw7r4fXMw==
=H9dS
-----END PGP SIGNATURE-----

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument
  2021-01-14 11:36                               ` Peter Zijlstra
@ 2021-01-14 13:28                                 ` Borislav Petkov
  2021-01-14 15:53                                   ` Peter Zijlstra
  2021-01-19 10:12                                 ` [tip: x86/entry] " tip-bot2 for Borislav Petkov
  1 sibling, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2021-01-14 13:28 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Nick Desaulniers, Josh Poimboeuf, Mark Brown, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Fangrui Song, Arnd Bergmann,
	Jonathan Corbet, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Thu, Jan 14, 2021 at 12:36:45PM +0100, Peter Zijlstra wrote:
> And while looking, I suppose we can delete the put_ret_addr_in_rdi crud,
> but that's another patch.

---
From: Borislav Petkov <bp@suse.de>
Date: Thu, 14 Jan 2021 14:25:35 +0100
Subject: [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument

That logic is unused since

  320100a5ffe5 ("x86/entry: Remove the TRACE_IRQS cruft")

Remove it.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/entry/thunk_64.S | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index c9a9fbf1655f..496b11ec469d 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -10,7 +10,7 @@
 #include <asm/export.h>
 
 	/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
-	.macro THUNK name, func, put_ret_addr_in_rdi=0
+	.macro THUNK name, func
 SYM_FUNC_START_NOALIGN(\name)
 	pushq %rbp
 	movq %rsp, %rbp
@@ -25,11 +25,6 @@ SYM_FUNC_START_NOALIGN(\name)
 	pushq %r10
 	pushq %r11
 
-	.if \put_ret_addr_in_rdi
-	/* 8(%rbp) is return addr on stack */
-	movq 8(%rbp), %rdi
-	.endif
-
 	call \func
 	jmp  __thunk_restore
 SYM_FUNC_END(\name)
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [Linux-kernel-mentees] Fwd: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-14 11:49                         ` [Linux-kernel-mentees] Fwd: " Lukas Bulwahn
@ 2021-01-14 14:21                           ` Aditya
  2021-01-14 14:35                             ` Lukas Bulwahn
  0 siblings, 1 reply; 33+ messages in thread
From: Aditya @ 2021-01-14 14:21 UTC (permalink / raw)
  To: Lukas Bulwahn, Dwaipayan Ray; +Cc: Joe Perches, linux-kernel-mentees

On 14/1/21 5:19 pm, Lukas Bulwahn wrote:
> Dwaipayan and Aditya, here is another candidate for a checkpatch rule.
> 
> Lukas
> 
> ---------- Forwarded message ---------
> From: Mark Brown <broonie@kernel.org>
> Date: Tue, Jan 12, 2021 at 10:02 PM
> Subject: Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
> To: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>, Thomas Gleixner
> <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov
> <bp@alien8.de>, Fangrui Song <maskray@google.com>, Arnd Bergmann
> <arnd@arndb.de>, Josh Poimboeuf <jpoimboe@redhat.com>, Jonathan Corbet
> <corbet@lwn.net>, <x86@kernel.org>, H. Peter Anvin <hpa@zytor.com>,
> Nathan Chancellor <natechancellor@gmail.com>, Miguel Ojeda
> <ojeda@kernel.org>, Jiri Slaby <jirislaby@kernel.org>, Joe Perches
> <joe@perches.com>, <linux-doc@vger.kernel.org>,
> <linux-kernel@vger.kernel.org>, <clang-built-linux@googlegroups.com>
> 
> 
> On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:
> 
> This:
> 
>> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
>> notes:
> 
>>   So basically, you can use an .L symbol *inside* a function or a code
>>   segment, you just can't use the .L symbol to contain the code using a
>>   SYM_*_START/END annotation pair.
> 
> is a stronger statement than this:
> 
>> +  Developers should avoid using local symbol names that are prefixed with
>> +  ``.L``, as this has special meaning for the assembler; a symbol entry will
>> +  not be emitted into the symbol table. This can prevent ``objtool`` from
>> +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
>> +  used, and ``.L`` prefixed local symbol names are still generally useable
>> +  within a function, but ``.L`` prefixed local symbol names should not be used
>> +  to denote the beginning or end of code regions via
>> +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
> 
> and seems more what I'd expect - SYM_FUNC* is also affected for example.
> Even though other usages are probably not very likely it seems better to
> keep the stronger statement in case someone comes up with one, and to
> stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
> affected.
> 
> This also looks like a good candiate for a checkpatch rule, but that can
> be done separately of course.
> 

Thanks Lukas!
I'd like to take a shot at it. This is what I have understood from the
discussion:
Introduce a new warning rule for .S files, where ".L" prefixes are
used in the code, if it is occurring between any kind of START/END pair.
This is what I am planning to do:
- Check if the file is ".S" file.
- If the line contains ".L" prefixed symbol, give user a
warning/check, so that they can ensure that the line is not inside
START/END block. (As we may not be able to make sure about the same,
if the START/END line is not in the patch; otherwise we could run a
while loop)

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] Fwd: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-14 14:21                           ` Aditya
@ 2021-01-14 14:35                             ` Lukas Bulwahn
  0 siblings, 0 replies; 33+ messages in thread
From: Lukas Bulwahn @ 2021-01-14 14:35 UTC (permalink / raw)
  To: Aditya; +Cc: Joe Perches, Dwaipayan Ray, linux-kernel-mentees

Sounds good to me.

On Thu, Jan 14, 2021 at 3:21 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 14/1/21 5:19 pm, Lukas Bulwahn wrote:
> > Dwaipayan and Aditya, here is another candidate for a checkpatch rule.
> >
> > Lukas
> >
> > ---------- Forwarded message ---------
> > From: Mark Brown <broonie@kernel.org>
> > Date: Tue, Jan 12, 2021 at 10:02 PM
> > Subject: Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
> > To: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Andy Lutomirski <luto@kernel.org>, Thomas Gleixner
> > <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov
> > <bp@alien8.de>, Fangrui Song <maskray@google.com>, Arnd Bergmann
> > <arnd@arndb.de>, Josh Poimboeuf <jpoimboe@redhat.com>, Jonathan Corbet
> > <corbet@lwn.net>, <x86@kernel.org>, H. Peter Anvin <hpa@zytor.com>,
> > Nathan Chancellor <natechancellor@gmail.com>, Miguel Ojeda
> > <ojeda@kernel.org>, Jiri Slaby <jirislaby@kernel.org>, Joe Perches
> > <joe@perches.com>, <linux-doc@vger.kernel.org>,
> > <linux-kernel@vger.kernel.org>, <clang-built-linux@googlegroups.com>
> >
> >
> > On Tue, Jan 12, 2021 at 11:46:24AM -0800, Nick Desaulniers wrote:
> >
> > This:
> >
> >> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> >> notes:
> >
> >>   So basically, you can use an .L symbol *inside* a function or a code
> >>   segment, you just can't use the .L symbol to contain the code using a
> >>   SYM_*_START/END annotation pair.
> >
> > is a stronger statement than this:
> >
> >> +  Developers should avoid using local symbol names that are prefixed with
> >> +  ``.L``, as this has special meaning for the assembler; a symbol entry will
> >> +  not be emitted into the symbol table. This can prevent ``objtool`` from
> >> +  generating correct unwind info. Symbols with STB_LOCAL binding may still be
> >> +  used, and ``.L`` prefixed local symbol names are still generally useable
> >> +  within a function, but ``.L`` prefixed local symbol names should not be used
> >> +  to denote the beginning or end of code regions via
> >> +  ``SYM_CODE_START_LOCAL``/``SYM_CODE_END``.
> >
> > and seems more what I'd expect - SYM_FUNC* is also affected for example.
> > Even though other usages are probably not very likely it seems better to
> > keep the stronger statement in case someone comes up with one, and to
> > stop anyone spending time wondering why only SYM_CODE_START_LOCAL is
> > affected.
> >
> > This also looks like a good candiate for a checkpatch rule, but that can
> > be done separately of course.
> >
>
> Thanks Lukas!
> I'd like to take a shot at it. This is what I have understood from the
> discussion:
> Introduce a new warning rule for .S files, where ".L" prefixes are
> used in the code, if it is occurring between any kind of START/END pair.
> This is what I am planning to do:
> - Check if the file is ".S" file.
> - If the line contains ".L" prefixed symbol, give user a
> warning/check, so that they can ensure that the line is not inside
> START/END block. (As we may not be able to make sure about the same,
> if the START/END line is not in the patch; otherwise we could run a
> while loop)
>
> What do you think?
>
> Thanks
> Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [PATCH v4] x86/entry: emit a symbol for register restoring thunk
  2021-01-14 10:39                             ` Borislav Petkov
  2021-01-14 11:36                               ` Peter Zijlstra
@ 2021-01-14 15:14                               ` Josh Poimboeuf
  1 sibling, 0 replies; 33+ messages in thread
From: Josh Poimboeuf @ 2021-01-14 15:14 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nick Desaulniers, Mark Brown, Andy Lutomirski, Thomas Gleixner,
	Ingo Molnar, Fangrui Song, Arnd Bergmann, Jonathan Corbet,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Thu, Jan 14, 2021 at 11:39:28AM +0100, Borislav Petkov wrote:
> On Wed, Jan 13, 2021 at 09:56:04AM -0800, Nick Desaulniers wrote:
> > Apologies, that was not my intention.  I've sent a follow up in
> > https://lore.kernel.org/lkml/20210113174620.958429-1-ndesaulniers@google.com/T/#u
> > since BP picked up v3 in tip x86/entry:
> > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/entry&id=bde718b7e154afc99e1956b18a848401ce8e1f8e
> 
> It is the topmost patch so I can rebase...
> 
> Also, I replicated that text into linkage.h and removed the change over
> SYM_CODE_START and I've got the below.
> 
> Further complaints?
> 
> ---
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Tue, 12 Jan 2021 11:46:24 -0800
> Subject: [PATCH] x86/entry: Emit a symbol for register restoring thunk
> 
> Arnd found a randconfig that produces the warning:
> 
>   arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
>   offset 0x3e
> 
> when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
> notes:
> 
>   With the LLVM assembler not generating section symbols, objtool has no
>   way to reference this code when it generates ORC unwinder entries,
>   because this code is outside of any ELF function.
> 
>   The limitation now being imposed by objtool is that all code must be
>   contained in an ELF symbol.  And .L symbols don't create such symbols.
> 
>   So basically, you can use an .L symbol *inside* a function or a code
>   segment, you just can't use the .L symbol to contain the code using a
>   SYM_*_START/END annotation pair.
> 
> Fangrui notes that this optimization is helpful for reducing image size
> when compiling with -ffunction-sections and -fdata-sections. I have
> observed on the order of tens of thousands of symbols for the kernel
> images built with those flags.
> 
> A patch has been authored against GNU binutils to match this behavior
> of not generating unused section symbols ([1]), so this will
> also become a problem for users of GNU binutils once they upgrade to 2.36.
> 
> Omit the .L prefix on a label so that the assembler will emit an entry
> into the symbol table for the label, with STB_LOCAL binding. This
> enables objtool to generate proper unwind info here with LLVM_IAS=1 or
> GNU binutils 2.36+.
> 
>  [ bp: Massage commit message. ]

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

-- 
Josh


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

* Re: [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument
  2021-01-14 13:28                                 ` [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument Borislav Petkov
@ 2021-01-14 15:53                                   ` Peter Zijlstra
  0 siblings, 0 replies; 33+ messages in thread
From: Peter Zijlstra @ 2021-01-14 15:53 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nick Desaulniers, Josh Poimboeuf, Mark Brown, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Fangrui Song, Arnd Bergmann,
	Jonathan Corbet, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	H. Peter Anvin, Nathan Chancellor, Miguel Ojeda, Jiri Slaby,
	Joe Perches, Linux Doc Mailing List, LKML, clang-built-linux

On Thu, Jan 14, 2021 at 02:28:09PM +0100, Borislav Petkov wrote:
> On Thu, Jan 14, 2021 at 12:36:45PM +0100, Peter Zijlstra wrote:
> > And while looking, I suppose we can delete the put_ret_addr_in_rdi crud,
> > but that's another patch.
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Thu, 14 Jan 2021 14:25:35 +0100
> Subject: [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument
> 
> That logic is unused since
> 
>   320100a5ffe5 ("x86/entry: Remove the TRACE_IRQS cruft")
> 
> Remove it.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by; Peter Zijlstra (Intel) <peterz@infradead.org>

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

* [tip: x86/entry] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument
  2021-01-14 11:36                               ` Peter Zijlstra
  2021-01-14 13:28                                 ` [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument Borislav Petkov
@ 2021-01-19 10:12                                 ` tip-bot2 for Borislav Petkov
  1 sibling, 0 replies; 33+ messages in thread
From: tip-bot2 for Borislav Petkov @ 2021-01-19 10:12 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Peter Zijlstra (Intel), Borislav Petkov, x86, linux-kernel

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

Commit-ID:     0bab9cb2d980d7c075cffb9216155f7835237f98
Gitweb:        https://git.kernel.org/tip/0bab9cb2d980d7c075cffb9216155f7835237f98
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Thu, 14 Jan 2021 14:25:35 +01:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 19 Jan 2021 11:06:14 +01:00

x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument

That logic is unused since

  320100a5ffe5 ("x86/entry: Remove the TRACE_IRQS cruft")

Remove it.

Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/YAAszZJ2GcIYZmB5@hirez.programming.kicks-ass.net
---
 arch/x86/entry/thunk_64.S | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index c9a9fbf..496b11e 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -10,7 +10,7 @@
 #include <asm/export.h>
 
 	/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
-	.macro THUNK name, func, put_ret_addr_in_rdi=0
+	.macro THUNK name, func
 SYM_FUNC_START_NOALIGN(\name)
 	pushq %rbp
 	movq %rsp, %rbp
@@ -25,11 +25,6 @@ SYM_FUNC_START_NOALIGN(\name)
 	pushq %r10
 	pushq %r11
 
-	.if \put_ret_addr_in_rdi
-	/* 8(%rbp) is return addr on stack */
-	movq 8(%rbp), %rdi
-	.endif
-
 	call \func
 	jmp  __thunk_restore
 SYM_FUNC_END(\name)

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

* [tip: x86/entry] x86/entry: Emit a symbol for register restoring thunk
  2021-01-12 19:46                     ` [PATCH v4] " Nick Desaulniers
  2021-01-12 21:01                       ` Mark Brown
  2021-01-13 11:52                       ` [tip: x86/entry] x86/entry: Emit " tip-bot2 for Nick Desaulniers
@ 2021-01-19 10:12                       ` tip-bot2 for Nick Desaulniers
  2 siblings, 0 replies; 33+ messages in thread
From: tip-bot2 for Nick Desaulniers @ 2021-01-19 10:12 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Arnd Bergmann, Josh Poimboeuf, Borislav Petkov, Mark Brown,
	Nick Desaulniers, Borislav Petkov, Peter Zijlstra (Intel),
	x86, linux-kernel

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

Commit-ID:     5e6dca82bcaa49348f9e5fcb48df4881f6d6c4ae
Gitweb:        https://git.kernel.org/tip/5e6dca82bcaa49348f9e5fcb48df4881f6d6c4ae
Author:        Nick Desaulniers <ndesaulniers@google.com>
AuthorDate:    Tue, 12 Jan 2021 11:46:24 -08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 14 Jan 2021 17:18:25 +01:00

x86/entry: Emit a symbol for register restoring thunk

Arnd found a randconfig that produces the warning:

  arch/x86/entry/thunk_64.o: warning: objtool: missing symbol for insn at
  offset 0x3e

when building with LLVM_IAS=1 (Clang's integrated assembler). Josh
notes:

  With the LLVM assembler not generating section symbols, objtool has no
  way to reference this code when it generates ORC unwinder entries,
  because this code is outside of any ELF function.

  The limitation now being imposed by objtool is that all code must be
  contained in an ELF symbol.  And .L symbols don't create such symbols.

  So basically, you can use an .L symbol *inside* a function or a code
  segment, you just can't use the .L symbol to contain the code using a
  SYM_*_START/END annotation pair.

Fangrui notes that this optimization is helpful for reducing image size
when compiling with -ffunction-sections and -fdata-sections. I have
observed on the order of tens of thousands of symbols for the kernel
images built with those flags.

A patch has been authored against GNU binutils to match this behavior
of not generating unused section symbols ([1]), so this will
also become a problem for users of GNU binutils once they upgrade to 2.36.

Omit the .L prefix on a label so that the assembler will emit an entry
into the symbol table for the label, with STB_LOCAL binding. This
enables objtool to generate proper unwind info here with LLVM_IAS=1 or
GNU binutils 2.36+.

 [ bp: Massage commit message. ]

Reported-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20210112194625.4181814-1-ndesaulniers@google.com
Link: https://github.com/ClangBuiltLinux/linux/issues/1209
Link: https://reviews.llvm.org/D93783
Link: https://sourceware.org/binutils/docs/as/Symbol-Names.html
Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d1bcae833b32f1408485ce69f844dcd7ded093a8 [1]
---
 Documentation/asm-annotations.rst | 5 +++++
 arch/x86/entry/thunk_64.S         | 8 ++++----
 include/linux/linkage.h           | 5 +++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index 32ea574..76424e0 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -100,6 +100,11 @@ Instruction Macros
 ~~~~~~~~~~~~~~~~~~
 This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
 
+``objtool`` requires that all code must be contained in an ELF symbol. Symbol
+names that have a ``.L`` prefix do not emit symbol table entries. ``.L``
+prefixed symbols can be used within a code region, but should be avoided for
+denoting a range of code via ``SYM_*_START/END`` annotations.
+
 * ``SYM_FUNC_START`` and ``SYM_FUNC_START_LOCAL`` are supposed to be **the
   most frequent markings**. They are used for functions with standard calling
   conventions -- global and local. Like in C, they both align the functions to
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index ccd3287..c9a9fbf 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -31,7 +31,7 @@ SYM_FUNC_START_NOALIGN(\name)
 	.endif
 
 	call \func
-	jmp  .L_restore
+	jmp  __thunk_restore
 SYM_FUNC_END(\name)
 	_ASM_NOKPROBE(\name)
 	.endm
@@ -44,7 +44,7 @@ SYM_FUNC_END(\name)
 #endif
 
 #ifdef CONFIG_PREEMPTION
-SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
+SYM_CODE_START_LOCAL_NOALIGN(__thunk_restore)
 	popq %r11
 	popq %r10
 	popq %r9
@@ -56,6 +56,6 @@ SYM_CODE_START_LOCAL_NOALIGN(.L_restore)
 	popq %rdi
 	popq %rbp
 	ret
-	_ASM_NOKPROBE(.L_restore)
-SYM_CODE_END(.L_restore)
+	_ASM_NOKPROBE(__thunk_restore)
+SYM_CODE_END(__thunk_restore)
 #endif
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index 5bcfbd9..dbf8506 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -178,6 +178,11 @@
  * Objtool generates debug info for both FUNC & CODE, but needs special
  * annotations for each CODE's start (to describe the actual stack frame).
  *
+ * Objtool requires that all code must be contained in an ELF symbol. Symbol
+ * names that have a  .L prefix do not emit symbol table entries. .L
+ * prefixed symbols can be used within a code region, but should be avoided for
+ * denoting a range of code via ``SYM_*_START/END`` annotations.
+ *
  * ALIAS -- does not generate debug info -- the aliased function will
  */
 

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

end of thread, other threads:[~2021-01-19 10:39 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23 23:21 [PATCH] x86/entry: use STB_GLOBAL for register restoring thunk Nick Desaulniers
2020-12-24  4:55 ` Josh Poimboeuf
2021-01-06  0:43   ` [PATCH v2] " Nick Desaulniers
2021-01-06  1:58     ` Josh Poimboeuf
2021-01-11 20:38       ` [PATCH v3] x86/entry: emit a symbol " Nick Desaulniers
2021-01-11 20:58         ` Fangrui Song
2021-01-11 22:09           ` Josh Poimboeuf
2021-01-11 22:16             ` Fāng-ruì Sòng
2021-01-11 22:12         ` Josh Poimboeuf
2021-01-12  0:38         ` Borislav Petkov
2021-01-12  0:41           ` Fāng-ruì Sòng
2021-01-12  1:00             ` Borislav Petkov
2021-01-12  1:13               ` Nick Desaulniers
2021-01-12  1:59                 ` Josh Poimboeuf
2021-01-12 11:54                   ` Borislav Petkov
2021-01-12 19:46                     ` [PATCH v4] " Nick Desaulniers
2021-01-12 21:01                       ` Mark Brown
2021-01-13 16:59                         ` Josh Poimboeuf
2021-01-13 17:46                           ` [PATCH] Documentation: asm-annotation: clarify .L local symbol names Nick Desaulniers
2021-01-13 19:56                             ` Mark Brown
2021-01-13 17:56                           ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Nick Desaulniers
2021-01-14 10:39                             ` Borislav Petkov
2021-01-14 11:36                               ` Peter Zijlstra
2021-01-14 13:28                                 ` [PATCH] x86/entry: Remove put_ret_addr_in_rdi THUNK macro argument Borislav Petkov
2021-01-14 15:53                                   ` Peter Zijlstra
2021-01-19 10:12                                 ` [tip: x86/entry] " tip-bot2 for Borislav Petkov
2021-01-14 15:14                               ` [PATCH v4] x86/entry: emit a symbol for register restoring thunk Josh Poimboeuf
2021-01-14 11:49                         ` [Linux-kernel-mentees] Fwd: " Lukas Bulwahn
2021-01-14 14:21                           ` Aditya
2021-01-14 14:35                             ` Lukas Bulwahn
2021-01-13 11:52                       ` [tip: x86/entry] x86/entry: Emit " tip-bot2 for Nick Desaulniers
2021-01-19 10:12                       ` tip-bot2 for Nick Desaulniers
2021-01-12 11:47                 ` [PATCH v3] x86/entry: emit " Borislav Petkov

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.