| Message ID | 20201013003203.4168817-23-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Clang LTO | expand |
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?
+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.
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.
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.
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
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.
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
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.
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.
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;
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 ?!
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
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
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);
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
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.
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
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
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
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
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;
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
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
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;
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...
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
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?
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
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.
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 */
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
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.
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
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 --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)
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(+)