diff mbox series

[v6,22/25] x86/asm: annotate indirect jumps

Message ID 20201013003203.4168817-23-samitolvanen@google.com
State New, archived
Headers show
Series Add support for Clang LTO | expand

Commit Message

Sami Tolvanen Oct. 13, 2020, 12:32 a.m. UTC
Running objtool --vmlinux --duplicate on vmlinux.o produces a few
warnings about indirect jumps with retpoline:

  vmlinux.o: warning: objtool: wakeup_long64()+0x61: indirect jump
  found in RETPOLINE build
  ...

This change adds ANNOTATE_RETPOLINE_SAFE annotations to the jumps
in assembly code to stop the warnings.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/kernel/acpi/wakeup_64.S  | 2 ++
 arch/x86/platform/pvh/head.S      | 2 ++
 arch/x86/power/hibernate_asm_64.S | 3 +++
 3 files changed, 7 insertions(+)

Comments

Kees Cook Oct. 14, 2020, 10:46 p.m. UTC | #1
On Mon, Oct 12, 2020 at 05:32:00PM -0700, Sami Tolvanen wrote:
> Running objtool --vmlinux --duplicate on vmlinux.o produces a few
> warnings about indirect jumps with retpoline:
> 
>   vmlinux.o: warning: objtool: wakeup_long64()+0x61: indirect jump
>   found in RETPOLINE build
>   ...
> 
> This change adds ANNOTATE_RETPOLINE_SAFE annotations to the jumps
> in assembly code to stop the warnings.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

This looks like it's an independent fix -- can an x86 maintainer pick
up this patch directly?
Jann Horn Oct. 14, 2020, 11:23 p.m. UTC | #2
+objtool folks

On Tue, Oct 13, 2020 at 2:35 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> Running objtool --vmlinux --duplicate on vmlinux.o produces a few
> warnings about indirect jumps with retpoline:
>
>   vmlinux.o: warning: objtool: wakeup_long64()+0x61: indirect jump
>   found in RETPOLINE build
>   ...
>
> This change adds ANNOTATE_RETPOLINE_SAFE annotations to the jumps
> in assembly code to stop the warnings.

In other words, this patch deals with the fact that
OBJECT_FILES_NON_STANDARD stops being effective for object files that
are linked into the main kernel when LTO is on, right?
All the files you're touching here are supposed to be excluded from
objtool warnings at the moment:

$ grep OBJECT_FILES_NON_STANDARD arch/x86/kernel/acpi/Makefile
OBJECT_FILES_NON_STANDARD_wakeup_$(BITS).o := y
$ grep OBJECT_FILES_NON_STANDARD arch/x86/platform/pvh/Makefile
OBJECT_FILES_NON_STANDARD_head.o := y
$ grep OBJECT_FILES_NON_STANDARD arch/x86/power/Makefile
OBJECT_FILES_NON_STANDARD_hibernate_asm_$(BITS).o := y

It would probably be good to keep LTO and non-LTO builds in sync about
which files are subjected to objtool checks. So either you should be
removing the OBJECT_FILES_NON_STANDARD annotations for anything that
is linked into the main kernel (which would be a nice cleanup, if that
is possible), or alternatively ensure that code from these files is
excluded from objtool checks even with LTO (that'd probably be messy
and a bad idea?).

Grepping for other files marked as OBJECT_FILES_NON_STANDARD that
might be included in the main kernel on x86, I also see stuff like:

    5 arch/x86/crypto/Makefile                            5
OBJECT_FILES_NON_STANDARD := y
   10 arch/x86/kernel/Makefile                           39
OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o          := y
   12 arch/x86/kvm/Makefile                               7
OBJECT_FILES_NON_STANDARD_vmenter.o := y

for which I think the same thing applies.
Peter Zijlstra Oct. 15, 2020, 10:22 a.m. UTC | #3
On Thu, Oct 15, 2020 at 01:23:41AM +0200, Jann Horn wrote:

> It would probably be good to keep LTO and non-LTO builds in sync about
> which files are subjected to objtool checks. So either you should be
> removing the OBJECT_FILES_NON_STANDARD annotations for anything that
> is linked into the main kernel (which would be a nice cleanup, if that
> is possible), 

This, I've had to do that for a number of files already for the limited
vmlinux.o passes we needed for noinstr validation.
Josh Poimboeuf Oct. 15, 2020, 8:39 p.m. UTC | #4
On Thu, Oct 15, 2020 at 12:22:16PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 15, 2020 at 01:23:41AM +0200, Jann Horn wrote:
> 
> > It would probably be good to keep LTO and non-LTO builds in sync about
> > which files are subjected to objtool checks. So either you should be
> > removing the OBJECT_FILES_NON_STANDARD annotations for anything that
> > is linked into the main kernel (which would be a nice cleanup, if that
> > is possible), 
> 
> This, I've had to do that for a number of files already for the limited
> vmlinux.o passes we needed for noinstr validation.

Getting rid of OBJECT_FILES_NON_STANDARD is indeed the end goal, though
I'm not sure how practical that will be for some of the weirder edge
case.

On a related note, I have some old crypto cleanups which need dusting
off.
Sami Tolvanen Oct. 20, 2020, 4:45 p.m. UTC | #5
On Thu, Oct 15, 2020 at 1:39 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Oct 15, 2020 at 12:22:16PM +0200, Peter Zijlstra wrote:
> > On Thu, Oct 15, 2020 at 01:23:41AM +0200, Jann Horn wrote:
> >
> > > It would probably be good to keep LTO and non-LTO builds in sync about
> > > which files are subjected to objtool checks. So either you should be
> > > removing the OBJECT_FILES_NON_STANDARD annotations for anything that
> > > is linked into the main kernel (which would be a nice cleanup, if that
> > > is possible),
> >
> > This, I've had to do that for a number of files already for the limited
> > vmlinux.o passes we needed for noinstr validation.
>
> Getting rid of OBJECT_FILES_NON_STANDARD is indeed the end goal, though
> I'm not sure how practical that will be for some of the weirder edge
> case.
>
> On a related note, I have some old crypto cleanups which need dusting
> off.

Building allyesconfig with this series and LTO enabled, I still see
the following objtool warnings for vmlinux.o, grouped by source file:

arch/x86/entry/entry_64.S:
__switch_to_asm()+0x0: undefined stack state
.entry.text+0xffd: sibling call from callable instruction with
modified stack frame
.entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0

arch/x86/entry/entry_64_compat.S:
.entry.text+0x1754: unsupported instruction in callable function
.entry.text+0x1634: redundant CLD
.entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
.entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0

arch/x86/kernel/head_64.S:
.head.text+0xfb: unsupported instruction in callable function

arch/x86/kernel/acpi/wakeup_64.S:
do_suspend_lowlevel()+0x116: sibling call from callable instruction
with modified stack frame

arch/x86/crypto/camellia-aesni-avx2-asm_64.S:
camellia_cbc_dec_32way()+0xb3: stack state mismatch: cfa1=7+520 cfa2=7+8
camellia_ctr_32way()+0x1a: stack state mismatch: cfa1=7+520 cfa2=7+8

arch/x86/crypto/aesni-intel_avx-x86_64.S:
aesni_gcm_init_avx_gen2()+0x12: unsupported stack pointer realignment
aesni_gcm_enc_update_avx_gen2()+0x12: unsupported stack pointer realignment
aesni_gcm_dec_update_avx_gen2()+0x12: unsupported stack pointer realignment
aesni_gcm_finalize_avx_gen2()+0x12: unsupported stack pointer realignment
aesni_gcm_init_avx_gen4()+0x12: unsupported stack pointer realignment
aesni_gcm_enc_update_avx_gen4()+0x12: unsupported stack pointer realignment
aesni_gcm_dec_update_avx_gen4()+0x12: unsupported stack pointer realignment
aesni_gcm_finalize_avx_gen4()+0x12: unsupported stack pointer realignment

arch/x86/crypto/sha1_avx2_x86_64_asm.S:
sha1_transform_avx2()+0xc: unsupported stack pointer realignment

arch/x86/crypto/sha1_ni_asm.S:
sha1_ni_transform()+0x7: unsupported stack pointer realignment

arch/x86/crypto/sha256-avx2-asm.S:
sha256_transform_rorx()+0x13: unsupported stack pointer realignment

arch/x86/crypto/sha512-ssse3-asm.S:
sha512_transform_ssse3()+0x14: unsupported stack pointer realignment

arch/x86/crypto/sha512-avx-asm.S:
sha512_transform_avx()+0x14: unsupported stack pointer realignment

arch/x86/crypto/sha512-avx2-asm.S:
sha512_transform_rorx()+0x7: unsupported stack pointer realignment

arch/x86/lib/retpoline.S:
__x86_retpoline_rdi()+0x10: return with modified stack frame
__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0

Josh, Peter, any thoughts on what would be the preferred way to fix
these, or how to tell objtool to ignore this code?

Sami
Josh Poimboeuf Oct. 20, 2020, 6:52 p.m. UTC | #6
On Tue, Oct 20, 2020 at 09:45:06AM -0700, Sami Tolvanen wrote:
> On Thu, Oct 15, 2020 at 1:39 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Oct 15, 2020 at 12:22:16PM +0200, Peter Zijlstra wrote:
> > > On Thu, Oct 15, 2020 at 01:23:41AM +0200, Jann Horn wrote:
> > >
> > > > It would probably be good to keep LTO and non-LTO builds in sync about
> > > > which files are subjected to objtool checks. So either you should be
> > > > removing the OBJECT_FILES_NON_STANDARD annotations for anything that
> > > > is linked into the main kernel (which would be a nice cleanup, if that
> > > > is possible),
> > >
> > > This, I've had to do that for a number of files already for the limited
> > > vmlinux.o passes we needed for noinstr validation.
> >
> > Getting rid of OBJECT_FILES_NON_STANDARD is indeed the end goal, though
> > I'm not sure how practical that will be for some of the weirder edge
> > case.
> >
> > On a related note, I have some old crypto cleanups which need dusting
> > off.
> 
> Building allyesconfig with this series and LTO enabled, I still see
> the following objtool warnings for vmlinux.o, grouped by source file:
> 
> arch/x86/entry/entry_64.S:
> __switch_to_asm()+0x0: undefined stack state
> .entry.text+0xffd: sibling call from callable instruction with
> modified stack frame
> .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0

Not sure what this one's about, there's no OBJECT_FILES_NON_STANDARD?

> arch/x86/entry/entry_64_compat.S:
> .entry.text+0x1754: unsupported instruction in callable function
> .entry.text+0x1634: redundant CLD
> .entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
> .entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0

Ditto.

> arch/x86/kernel/head_64.S:
> .head.text+0xfb: unsupported instruction in callable function

Ditto.

> arch/x86/kernel/acpi/wakeup_64.S:
> do_suspend_lowlevel()+0x116: sibling call from callable instruction
> with modified stack frame

We'll need to look at how to handle this one.

> arch/x86/crypto/camellia-aesni-avx2-asm_64.S:
> camellia_cbc_dec_32way()+0xb3: stack state mismatch: cfa1=7+520 cfa2=7+8
> camellia_ctr_32way()+0x1a: stack state mismatch: cfa1=7+520 cfa2=7+8

I can clean off my patches for all the crypto warnings.

> arch/x86/lib/retpoline.S:
> __x86_retpoline_rdi()+0x10: return with modified stack frame
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0

Is this with upstream?  I thought we fixed that with
UNWIND_HINT_RET_OFFSET.

> Josh, Peter, any thoughts on what would be the preferred way to fix
> these, or how to tell objtool to ignore this code?

One way or another, the patches need to be free of warnings before
getting merged.  I can help, though I'm traveling and only have limited
bandwidth for at least the rest of the month.

Ideally we'd want to have objtool understand everything, with no
whitelisting, but some cases (e.g. suspend code) can be tricky.

I wouldn't be opposed to embedding the whitelist in the binary, in a
discardable section.  It should be relatively easy, but as I mentioned I
may or may not have time to work on it for a bit.  I'm working half
days, and now the ocean beckons from the window of my camper.
Sami Tolvanen Oct. 20, 2020, 7:24 p.m. UTC | #7
On Tue, Oct 20, 2020 at 11:52 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Tue, Oct 20, 2020 at 09:45:06AM -0700, Sami Tolvanen wrote:
> > On Thu, Oct 15, 2020 at 1:39 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Thu, Oct 15, 2020 at 12:22:16PM +0200, Peter Zijlstra wrote:
> > > > On Thu, Oct 15, 2020 at 01:23:41AM +0200, Jann Horn wrote:
> > > >
> > > > > It would probably be good to keep LTO and non-LTO builds in sync about
> > > > > which files are subjected to objtool checks. So either you should be
> > > > > removing the OBJECT_FILES_NON_STANDARD annotations for anything that
> > > > > is linked into the main kernel (which would be a nice cleanup, if that
> > > > > is possible),
> > > >
> > > > This, I've had to do that for a number of files already for the limited
> > > > vmlinux.o passes we needed for noinstr validation.
> > >
> > > Getting rid of OBJECT_FILES_NON_STANDARD is indeed the end goal, though
> > > I'm not sure how practical that will be for some of the weirder edge
> > > case.
> > >
> > > On a related note, I have some old crypto cleanups which need dusting
> > > off.
> >
> > Building allyesconfig with this series and LTO enabled, I still see
> > the following objtool warnings for vmlinux.o, grouped by source file:
> >
> > arch/x86/entry/entry_64.S:
> > __switch_to_asm()+0x0: undefined stack state
> > .entry.text+0xffd: sibling call from callable instruction with
> > modified stack frame
> > .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
>
> Not sure what this one's about, there's no OBJECT_FILES_NON_STANDARD?

Correct, because with LTO, we won't have an ELF binary to process
until we compile everything into vmlinux.o, and at that point we can
no longer skip individual object files.

The sibling call warning is in
swapgs_restore_regs_and_return_to_usermode and the stack state
mismatch in entry_SYSCALL_64_after_hwframe.

> > arch/x86/entry/entry_64_compat.S:
> > .entry.text+0x1754: unsupported instruction in callable function

This comes from a sysretl instruction in entry_SYSCALL_compat.

> > .entry.text+0x1634: redundant CLD
> > .entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
> > .entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0
>
> Ditto.

These are all from entry_SYSENTER_compat_after_hwframe.

> > arch/x86/kernel/head_64.S:
> > .head.text+0xfb: unsupported instruction in callable function
>
> Ditto.

This is lretq in secondary_startup_64_no_verify.

> > arch/x86/crypto/camellia-aesni-avx2-asm_64.S:
> > camellia_cbc_dec_32way()+0xb3: stack state mismatch: cfa1=7+520 cfa2=7+8
> > camellia_ctr_32way()+0x1a: stack state mismatch: cfa1=7+520 cfa2=7+8
>
> I can clean off my patches for all the crypto warnings.

Great, sounds good.

> > arch/x86/lib/retpoline.S:
> > __x86_retpoline_rdi()+0x10: return with modified stack frame
> > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
> > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
>
> Is this with upstream?  I thought we fixed that with
> UNWIND_HINT_RET_OFFSET.

Yes, and the UNWIND_HINT_RET_OFFSET is there.

> > Josh, Peter, any thoughts on what would be the preferred way to fix
> > these, or how to tell objtool to ignore this code?
>
> One way or another, the patches need to be free of warnings before
> getting merged.  I can help, though I'm traveling and only have limited
> bandwidth for at least the rest of the month.
>
> Ideally we'd want to have objtool understand everything, with no
> whitelisting, but some cases (e.g. suspend code) can be tricky.
>
> I wouldn't be opposed to embedding the whitelist in the binary, in a
> discardable section.  It should be relatively easy, but as I mentioned I
> may or may not have time to work on it for a bit.  I'm working half
> days, and now the ocean beckons from the window of my camper.

Something similar to STACK_FRAME_NON_STANDARD()? Using that seems to
result in "BUG: why am I validating an ignored function?" warnings, so
I suspect some additional changes are needed.

Sami
Peter Zijlstra Oct. 21, 2020, 8:56 a.m. UTC | #8
On Tue, Oct 20, 2020 at 12:24:37PM -0700, Sami Tolvanen wrote:
> > > Building allyesconfig with this series and LTO enabled, I still see
> > > the following objtool warnings for vmlinux.o, grouped by source file:
> > >
> > > arch/x86/entry/entry_64.S:
> > > __switch_to_asm()+0x0: undefined stack state
> > > .entry.text+0xffd: sibling call from callable instruction with
> > > modified stack frame
> > > .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
> >
> > Not sure what this one's about, there's no OBJECT_FILES_NON_STANDARD?
> 
> Correct, because with LTO, we won't have an ELF binary to process
> until we compile everything into vmlinux.o, and at that point we can
> no longer skip individual object files.

I think what Josh was trying to say is; this file is subject to objtool
on a normal build and does not generate warnings. So why would it
generate warnings when subject to objtool as result of a vmlinux run
(due to LTO or otherwise).

In fact, when I build a x86_64-defconfig and then run:

  $ objtool check -barf defconfig-build/vmlinux.o

I do not see these in particular, although I do see a lot of:

  "sibling call from callable instruction with modified stack frame"
  "falls through to next function"

that did not show up in the individual objtool runs during the build.

The "falls through to next function" seems to be limited to things like:

  warning: objtool: setup_vq() falls through to next function setup_vq.cold()
  warning: objtool: e1000_xmit_frame() falls through to next function e1000_xmit_frame.cold()

So something's weird with the .cold thing on vmlinux.o runs.
Peter Zijlstra Oct. 21, 2020, 9:08 a.m. UTC | #9
On Wed, Oct 21, 2020 at 10:56:06AM +0200, Peter Zijlstra wrote:

> The "falls through to next function" seems to be limited to things like:
> 
>   warning: objtool: setup_vq() falls through to next function setup_vq.cold()
>   warning: objtool: e1000_xmit_frame() falls through to next function e1000_xmit_frame.cold()
> 
> So something's weird with the .cold thing on vmlinux.o runs.

Shiny, check this:

$ nm defconfig-build/vmlinux.o | grep setup_vq
00000000004d33a0 t setup_vq
00000000004d4c20 t setup_vq
000000000001edcc t setup_vq.cold
000000000001ee31 t setup_vq.cold
00000000004d3dc0 t vp_setup_vq

$ nm defconfig-build/vmlinux.o | grep e1000_xmit_frame
0000000000741490 t e1000_xmit_frame
0000000000763620 t e1000_xmit_frame
000000000002f579 t e1000_xmit_frame.cold
0000000000032b6e t e1000_xmit_frame.cold

$ nm defconfig-build/vmlinux.o | grep e1000_diag_test
000000000074c220 t e1000_diag_test
000000000075eb70 t e1000_diag_test
000000000002fc2a t e1000_diag_test.cold
0000000000030880 t e1000_diag_test.cold

I guess objtool goes sideways when there's multiple symbols with the
same name in a single object file. This obvously never happens on single
TU .o files.

Not sure what to do about that.
Peter Zijlstra Oct. 21, 2020, 9:32 a.m. UTC | #10
On Wed, Oct 21, 2020 at 10:56:06AM +0200, Peter Zijlstra wrote:

> I do not see these in particular, although I do see a lot of:
> 
>   "sibling call from callable instruction with modified stack frame"

defconfig-build/vmlinux.o: warning: objtool: msr_write()+0x10a: sibling call from callable instruction with modified stack frame
defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x99: (branch)
defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x3e: (branch)
defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x0: <=== (sym)

$ nm defconfig-build/vmlinux.o | grep msr_write
0000000000043250 t msr_write
00000000004289c0 T msr_write
0000000000003056 t msr_write.cold

Below 'fixes' it. So this is also caused by duplicate symbols.

---
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 3bd905e10ee2..e36331f8f217 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -48,17 +48,6 @@ int msr_read(u32 msr, struct msr *m)
 	return err;
 }
 
-/**
- * Write an MSR with error handling
- *
- * @msr: MSR to write
- * @m: value to write
- */
-int msr_write(u32 msr, struct msr *m)
-{
-	return wrmsrl_safe(msr, m->q);
-}
-
 static inline int __flip_bit(u32 msr, u8 bit, bool set)
 {
 	struct msr m, m1;
@@ -80,7 +69,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
 	if (m1.q == m.q)
 		return 0;
 
-	err = msr_write(msr, &m1);
+	err = wrmsr_safe(msr, m1.q);
 	if (err)
 		return err;
Peter Zijlstra Oct. 21, 2020, 9:51 a.m. UTC | #11
On Tue, Oct 20, 2020 at 01:52:17PM -0500, Josh Poimboeuf wrote:
> > arch/x86/lib/retpoline.S:
> > __x86_retpoline_rdi()+0x10: return with modified stack frame
> > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
> > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
> 
> Is this with upstream?  I thought we fixed that with
> UNWIND_HINT_RET_OFFSET.

I can't reproduce this one either; but I do get different warnings:

gcc (Debian 10.2.0-13) 10.2.0, x86_64-defconfig:

defconfig-build/vmlinux.o: warning: objtool: __x86_indirect_thunk_rax() falls through to next function __x86_retpoline_rax()
defconfig-build/vmlinux.o: warning: objtool:   .altinstr_replacement+0x1063: (branch)
defconfig-build/vmlinux.o: warning: objtool:   __x86_indirect_thunk_rax()+0x0: (alt)
defconfig-build/vmlinux.o: warning: objtool:   __x86_indirect_thunk_rax()+0x0: <=== (sym)

(for every single register, not just rax)

Which is daft as well, because the retpoline.o run is clean. It also
doesn't make sense because __x86_retpoline_rax isn't in fact STT_FUNC,
so WTH ?!
Sami Tolvanen Oct. 21, 2020, 3:01 p.m. UTC | #12
On Wed, Oct 21, 2020 at 1:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 20, 2020 at 12:24:37PM -0700, Sami Tolvanen wrote:
> > > > Building allyesconfig with this series and LTO enabled, I still see
> > > > the following objtool warnings for vmlinux.o, grouped by source file:
> > > >
> > > > arch/x86/entry/entry_64.S:
> > > > __switch_to_asm()+0x0: undefined stack state
> > > > .entry.text+0xffd: sibling call from callable instruction with
> > > > modified stack frame
> > > > .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
> > >
> > > Not sure what this one's about, there's no OBJECT_FILES_NON_STANDARD?
> >
> > Correct, because with LTO, we won't have an ELF binary to process
> > until we compile everything into vmlinux.o, and at that point we can
> > no longer skip individual object files.
>
> I think what Josh was trying to say is; this file is subject to objtool
> on a normal build and does not generate warnings. So why would it
> generate warnings when subject to objtool as result of a vmlinux run
> (due to LTO or otherwise).

Ah, right. It also doesn't generate warnings when I build defconfig
with LTO, so clearly something confuses objtool here.

Sami
Josh Poimboeuf Oct. 21, 2020, 6:30 p.m. UTC | #13
On Wed, Oct 21, 2020 at 11:51:33AM +0200, Peter Zijlstra wrote:
> On Tue, Oct 20, 2020 at 01:52:17PM -0500, Josh Poimboeuf wrote:
> > > arch/x86/lib/retpoline.S:
> > > __x86_retpoline_rdi()+0x10: return with modified stack frame
> > > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
> > > __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
> > 
> > Is this with upstream?  I thought we fixed that with
> > UNWIND_HINT_RET_OFFSET.
> 
> I can't reproduce this one either; but I do get different warnings:
> 
> gcc (Debian 10.2.0-13) 10.2.0, x86_64-defconfig:
> 
> defconfig-build/vmlinux.o: warning: objtool: __x86_indirect_thunk_rax() falls through to next function __x86_retpoline_rax()
> defconfig-build/vmlinux.o: warning: objtool:   .altinstr_replacement+0x1063: (branch)
> defconfig-build/vmlinux.o: warning: objtool:   __x86_indirect_thunk_rax()+0x0: (alt)
> defconfig-build/vmlinux.o: warning: objtool:   __x86_indirect_thunk_rax()+0x0: <=== (sym)
> 
> (for every single register, not just rax)
> 
> Which is daft as well, because the retpoline.o run is clean. It also
> doesn't make sense because __x86_retpoline_rax isn't in fact STT_FUNC,
> so WTH ?!

It is STT_FUNC:

  SYM_FUNC_START_NOALIGN(__x86_retpoline_\reg)

  $ readelf -s vmlinux.o |grep __x86_retpoline_rax
  129749: 0000000000000005    17 FUNC    GLOBAL DEFAULT   39 __x86_retpoline_rax
Josh Poimboeuf Oct. 21, 2020, 9:27 p.m. UTC | #14
On Wed, Oct 21, 2020 at 11:32:13AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 21, 2020 at 10:56:06AM +0200, Peter Zijlstra wrote:
> 
> > I do not see these in particular, although I do see a lot of:
> > 
> >   "sibling call from callable instruction with modified stack frame"
> 
> defconfig-build/vmlinux.o: warning: objtool: msr_write()+0x10a: sibling call from callable instruction with modified stack frame
> defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x99: (branch)
> defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x3e: (branch)
> defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x0: <=== (sym)
> 
> $ nm defconfig-build/vmlinux.o | grep msr_write
> 0000000000043250 t msr_write
> 00000000004289c0 T msr_write
> 0000000000003056 t msr_write.cold
> 
> Below 'fixes' it. So this is also caused by duplicate symbols.

There's a new linker flag for renaming duplicates:

  https://sourceware.org/bugzilla/show_bug.cgi?id=26391

But I guess that doesn't help us now.

I don't have access to GCC 10 at the moment so I can't recreate it.
Does this fix it?

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e1d7460574b..aecdf25b2381 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -217,11 +217,14 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset)
 	return NULL;
 }
 
+#define for_each_possible_sym(elf, name, sym)				\
+	elf_hash_for_each_possible(elf->symbol_name_hash, sym, name_hash, str_hash(name))
+
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name)
 {
 	struct symbol *sym;
 
-	elf_hash_for_each_possible(elf->symbol_name_hash, sym, name_hash, str_hash(name))
+	for_each_possible_sym(elf, name, sym)
 		if (!strcmp(sym->name, name))
 			return sym;
 
@@ -432,6 +435,8 @@ static int read_symbols(struct elf *elf)
 		list_for_each_entry(sym, &sec->symbol_list, list) {
 			char pname[MAX_NAME_LEN + 1];
 			size_t pnamelen;
+			struct symbol *psym;
+
 			if (sym->type != STT_FUNC)
 				continue;
 
@@ -454,8 +459,14 @@ static int read_symbols(struct elf *elf)
 
 			strncpy(pname, sym->name, pnamelen);
 			pname[pnamelen] = '\0';
-			pfunc = find_symbol_by_name(elf, pname);
-
+			pfunc = NULL;
+			for_each_possible_sym(elf, pname, psym) {
+				if ((!psym->cfunc || psym->cfunc == psym) &&
+				    !strcmp(psym->name, pname)) {
+					pfunc = psym;
+					break;
+				}
+			}
 			if (!pfunc) {
 				WARN("%s(): can't find parent function",
 				     sym->name);
Sami Tolvanen Oct. 22, 2020, 12:22 a.m. UTC | #15
On Wed, Oct 21, 2020 at 1:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 20, 2020 at 12:24:37PM -0700, Sami Tolvanen wrote:
> > > > Building allyesconfig with this series and LTO enabled, I still see
> > > > the following objtool warnings for vmlinux.o, grouped by source file:
> > > >
> > > > arch/x86/entry/entry_64.S:
> > > > __switch_to_asm()+0x0: undefined stack state
> > > > .entry.text+0xffd: sibling call from callable instruction with
> > > > modified stack frame
> > > > .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
> > >
> > > Not sure what this one's about, there's no OBJECT_FILES_NON_STANDARD?
> >
> > Correct, because with LTO, we won't have an ELF binary to process
> > until we compile everything into vmlinux.o, and at that point we can
> > no longer skip individual object files.
>
> I think what Josh was trying to say is; this file is subject to objtool
> on a normal build and does not generate warnings. So why would it
> generate warnings when subject to objtool as result of a vmlinux run
> (due to LTO or otherwise).
>
> In fact, when I build a x86_64-defconfig and then run:
>
>   $ objtool check -barf defconfig-build/vmlinux.o

Note that I'm passing also --vmlinux and --duplicate to objtool when
processing vmlinux.o, and this series has a patch to split noinstr
validation from --vmlinux, so that's skipped.

> I do not see these in particular, although I do see a lot of:
>
>   "sibling call from callable instruction with modified stack frame"
>   "falls through to next function"
>
> that did not show up in the individual objtool runs during the build.

I'm able to reproduce these warnings with gcc 9.3 + allyesconfig, with
KASAN and GCOV_KERNEL disabled, as they are not enabled in LTO builds
either. This is without the LTO series applied, so we also have the
retpoline warnings:

$ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep -vE
'(sibling|instr)'
vmlinux.o: warning: objtool: wakeup_long64()+0x61: indirect jump found
in RETPOLINE build
vmlinux.o: warning: objtool: .text+0x826308a: indirect jump found in
RETPOLINE build
vmlinux.o: warning: objtool: .text+0x82630c5: indirect jump found in
RETPOLINE build
vmlinux.o: warning: objtool: .head.text+0x748: indirect jump found in
RETPOLINE build
vmlinux.o: warning: objtool:
set_bringup_idt_handler.constprop.0()+0x0: undefined stack state
vmlinux.o: warning: objtool: .entry.text+0x1634: redundant CLD
vmlinux.o: warning: objtool: camellia_cbc_dec_32way()+0xb3: stack
state mismatch: cfa1=7+520 cfa2=7+8
vmlinux.o: warning: objtool: camellia_ctr_32way()+0x1a: stack state
mismatch: cfa1=7+520 cfa2=7+8
vmlinux.o: warning: objtool: aesni_gcm_init_avx_gen2()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_enc_update_avx_gen2()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_dec_update_avx_gen2()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_finalize_avx_gen2()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_init_avx_gen4()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_enc_update_avx_gen4()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_dec_update_avx_gen4()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: aesni_gcm_finalize_avx_gen4()+0x12:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: sha1_transform_avx2()+0xc: unsupported
stack pointer realignment
vmlinux.o: warning: objtool: sha1_ni_transform()+0x7: unsupported
stack pointer realignment
vmlinux.o: warning: objtool: sha256_transform_rorx()+0x13: unsupported
stack pointer realignment
vmlinux.o: warning: objtool: sha512_transform_ssse3()+0x14:
unsupported stack pointer realignment
vmlinux.o: warning: objtool: sha512_transform_avx()+0x14: unsupported
stack pointer realignment
vmlinux.o: warning: objtool: sha512_transform_rorx()+0x7: unsupported
stack pointer realignment
vmlinux.o: warning: objtool: __x86_retpoline_rdi()+0x10: return with
modified stack frame
vmlinux.o: warning: objtool: __x86_retpoline_rdi()+0x0: stack state
mismatch: cfa1=7+32 cfa2=7+8
vmlinux.o: warning: objtool: __x86_retpoline_rdi()+0x0: stack state
mismatch: cfa1=7+32 cfa2=-1+0
vmlinux.o: warning: objtool: reset_early_page_tables()+0x0: stack
state mismatch: cfa1=7+8 cfa2=-1+0
vmlinux.o: warning: objtool: .entry.text+0x48: stack state mismatch:
cfa1=7-8 cfa2=-1+0
vmlinux.o: warning: objtool: .entry.text+0x15fd: stack state mismatch:
cfa1=7-8 cfa2=-1+0
vmlinux.o: warning: objtool: .entry.text+0x168c: stack state mismatch:
cfa1=7-8 cfa2=-1+0

There are a couple of differences, like the first "undefined stack
state" warning pointing to set_bringup_idt_handler.constprop.0()
instead of __switch_to_asm(). I tried running this with --backtrace,
but objtool segfaults at the first .entry.text warning:

$ ./tools/objtool/objtool check -barfld vmlinux.o
...
vmlinux.o: warning: objtool:
set_bringup_idt_handler.constprop.0()+0x0: undefined stack state
vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)
...
vmlinux.o: warning: objtool: .entry.text+0xffd: sibling call from
callable instruction with modified stack frame
vmlinux.o: warning: objtool:   .entry.text+0xfcb: (branch)
Segmentation fault

Going back to the allyesconfig+LTO vmlinux.o, the "undefined stack
state" warning looks quite similar:

$ ./tools/objtool/objtool check -barlfd vmlinux.o
vmlinux.o: warning: objtool: __switch_to_asm()+0x0: undefined stack state
vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)
vmlinux.o: warning: objtool: .entry.text+0xffd: sibling call from
callable instruction with modified stack frame
vmlinux.o: warning: objtool:   .entry.text+0xfcb: (branch)
Segmentation fault

Sami
Peter Zijlstra Oct. 22, 2020, 7:25 a.m. UTC | #16
On Wed, Oct 21, 2020 at 04:27:47PM -0500, Josh Poimboeuf wrote:
> On Wed, Oct 21, 2020 at 11:32:13AM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 21, 2020 at 10:56:06AM +0200, Peter Zijlstra wrote:
> > 
> > > I do not see these in particular, although I do see a lot of:
> > > 
> > >   "sibling call from callable instruction with modified stack frame"
> > 
> > defconfig-build/vmlinux.o: warning: objtool: msr_write()+0x10a: sibling call from callable instruction with modified stack frame
> > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x99: (branch)
> > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x3e: (branch)
> > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x0: <=== (sym)
> > 
> > $ nm defconfig-build/vmlinux.o | grep msr_write
> > 0000000000043250 t msr_write
> > 00000000004289c0 T msr_write
> > 0000000000003056 t msr_write.cold
> > 
> > Below 'fixes' it. So this is also caused by duplicate symbols.
> 
> There's a new linker flag for renaming duplicates:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=26391
> 
> But I guess that doesn't help us now.

Well, depends a bit if clang can do it; we only need this for LTO builds
for now.

> I don't have access to GCC 10 at the moment so I can't recreate it.
> Does this fix it?

Doesn't seem to do the trick :/ I'll try and have a poke later.
Sami Tolvanen Oct. 23, 2020, 5:36 p.m. UTC | #17
On Wed, Oct 21, 2020 at 05:22:59PM -0700, Sami Tolvanen wrote:
> There are a couple of differences, like the first "undefined stack
> state" warning pointing to set_bringup_idt_handler.constprop.0()
> instead of __switch_to_asm(). I tried running this with --backtrace,
> but objtool segfaults at the first .entry.text warning:

Looks like it segfaults when calling BT_FUNC() for an instruction that
doesn't have a section (?). Applying this patch allows objtool to finish
with --backtrace:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c216dd4d662c..618b0c4f2890 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2604,7 +2604,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				ret = validate_branch(file, func,
 						      insn->jump_dest, state);
 				if (ret) {
-					if (backtrace)
+					if (backtrace && insn->sec)
 						BT_FUNC("(branch)", insn);
 					return ret;
 				}


Running objtool -barfld on an allyesconfig+LTO vmlinux.o prints out the
following, ignoring the crypto warnings for now:

__switch_to_asm()+0x0: undefined stack state
  xen_hypercall_set_trap_table()+0x0: <=== (sym)
.entry.text+0xffd: sibling call from callable instruction with modified stack frame
  .entry.text+0xfcb: (branch)
  .entry.text+0xfb5: (alt)
  .entry.text+0xfb0: (alt)
  .entry.text+0xf78: (branch)
  .entry.text+0x9c: (branch)
  xen_syscall_target()+0x15: (branch)
  xen_syscall_target()+0x0: <=== (sym)
.entry.text+0x1754: unsupported instruction in callable function
  .entry.text+0x171d: (branch)
  .entry.text+0x1707: (alt)
  .entry.text+0x1701: (alt)
  xen_syscall32_target()+0x15: (branch)
  xen_syscall32_target()+0x0: <=== (sym)
.entry.text+0x1634: redundant CLD
do_suspend_lowlevel()+0x116: sibling call from callable instruction with modified stack frame
  do_suspend_lowlevel()+0x9a: (branch)
  do_suspend_lowlevel()+0x0: <=== (sym)
... [skipping crypto stack pointer alignment warnings] ...
__x86_retpoline_rdi()+0x10: return with modified stack frame
  __x86_retpoline_rdi()+0x0: (branch)
  .altinstr_replacement+0x13d: (branch)
  .text+0xaf4c7: (alt)
  .text+0xb03b0: (branch)
  .text+0xaf482: (branch)
  crc_pcl()+0x10: (branch)
  crc_pcl()+0x0: <=== (sym)
__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
  .altinstr_replacement+0x20b: (branch)
  __x86_indirect_thunk_rdi()+0x0: (alt)
  __x86_indirect_thunk_rdi()+0x0: <=== (sym)
.head.text+0xfb: unsupported instruction in callable function
  .head.text+0x207: (branch)
  sev_es_play_dead()+0xff: (branch)
  sev_es_play_dead()+0xd2: (branch)
  sev_es_play_dead()+0xa8: (alt)
  sev_es_play_dead()+0x144: (branch)
  sev_es_play_dead()+0x10b: (branch)
  sev_es_play_dead()+0x1f: (branch)
  sev_es_play_dead()+0x0: <=== (sym)
__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
  .altinstr_replacement+0x107: (branch)
  .text+0x2885: (alt)
  .text+0x2860: <=== (hint)
.entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x21: (alt)
  .entry.text+0x1c: (alt)
  .entry.text+0x10: <=== (hint)
.entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x15dc: (alt)
  .entry.text+0x15d7: (alt)
  .entry.text+0x15d0: <=== (hint)
.entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x166b: (alt)
  .entry.text+0x1666: (alt)
  .entry.text+0x1660: <=== (hint)

Sami
Sami Tolvanen Oct. 23, 2020, 5:48 p.m. UTC | #18
On Thu, Oct 22, 2020 at 09:25:53AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 21, 2020 at 04:27:47PM -0500, Josh Poimboeuf wrote:
> > On Wed, Oct 21, 2020 at 11:32:13AM +0200, Peter Zijlstra wrote:
> > > On Wed, Oct 21, 2020 at 10:56:06AM +0200, Peter Zijlstra wrote:
> > > 
> > > > I do not see these in particular, although I do see a lot of:
> > > > 
> > > >   "sibling call from callable instruction with modified stack frame"
> > > 
> > > defconfig-build/vmlinux.o: warning: objtool: msr_write()+0x10a: sibling call from callable instruction with modified stack frame
> > > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x99: (branch)
> > > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x3e: (branch)
> > > defconfig-build/vmlinux.o: warning: objtool:   msr_write()+0x0: <=== (sym)
> > > 
> > > $ nm defconfig-build/vmlinux.o | grep msr_write
> > > 0000000000043250 t msr_write
> > > 00000000004289c0 T msr_write
> > > 0000000000003056 t msr_write.cold
> > > 
> > > Below 'fixes' it. So this is also caused by duplicate symbols.
> > 
> > There's a new linker flag for renaming duplicates:
> > 
> >   https://sourceware.org/bugzilla/show_bug.cgi?id=26391
> > 
> > But I guess that doesn't help us now.
> 
> Well, depends a bit if clang can do it; we only need this for LTO builds
> for now.

LLD doesn't seem to support -z unique-symbol.

Sami
Nick Desaulniers Oct. 23, 2020, 6:04 p.m. UTC | #19
On Fri, Oct 23, 2020 at 10:48 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Oct 22, 2020 at 09:25:53AM +0200, Peter Zijlstra wrote:
> > > There's a new linker flag for renaming duplicates:
> > >
> > >   https://sourceware.org/bugzilla/show_bug.cgi?id=26391
> > >
> > > But I guess that doesn't help us now.
> >
> > Well, depends a bit if clang can do it; we only need this for LTO builds
> > for now.
>
> LLD doesn't seem to support -z unique-symbol.

https://github.com/ClangBuiltLinux/linux/issues/1184
Sami Tolvanen Nov. 9, 2020, 11:11 p.m. UTC | #20
On Fri, Oct 23, 2020 at 10:36 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Wed, Oct 21, 2020 at 05:22:59PM -0700, Sami Tolvanen wrote:
> > There are a couple of differences, like the first "undefined stack
> > state" warning pointing to set_bringup_idt_handler.constprop.0()
> > instead of __switch_to_asm(). I tried running this with --backtrace,
> > but objtool segfaults at the first .entry.text warning:
>
> Looks like it segfaults when calling BT_FUNC() for an instruction that
> doesn't have a section (?). Applying this patch allows objtool to finish
> with --backtrace:
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index c216dd4d662c..618b0c4f2890 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2604,7 +2604,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>                                 ret = validate_branch(file, func,
>                                                       insn->jump_dest, state);
>                                 if (ret) {
> -                                       if (backtrace)
> +                                       if (backtrace && insn->sec)
>                                                 BT_FUNC("(branch)", insn);
>                                         return ret;
>                                 }
>
>
> Running objtool -barfld on an allyesconfig+LTO vmlinux.o prints out the
> following, ignoring the crypto warnings for now:

OK, I spent some time looking at these warnings and the configs needed
to reproduce them without building allyesconfig:

CONFIG_XEN

__switch_to_asm()+0x0: undefined stack state
  xen_hypercall_set_trap_table()+0x0: <=== (sym)

CONFIG_XEN_PV

.entry.text+0xffd: sibling call from callable instruction with
modified stack frame
  .entry.text+0xfcb: (branch)
  .entry.text+0xfb5: (alt)
  .entry.text+0xfb0: (alt)
  .entry.text+0xf78: (branch)
  .entry.text+0x9c: (branch)
  xen_syscall_target()+0x15: (branch)
  xen_syscall_target()+0x0: <=== (sym)
.entry.text+0x1754: unsupported instruction in callable function
  .entry.text+0x171d: (branch)
  .entry.text+0x1707: (alt)
  .entry.text+0x1701: (alt)
  xen_syscall32_target()+0x15: (branch)
  xen_syscall32_target()+0x0: <=== (sym)
.entry.text+0x1634: redundant CLD

Backtrace doesn’t print out anything useful for the “redundant CLD”
error, but it occurs when validate_branch is looking at
xen_sysenter_target.

do_suspend_lowlevel()+0x116: sibling call from callable instruction
with modified stack frame
  do_suspend_lowlevel()+0x9a: (branch)
  do_suspend_lowlevel()+0x0: <=== (sym)

.entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x21: (alt)
  .entry.text+0x1c: (alt)
  .entry.text+0x10: <=== (hint)
.entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x15dc: (alt)
  .entry.text+0x15d7: (alt)
  .entry.text+0x15d0: <=== (hint)
.entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0
  .altinstr_replacement+0xffffffffffffffff: (branch)
  .entry.text+0x166b: (alt)
  .entry.text+0x1666: (alt)
  .entry.text+0x1660: <=== (hint)

It looks like the stack state mismatch warnings can be fixed by adding
unwind hints also to entry_SYSCALL_64_after_hwframe,
entry_SYSENTER_compat_after_hwframe, and
entry_SYSCALL_compat_after_hwframe. Does that sound correct?

CONFIG_AMD_MEM_ENCRYPT

.head.text+0xfb: unsupported instruction in callable function
  .head.text+0x207: (branch)
  sev_es_play_dead()+0xff: (branch)
  sev_es_play_dead()+0xd2: (branch)
  sev_es_play_dead()+0xa8: (alt)
  sev_es_play_dead()+0x144: (branch)
  sev_es_play_dead()+0x10b: (branch)
  sev_es_play_dead()+0x1f: (branch)
  sev_es_play_dead()+0x0: <=== (sym)

This happens because sev_es_play_dead calls start_cpu0. It always has,
but objtool hasn’t been able to follow the call when processing only
sev-es.o. Any thoughts on the preferred way to fix this one?

CONFIG_CRYPTO_CRC32C_INTEL

__x86_retpoline_rdi()+0x10: return with modified stack frame
  __x86_retpoline_rdi()+0x0: (branch)
  .altinstr_replacement+0x147: (branch)
  .text+0xaf4c7: (alt)
  .text+0xb03b0: (branch)
  .text+0xaf482: (branch)
  crc_pcl()+0x10: (branch)
  crc_pcl()+0x0: <=== (sym)

__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
  .altinstr_replacement+0x265: (branch)
  __x86_indirect_thunk_rdi()+0x0: (alt)
  __x86_indirect_thunk_rdi()+0x0: <=== (sym)

This is different from the warnings in the rest of the arch/x86/crypto
code. Do we need some kind of a hint before the JMP_NOSPEC in crc_pcl?

CONFIG_FUNCTION_TRACER

__x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
  .altinstr_replacement+0x111: (branch)
  .text+0x28a5: (alt)
  .text+0x2880: <=== (hint)

This unwind hint is in return_to_handler. Removing it obviously stops
the warning and doesn’t seem to result in any other complaints from
objtool. Is this hint correct?

The remaining warnings are all “unsupported stack pointer realignment”
issues in the crypto code and can be reproduced with the following
configs:

CONFIG_CRYPTO_AES_NI_INTEL
CONFIG_CRYPTO_CAMELLIA_AESNI_AVX2_X86_64
CONFIG_CRYPTO_SHA1_SSSE3
CONFIG_CRYPTO_SHA256_SSSE3
CONFIG_CRYPTO_SHA512_SSSE3

Josh, have you had a chance to look at the crypto patches you mentioned earlier?

Sami
Josh Poimboeuf Nov. 10, 2020, 2:29 a.m. UTC | #21
On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> On Fri, Oct 23, 2020 at 10:36 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Wed, Oct 21, 2020 at 05:22:59PM -0700, Sami Tolvanen wrote:
> > > There are a couple of differences, like the first "undefined stack
> > > state" warning pointing to set_bringup_idt_handler.constprop.0()
> > > instead of __switch_to_asm(). I tried running this with --backtrace,
> > > but objtool segfaults at the first .entry.text warning:
> >
> > Looks like it segfaults when calling BT_FUNC() for an instruction that
> > doesn't have a section (?). Applying this patch allows objtool to finish
> > with --backtrace:
> >
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index c216dd4d662c..618b0c4f2890 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2604,7 +2604,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> >                                 ret = validate_branch(file, func,
> >                                                       insn->jump_dest, state);
> >                                 if (ret) {
> > -                                       if (backtrace)
> > +                                       if (backtrace && insn->sec)
> >                                                 BT_FUNC("(branch)", insn);
> >                                         return ret;
> >                                 }
> >
> >
> > Running objtool -barfld on an allyesconfig+LTO vmlinux.o prints out the
> > following, ignoring the crypto warnings for now:
> 
> OK, I spent some time looking at these warnings and the configs needed
> to reproduce them without building allyesconfig:
> 
> CONFIG_XEN
> 
> __switch_to_asm()+0x0: undefined stack state
>   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> 
> CONFIG_XEN_PV
> 
> .entry.text+0xffd: sibling call from callable instruction with
> modified stack frame
>   .entry.text+0xfcb: (branch)
>   .entry.text+0xfb5: (alt)
>   .entry.text+0xfb0: (alt)
>   .entry.text+0xf78: (branch)
>   .entry.text+0x9c: (branch)
>   xen_syscall_target()+0x15: (branch)
>   xen_syscall_target()+0x0: <=== (sym)
> .entry.text+0x1754: unsupported instruction in callable function
>   .entry.text+0x171d: (branch)
>   .entry.text+0x1707: (alt)
>   .entry.text+0x1701: (alt)
>   xen_syscall32_target()+0x15: (branch)
>   xen_syscall32_target()+0x0: <=== (sym)
> .entry.text+0x1634: redundant CLD
> 
> Backtrace doesn’t print out anything useful for the “redundant CLD”
> error, but it occurs when validate_branch is looking at
> xen_sysenter_target.
> 
> do_suspend_lowlevel()+0x116: sibling call from callable instruction
> with modified stack frame
>   do_suspend_lowlevel()+0x9a: (branch)
>   do_suspend_lowlevel()+0x0: <=== (sym)
> 
> .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x21: (alt)
>   .entry.text+0x1c: (alt)
>   .entry.text+0x10: <=== (hint)
> .entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x15dc: (alt)
>   .entry.text+0x15d7: (alt)
>   .entry.text+0x15d0: <=== (hint)
> .entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x166b: (alt)
>   .entry.text+0x1666: (alt)
>   .entry.text+0x1660: <=== (hint)

I can't make much sense of most of these warnings.  Disassembly would
help.

(Also, something like the patch below should help objtool show more
symbol names.)

> It looks like the stack state mismatch warnings can be fixed by adding
> unwind hints also to entry_SYSCALL_64_after_hwframe,
> entry_SYSENTER_compat_after_hwframe, and
> entry_SYSCALL_compat_after_hwframe. Does that sound correct?

No, those code paths should already have the hints they need, unless I'm
missing something.

> CONFIG_AMD_MEM_ENCRYPT
> 
> .head.text+0xfb: unsupported instruction in callable function
>   .head.text+0x207: (branch)
>   sev_es_play_dead()+0xff: (branch)
>   sev_es_play_dead()+0xd2: (branch)
>   sev_es_play_dead()+0xa8: (alt)
>   sev_es_play_dead()+0x144: (branch)
>   sev_es_play_dead()+0x10b: (branch)
>   sev_es_play_dead()+0x1f: (branch)
>   sev_es_play_dead()+0x0: <=== (sym)
> 
> This happens because sev_es_play_dead calls start_cpu0. It always has,
> but objtool hasn’t been able to follow the call when processing only
> sev-es.o. Any thoughts on the preferred way to fix this one?

Objtool isn't supposed to traverse through call instructions like that.
Is LTO inlining the call or something?

> CONFIG_CRYPTO_CRC32C_INTEL
> 
> __x86_retpoline_rdi()+0x10: return with modified stack frame
>   __x86_retpoline_rdi()+0x0: (branch)
>   .altinstr_replacement+0x147: (branch)
>   .text+0xaf4c7: (alt)
>   .text+0xb03b0: (branch)
>   .text+0xaf482: (branch)
>   crc_pcl()+0x10: (branch)
>   crc_pcl()+0x0: <=== (sym)
> 
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
>   .altinstr_replacement+0x265: (branch)
>   __x86_indirect_thunk_rdi()+0x0: (alt)
>   __x86_indirect_thunk_rdi()+0x0: <=== (sym)
> 
> This is different from the warnings in the rest of the arch/x86/crypto
> code. Do we need some kind of a hint before the JMP_NOSPEC in crc_pcl?

I'll need to look more into that one.

> CONFIG_FUNCTION_TRACER
> 
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
>   .altinstr_replacement+0x111: (branch)
>   .text+0x28a5: (alt)
>   .text+0x2880: <=== (hint)
> 
> This unwind hint is in return_to_handler. Removing it obviously stops
> the warning and doesn’t seem to result in any other complaints from
> objtool. Is this hint correct?

The hint is supposed to be there.  I don't understand this one either.
Did it inline the call to ftrace_return_to_handler()?

> The remaining warnings are all “unsupported stack pointer realignment”
> issues in the crypto code and can be reproduced with the following
> configs:
> 
> CONFIG_CRYPTO_AES_NI_INTEL
> CONFIG_CRYPTO_CAMELLIA_AESNI_AVX2_X86_64
> CONFIG_CRYPTO_SHA1_SSSE3
> CONFIG_CRYPTO_SHA256_SSSE3
> CONFIG_CRYPTO_SHA512_SSSE3
> 
> Josh, have you had a chance to look at the crypto patches you mentioned earlier?

I've been traveling for several weeks, but now my work schedule is
getting more normal, so I'll hopefully be able to spend time on this.

How would I recreate all these warnings?

Is it

  https://github.com/samitolvanen/linux.git lto-v6

plus a certain version of clang?

Also, any details on how to build clang would be appreciated, it's been
a while since I tried.


Here's the patch for hopefully making the warnings more helpful:


diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..e5f5cb107664 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2432,6 +2432,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 	sec = insn->sec;
 
 	while (1) {
+
+		if (insn->offset == 0x48)
+			WARN_FUNC("yo", sec, insn->offset);
 		next_insn = next_insn_same_sec(file, insn);
 
 		if (file->c_file && func && insn->func && func != insn->func->pfunc) {
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e1d7460574b..ced7e4754cba 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -217,6 +217,21 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset)
 	return NULL;
 }
 
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset)
+{
+	struct symbol *sym;
+
+	/*
+	 * This is slow, but used for warning messages.
+	 */
+	while (1) {
+		sym = find_symbol_by_offset(sec, offset);
+		if (sym || !offset)
+			return sym;
+		offset--;
+	}
+}
+
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name)
 {
 	struct symbol *sym;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 807f8c670097..841902ed381e 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -136,10 +136,11 @@ struct symbol *find_func_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name);
 struct symbol *find_symbol_containing(const struct section *sec, unsigned long offset);
+struct symbol *find_func_containing(struct section *sec, unsigned long offset);
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest(const struct elf *elf, struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
 				     unsigned long offset, unsigned int len);
-struct symbol *find_func_containing(struct section *sec, unsigned long offset);
 int elf_rebuild_reloc_section(struct elf *elf, struct section *sec);
 
 #define for_each_sec(file, sec)						\
diff --git a/tools/objtool/warn.h b/tools/objtool/warn.h
index 7799f60de80a..33da0f2ed9d5 100644
--- a/tools/objtool/warn.h
+++ b/tools/objtool/warn.h
@@ -22,6 +22,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
 	unsigned long name_off;
 
 	func = find_func_containing(sec, offset);
+	if (!func)
+		func = find_symbol_preceding(sec, offset);
 	if (func) {
 		name = func->name;
 		name_off = offset - func->offset;
Nick Desaulniers Nov. 10, 2020, 3:18 a.m. UTC | #22
On Mon, Nov 9, 2020 at 6:29 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Also, any details on how to build clang would be appreciated, it's been
> a while since I tried.

$ git clone https://github.com/llvm/llvm-project.git --depth 1
$ mkdir llvm-project/llvm/build
$ cd !$
$ cmake .. -DCMAKE_BUILD_TYPE=Release -G Ninja
-DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt"
$ ninja
$ export PATH=$(pwd)/bin:$PATH
$ clang --version
Sami Tolvanen Nov. 10, 2020, 4:48 a.m. UTC | #23
On Mon, Nov 9, 2020 at 6:29 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> How would I recreate all these warnings?

You can reproduce all of these using a normal gcc build without any of
the LTO patches by running objtool check -arfld vmlinux.o. However,
with gcc you'll see even more warnings due to duplicate symbol names,
as Peter pointed out elsewhere in the thread, so I looked at only the
warnings that objtool also prints with LTO.

Note that the LTO series contains a patch to split noinstr validation
from --vmlinux, as we need to run objtool here even if
CONFIG_VMLINUX_VALIDATION isn't selected, so I have not looked at the
noinstr warnings. The latest LTO tree is available here:

https://github.com/samitolvanen/linux/commits/clang-lto

> Here's the patch for hopefully making the warnings more helpful:

Thanks, I'll give it a try.

Sami
Josh Poimboeuf Nov. 10, 2020, 4:11 p.m. UTC | #24
On Mon, Nov 09, 2020 at 08:48:01PM -0800, Sami Tolvanen wrote:
> On Mon, Nov 9, 2020 at 6:29 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > How would I recreate all these warnings?
> 
> You can reproduce all of these using a normal gcc build without any of
> the LTO patches by running objtool check -arfld vmlinux.o. However,
> with gcc you'll see even more warnings due to duplicate symbol names,
> as Peter pointed out elsewhere in the thread, so I looked at only the
> warnings that objtool also prints with LTO.
> 
> Note that the LTO series contains a patch to split noinstr validation
> from --vmlinux, as we need to run objtool here even if
> CONFIG_VMLINUX_VALIDATION isn't selected, so I have not looked at the
> noinstr warnings. The latest LTO tree is available here:
> 
> https://github.com/samitolvanen/linux/commits/clang-lto
> 
> > Here's the patch for hopefully making the warnings more helpful:
> 
> Thanks, I'll give it a try.

Here's the version without the nonsensical debug warning :-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e1d7460574b..ced7e4754cba 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -217,6 +217,21 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset)
 	return NULL;
 }
 
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset)
+{
+	struct symbol *sym;
+
+	/*
+	 * This is slow, but used for warning messages.
+	 */
+	while (1) {
+		sym = find_symbol_by_offset(sec, offset);
+		if (sym || !offset)
+			return sym;
+		offset--;
+	}
+}
+
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name)
 {
 	struct symbol *sym;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 807f8c670097..841902ed381e 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -136,10 +136,11 @@ struct symbol *find_func_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name);
 struct symbol *find_symbol_containing(const struct section *sec, unsigned long offset);
+struct symbol *find_func_containing(struct section *sec, unsigned long offset);
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest(const struct elf *elf, struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
 				     unsigned long offset, unsigned int len);
-struct symbol *find_func_containing(struct section *sec, unsigned long offset);
 int elf_rebuild_reloc_section(struct elf *elf, struct section *sec);
 
 #define for_each_sec(file, sec)						\
diff --git a/tools/objtool/warn.h b/tools/objtool/warn.h
index 7799f60de80a..33da0f2ed9d5 100644
--- a/tools/objtool/warn.h
+++ b/tools/objtool/warn.h
@@ -22,6 +22,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
 	unsigned long name_off;
 
 	func = find_func_containing(sec, offset);
+	if (!func)
+		func = find_symbol_preceding(sec, offset);
 	if (func) {
 		name = func->name;
 		name_off = offset - func->offset;
Josh Poimboeuf Nov. 10, 2020, 5:46 p.m. UTC | #25
On Mon, Nov 09, 2020 at 08:29:24PM -0600, Josh Poimboeuf wrote:
> On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> > CONFIG_XEN
> > 
> > __switch_to_asm()+0x0: undefined stack state
> >   xen_hypercall_set_trap_table()+0x0: <=== (sym)

With your branch + GCC 9 I can recreate all the warnings except this
one.

Will do some digging on the others...
Sami Tolvanen Nov. 10, 2020, 6:59 p.m. UTC | #26
On Tue, Nov 10, 2020 at 9:46 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Nov 09, 2020 at 08:29:24PM -0600, Josh Poimboeuf wrote:
> > On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> > > CONFIG_XEN
> > >
> > > __switch_to_asm()+0x0: undefined stack state
> > >   xen_hypercall_set_trap_table()+0x0: <=== (sym)
>
> With your branch + GCC 9 I can recreate all the warnings except this
> one.

In a gcc build this warning is replaced with a different one:

vmlinux.o: warning: objtool: __startup_secondary_64()+0x7: return with
modified stack frame

This just seems to depend on which function is placed right after the
code in xen-head.S. With gcc, the disassembly looks like this:

0000000000000000 <asm_cpu_bringup_and_idle>:
       0:       e8 00 00 00 00          callq  5 <asm_cpu_bringup_and_idle+0x5>
                        1: R_X86_64_PLT32       cpu_bringup_and_idle-0x4
       5:       e9 f6 0f 00 00          jmpq   1000
<xen_hypercall_set_trap_table>
...
0000000000001000 <xen_hypercall_set_trap_table>:
        ...
...
0000000000002000 <__startup_secondary_64>:

With Clang+LTO, we end up with __switch_to_asm here instead of
__startup_secondary_64.

> Will do some digging on the others...

Thanks!

Sami
Josh Poimboeuf Nov. 13, 2020, 7:54 p.m. UTC | #27
On Tue, Nov 10, 2020 at 10:59:55AM -0800, Sami Tolvanen wrote:
> On Tue, Nov 10, 2020 at 9:46 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Mon, Nov 09, 2020 at 08:29:24PM -0600, Josh Poimboeuf wrote:
> > > On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> > > > CONFIG_XEN
> > > >
> > > > __switch_to_asm()+0x0: undefined stack state
> > > >   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> >
> > With your branch + GCC 9 I can recreate all the warnings except this
> > one.
> 
> In a gcc build this warning is replaced with a different one:
> 
> vmlinux.o: warning: objtool: __startup_secondary_64()+0x7: return with
> modified stack frame
> 
> This just seems to depend on which function is placed right after the
> code in xen-head.S. With gcc, the disassembly looks like this:
> 
> 0000000000000000 <asm_cpu_bringup_and_idle>:
>        0:       e8 00 00 00 00          callq  5 <asm_cpu_bringup_and_idle+0x5>
>                         1: R_X86_64_PLT32       cpu_bringup_and_idle-0x4
>        5:       e9 f6 0f 00 00          jmpq   1000
> <xen_hypercall_set_trap_table>
> ...
> 0000000000001000 <xen_hypercall_set_trap_table>:
>         ...
> ...
> 0000000000002000 <__startup_secondary_64>:
> 
> With Clang+LTO, we end up with __switch_to_asm here instead of
> __startup_secondary_64.

I still don't see this warning for some reason.

Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
tools/objtool/check.c?
Sami Tolvanen Nov. 13, 2020, 8:24 p.m. UTC | #28
On Fri, Nov 13, 2020 at 11:54 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Tue, Nov 10, 2020 at 10:59:55AM -0800, Sami Tolvanen wrote:
> > On Tue, Nov 10, 2020 at 9:46 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Mon, Nov 09, 2020 at 08:29:24PM -0600, Josh Poimboeuf wrote:
> > > > On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> > > > > CONFIG_XEN
> > > > >
> > > > > __switch_to_asm()+0x0: undefined stack state
> > > > >   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> > >
> > > With your branch + GCC 9 I can recreate all the warnings except this
> > > one.
> >
> > In a gcc build this warning is replaced with a different one:
> >
> > vmlinux.o: warning: objtool: __startup_secondary_64()+0x7: return with
> > modified stack frame
> >
> > This just seems to depend on which function is placed right after the
> > code in xen-head.S. With gcc, the disassembly looks like this:
> >
> > 0000000000000000 <asm_cpu_bringup_and_idle>:
> >        0:       e8 00 00 00 00          callq  5 <asm_cpu_bringup_and_idle+0x5>
> >                         1: R_X86_64_PLT32       cpu_bringup_and_idle-0x4
> >        5:       e9 f6 0f 00 00          jmpq   1000
> > <xen_hypercall_set_trap_table>
> > ...
> > 0000000000001000 <xen_hypercall_set_trap_table>:
> >         ...
> > ...
> > 0000000000002000 <__startup_secondary_64>:
> >
> > With Clang+LTO, we end up with __switch_to_asm here instead of
> > __startup_secondary_64.
>
> I still don't see this warning for some reason.

Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:

$ git rev-parse HEAD
585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
$ make defconfig && \
./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
make olddefconfig && \
make -j110
...
$ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
modified stack frame

> Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
> tools/objtool/check.c?

No, that didn't fix the warning. Here's what I tested:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..f1f65f5688cf 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -156,6 +156,7 @@ static bool __dead_end_function(struct
objtool_file *file, struct symbol *func,
                "machine_real_restart",
                "rewind_stack_do_exit",
                "kunit_try_catch_throw",
+               "cpu_bringup_and_idle",
        };

        if (!func)

Sami
Josh Poimboeuf Nov. 13, 2020, 8:52 p.m. UTC | #29
On Fri, Nov 13, 2020 at 12:24:32PM -0800, Sami Tolvanen wrote:
> On Fri, Nov 13, 2020 at 11:54 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 10:59:55AM -0800, Sami Tolvanen wrote:
> > > On Tue, Nov 10, 2020 at 9:46 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >
> > > > On Mon, Nov 09, 2020 at 08:29:24PM -0600, Josh Poimboeuf wrote:
> > > > > On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> > > > > > CONFIG_XEN
> > > > > >
> > > > > > __switch_to_asm()+0x0: undefined stack state
> > > > > >   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> > > >
> > > > With your branch + GCC 9 I can recreate all the warnings except this
> > > > one.
> > >
> > > In a gcc build this warning is replaced with a different one:
> > >
> > > vmlinux.o: warning: objtool: __startup_secondary_64()+0x7: return with
> > > modified stack frame
> > >
> > > This just seems to depend on which function is placed right after the
> > > code in xen-head.S. With gcc, the disassembly looks like this:
> > >
> > > 0000000000000000 <asm_cpu_bringup_and_idle>:
> > >        0:       e8 00 00 00 00          callq  5 <asm_cpu_bringup_and_idle+0x5>
> > >                         1: R_X86_64_PLT32       cpu_bringup_and_idle-0x4
> > >        5:       e9 f6 0f 00 00          jmpq   1000
> > > <xen_hypercall_set_trap_table>
> > > ...
> > > 0000000000001000 <xen_hypercall_set_trap_table>:
> > >         ...
> > > ...
> > > 0000000000002000 <__startup_secondary_64>:
> > >
> > > With Clang+LTO, we end up with __switch_to_asm here instead of
> > > __startup_secondary_64.
> >
> > I still don't see this warning for some reason.
> 
> Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:
> 
> $ git rev-parse HEAD
> 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> $ make defconfig && \
> ./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
> make olddefconfig && \
> make -j110
> ...
> $ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
> vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
> modified stack frame

Ok, I see it now on Linus' tree.  I just didn't see it on your clang-lto
branch.
Josh Poimboeuf Nov. 13, 2020, 10:34 p.m. UTC | #30
On Fri, Nov 13, 2020 at 12:24:32PM -0800, Sami Tolvanen wrote:
> > I still don't see this warning for some reason.
> 
> Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:
> 
> $ git rev-parse HEAD
> 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> $ make defconfig && \
> ./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
> make olddefconfig && \
> make -j110
> ...
> $ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
> vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
> modified stack frame
> 
> > Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
> > tools/objtool/check.c?
> 
> No, that didn't fix the warning. Here's what I tested:

I think this fixes it:

From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] x86/xen: Fix objtool vmlinux.o validation of xen hypercalls

Objtool vmlinux.o validation is showing warnings like the following:

  # tools/objtool/objtool check -barfld vmlinux.o
  vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with modified stack frame
  vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)

Objtool falls through all the empty hypercall text and gets confused
when it encounters the first real function afterwards.  The empty unwind
hints in the hypercalls aren't working for some reason.  Replace them
with a more straightforward use of STACK_FRAME_NON_STANDARD.

Reported-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/xen/xen-head.S | 9 ++++-----
 include/linux/objtool.h | 8 ++++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 2d7c8f34f56c..3c538b1ff4a6 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -6,6 +6,7 @@
 
 #include <linux/elfnote.h>
 #include <linux/init.h>
+#include <linux/objtool.h>
 
 #include <asm/boot.h>
 #include <asm/asm.h>
@@ -67,14 +68,12 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
 .pushsection .text
 	.balign PAGE_SIZE
 SYM_CODE_START(hypercall_page)
-	.rept (PAGE_SIZE / 32)
-		UNWIND_HINT_EMPTY
-		.skip 32
-	.endr
+	.skip PAGE_SIZE
 
 #define HYPERCALL(n) \
 	.equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
-	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
+	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32; \
+	STACK_FRAME_NON_STANDARD xen_hypercall_##n
 #include <asm/xen-hypercalls.h>
 #undef HYPERCALL
 SYM_CODE_END(hypercall_page)
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 577f51436cf9..746617265236 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -109,6 +109,12 @@ struct unwind_hint {
 	.popsection
 .endm
 
+.macro STACK_FRAME_NON_STANDARD func:req
+	.pushsection .discard.func_stack_frame_non_standard
+		.long \func - .
+	.popsection
+.endm
+
 #endif /* __ASSEMBLY__ */
 
 #else /* !CONFIG_STACK_VALIDATION */
@@ -123,6 +129,8 @@ struct unwind_hint {
 .macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
 .endm
 #endif
+.macro STACK_FRAME_NON_STANDARD func:req
+.endm
 
 #endif /* CONFIG_STACK_VALIDATION */
Sami Tolvanen Nov. 13, 2020, 10:54 p.m. UTC | #31
On Fri, Nov 13, 2020 at 2:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Nov 13, 2020 at 12:24:32PM -0800, Sami Tolvanen wrote:
> > > I still don't see this warning for some reason.
> >
> > Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:
> >
> > $ git rev-parse HEAD
> > 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> > $ make defconfig && \
> > ./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
> > make olddefconfig && \
> > make -j110
> > ...
> > $ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
> > vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
> > modified stack frame
> >
> > > Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
> > > tools/objtool/check.c?
> >
> > No, that didn't fix the warning. Here's what I tested:
>
> I think this fixes it:
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] x86/xen: Fix objtool vmlinux.o validation of xen hypercalls
>
> Objtool vmlinux.o validation is showing warnings like the following:
>
>   # tools/objtool/objtool check -barfld vmlinux.o
>   vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with modified stack frame
>   vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)
>
> Objtool falls through all the empty hypercall text and gets confused
> when it encounters the first real function afterwards.  The empty unwind
> hints in the hypercalls aren't working for some reason.  Replace them
> with a more straightforward use of STACK_FRAME_NON_STANDARD.
>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/xen/xen-head.S | 9 ++++-----
>  include/linux/objtool.h | 8 ++++++++
>  2 files changed, 12 insertions(+), 5 deletions(-)

Confirmed, this fixes the warning, also in LTO builds. Thanks!

Tested-by: Sami Tolvanen <samitolvanen@google.com>

Sami
Josh Poimboeuf Nov. 13, 2020, 10:56 p.m. UTC | #32
On Fri, Nov 13, 2020 at 02:54:32PM -0800, Sami Tolvanen wrote:
> On Fri, Nov 13, 2020 at 2:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Nov 13, 2020 at 12:24:32PM -0800, Sami Tolvanen wrote:
> > > > I still don't see this warning for some reason.
> > >
> > > Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:
> > >
> > > $ git rev-parse HEAD
> > > 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> > > $ make defconfig && \
> > > ./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
> > > make olddefconfig && \
> > > make -j110
> > > ...
> > > $ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
> > > vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
> > > modified stack frame
> > >
> > > > Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
> > > > tools/objtool/check.c?
> > >
> > > No, that didn't fix the warning. Here's what I tested:
> >
> > I think this fixes it:
> >
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > Subject: [PATCH] x86/xen: Fix objtool vmlinux.o validation of xen hypercalls
> >
> > Objtool vmlinux.o validation is showing warnings like the following:
> >
> >   # tools/objtool/objtool check -barfld vmlinux.o
> >   vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with modified stack frame
> >   vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> >
> > Objtool falls through all the empty hypercall text and gets confused
> > when it encounters the first real function afterwards.  The empty unwind
> > hints in the hypercalls aren't working for some reason.  Replace them
> > with a more straightforward use of STACK_FRAME_NON_STANDARD.
> >
> > Reported-by: Sami Tolvanen <samitolvanen@google.com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/xen/xen-head.S | 9 ++++-----
> >  include/linux/objtool.h | 8 ++++++++
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> Confirmed, this fixes the warning, also in LTO builds. Thanks!
> 
> Tested-by: Sami Tolvanen <samitolvanen@google.com>

Good... I'll work through the rest of them.
Sami Tolvanen Nov. 13, 2020, 11:31 p.m. UTC | #33
On Fri, Nov 13, 2020 at 2:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Nov 13, 2020 at 12:24:32PM -0800, Sami Tolvanen wrote:
> > > I still don't see this warning for some reason.
> >
> > Do you have CONFIG_XEN enabled? I can reproduce this on ToT master as follows:
> >
> > $ git rev-parse HEAD
> > 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> > $ make defconfig && \
> > ./scripts/config -e HYPERVISOR_GUEST -e PARAVIRT -e XEN && \
> > make olddefconfig && \
> > make -j110
> > ...
> > $ ./tools/objtool/objtool check -arfld vmlinux.o 2>&1 | grep secondary
> > vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with
> > modified stack frame
> >
> > > Is it fixed by adding cpu_bringup_and_idle() to global_noreturns[] in
> > > tools/objtool/check.c?
> >
> > No, that didn't fix the warning. Here's what I tested:
>
> I think this fixes it:
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] x86/xen: Fix objtool vmlinux.o validation of xen hypercalls
>
> Objtool vmlinux.o validation is showing warnings like the following:
>
>   # tools/objtool/objtool check -barfld vmlinux.o
>   vmlinux.o: warning: objtool: __startup_secondary_64()+0x2: return with modified stack frame
>   vmlinux.o: warning: objtool:   xen_hypercall_set_trap_table()+0x0: <=== (sym)
>
> Objtool falls through all the empty hypercall text and gets confused
> when it encounters the first real function afterwards.  The empty unwind
> hints in the hypercalls aren't working for some reason.  Replace them
> with a more straightforward use of STACK_FRAME_NON_STANDARD.
>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/xen/xen-head.S | 9 ++++-----
>  include/linux/objtool.h | 8 ++++++++
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index 2d7c8f34f56c..3c538b1ff4a6 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -6,6 +6,7 @@
>
>  #include <linux/elfnote.h>
>  #include <linux/init.h>
> +#include <linux/objtool.h>
>
>  #include <asm/boot.h>
>  #include <asm/asm.h>
> @@ -67,14 +68,12 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
>  .pushsection .text
>         .balign PAGE_SIZE
>  SYM_CODE_START(hypercall_page)
> -       .rept (PAGE_SIZE / 32)
> -               UNWIND_HINT_EMPTY
> -               .skip 32
> -       .endr
> +       .skip PAGE_SIZE
>
>  #define HYPERCALL(n) \
>         .equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
> -       .type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
> +       .type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32; \
> +       STACK_FRAME_NON_STANDARD xen_hypercall_##n
>  #include <asm/xen-hypercalls.h>
>  #undef HYPERCALL
>  SYM_CODE_END(hypercall_page)
> diff --git a/include/linux/objtool.h b/include/linux/objtool.h
> index 577f51436cf9..746617265236 100644
> --- a/include/linux/objtool.h
> +++ b/include/linux/objtool.h
> @@ -109,6 +109,12 @@ struct unwind_hint {
>         .popsection
>  .endm
>
> +.macro STACK_FRAME_NON_STANDARD func:req
> +       .pushsection .discard.func_stack_frame_non_standard
> +               .long \func - .
> +       .popsection
> +.endm
> +
>  #endif /* __ASSEMBLY__ */
>
>  #else /* !CONFIG_STACK_VALIDATION */
> @@ -123,6 +129,8 @@ struct unwind_hint {
>  .macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
>  .endm
>  #endif
> +.macro STACK_FRAME_NON_STANDARD func:req
> +.endm

This macro needs to be before the #endif, so it's defined only for
assembly code. This breaks my arm64 builds even though x86 curiously
worked just fine.

Sami
Josh Poimboeuf Nov. 14, 2020, 12:49 a.m. UTC | #34
On Fri, Nov 13, 2020 at 03:31:34PM -0800, Sami Tolvanen wrote:
> >  #else /* !CONFIG_STACK_VALIDATION */
> > @@ -123,6 +129,8 @@ struct unwind_hint {
> >  .macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
> >  .endm
> >  #endif
> > +.macro STACK_FRAME_NON_STANDARD func:req
> > +.endm
> 
> This macro needs to be before the #endif, so it's defined only for
> assembly code. This breaks my arm64 builds even though x86 curiously
> worked just fine.

Yeah, I noticed that after syncing objtool.h with the tools copy.  Fixed
now.

I've got fixes for some of the other warnings, but I'll queue them up
and post when they're all ready.
diff mbox series

Patch

diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index c8daa92f38dc..041e79c4e195 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S
@@ -7,6 +7,7 @@ 
 #include <asm/msr.h>
 #include <asm/asm-offsets.h>
 #include <asm/frame.h>
+#include <asm/nospec-branch.h>
 
 # Copyright 2003 Pavel Machek <pavel@suse.cz
 
@@ -39,6 +40,7 @@  SYM_FUNC_START(wakeup_long64)
 	movq	saved_rbp, %rbp
 
 	movq	saved_rip, %rax
+	ANNOTATE_RETPOLINE_SAFE
 	jmp	*%rax
 SYM_FUNC_END(wakeup_long64)
 
diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
index 43b4d864817e..640b79cc64b8 100644
--- a/arch/x86/platform/pvh/head.S
+++ b/arch/x86/platform/pvh/head.S
@@ -15,6 +15,7 @@ 
 #include <asm/asm.h>
 #include <asm/boot.h>
 #include <asm/processor-flags.h>
+#include <asm/nospec-branch.h>
 #include <asm/msr.h>
 #include <xen/interface/elfnote.h>
 
@@ -105,6 +106,7 @@  SYM_CODE_START_LOCAL(pvh_start_xen)
 	/* startup_64 expects boot_params in %rsi. */
 	mov $_pa(pvh_bootparams), %rsi
 	mov $_pa(startup_64), %rax
+	ANNOTATE_RETPOLINE_SAFE
 	jmp *%rax
 
 #else /* CONFIG_X86_64 */
diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S
index 7918b8415f13..715509d94fa3 100644
--- a/arch/x86/power/hibernate_asm_64.S
+++ b/arch/x86/power/hibernate_asm_64.S
@@ -21,6 +21,7 @@ 
 #include <asm/asm-offsets.h>
 #include <asm/processor-flags.h>
 #include <asm/frame.h>
+#include <asm/nospec-branch.h>
 
 SYM_FUNC_START(swsusp_arch_suspend)
 	movq	$saved_context, %rax
@@ -66,6 +67,7 @@  SYM_CODE_START(restore_image)
 
 	/* jump to relocated restore code */
 	movq	relocated_restore_code(%rip), %rcx
+	ANNOTATE_RETPOLINE_SAFE
 	jmpq	*%rcx
 SYM_CODE_END(restore_image)
 
@@ -97,6 +99,7 @@  SYM_CODE_START(core_restore_code)
 
 .Ldone:
 	/* jump to the restore_registers address from the image header */
+	ANNOTATE_RETPOLINE_SAFE
 	jmpq	*%r8
 SYM_CODE_END(core_restore_code)