* [PATCH] x86/sev: Mark snp_abort() noreturn @ 2022-08-24 15:24 Borislav Petkov 2022-08-24 15:38 ` Peter Zijlstra 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool 0 siblings, 2 replies; 17+ messages in thread From: Borislav Petkov @ 2022-08-24 15:24 UTC (permalink / raw) To: X86 ML; +Cc: Michael Matz, linux-toolchains, LKML From: Borislav Petkov <bp@suse.de> Mark both the function prototype and definition as noreturn in order to prevent the compiler from doing transformations which confuse objtool like so: vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction This triggers with gcc-12. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/include/asm/sev.h | 2 +- arch/x86/kernel/sev.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 4a23e52fe0ee..ebc271bb6d8e 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -195,7 +195,7 @@ void snp_set_memory_shared(unsigned long vaddr, unsigned int npages); void snp_set_memory_private(unsigned long vaddr, unsigned int npages); void snp_set_wakeup_secondary_cpu(void); bool snp_init(struct boot_params *bp); -void snp_abort(void); +void __init __noreturn snp_abort(void); int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 4f84c3f11af5..a428c62330d3 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2112,7 +2112,7 @@ bool __init snp_init(struct boot_params *bp) return true; } -void __init snp_abort(void) +void __init __noreturn snp_abort(void) { sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); } -- 2.35.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 15:24 [PATCH] x86/sev: Mark snp_abort() noreturn Borislav Petkov @ 2022-08-24 15:38 ` Peter Zijlstra 2022-08-24 15:48 ` Borislav Petkov 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool 1 sibling, 1 reply; 17+ messages in thread From: Peter Zijlstra @ 2022-08-24 15:38 UTC (permalink / raw) To: Borislav Petkov Cc: X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > Mark both the function prototype and definition as noreturn in order to > prevent the compiler from doing transformations which confuse objtool > like so: > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction > > This triggers with gcc-12. You'll probably also have to add it to the objtool/check.c:global_noreturn[] list, for the case where objtool fails to recognise it as noreturn. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 15:38 ` Peter Zijlstra @ 2022-08-24 15:48 ` Borislav Petkov 2022-08-24 16:16 ` Peter Zijlstra 0 siblings, 1 reply; 17+ messages in thread From: Borislav Petkov @ 2022-08-24 15:48 UTC (permalink / raw) To: Peter Zijlstra Cc: X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 05:38:26PM +0200, Peter Zijlstra wrote: > You'll probably also have to add it to the > objtool/check.c:global_noreturn[] list, for the case where objtool fails > to recognise it as noreturn. How come? Doesn't objtool see the attribute? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 15:48 ` Borislav Petkov @ 2022-08-24 16:16 ` Peter Zijlstra 2022-08-24 16:43 ` [PATCH v2] " Borislav Petkov 0 siblings, 1 reply; 17+ messages in thread From: Peter Zijlstra @ 2022-08-24 16:16 UTC (permalink / raw) To: Borislav Petkov Cc: X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 05:48:56PM +0200, Borislav Petkov wrote: > On Wed, Aug 24, 2022 at 05:38:26PM +0200, Peter Zijlstra wrote: > > You'll probably also have to add it to the > > objtool/check.c:global_noreturn[] list, for the case where objtool fails > > to recognise it as noreturn. > > How come? > > Doesn't objtool see the attribute? If only there would be a trace of it left in the ELF file... ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] x86/sev: Mark snp_abort() noreturn 2022-08-24 16:16 ` Peter Zijlstra @ 2022-08-24 16:43 ` Borislav Petkov 0 siblings, 0 replies; 17+ messages in thread From: Borislav Petkov @ 2022-08-24 16:43 UTC (permalink / raw) To: Peter Zijlstra Cc: X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 06:16:34PM +0200, Peter Zijlstra wrote: > If only there would be a trace of it left in the ELF file... Ah, there's that. --- From: Borislav Petkov <bp@suse.de> Mark both the function prototype and definition as noreturn in order to prevent the compiler from doing transformations which confuse objtool like so: vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction This triggers with gcc-12. Add it and sev_es_terminate() to the objtool noreturn tracking array too. Sort it while at it. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/include/asm/sev.h | 2 +- arch/x86/kernel/sev.c | 2 +- tools/objtool/check.c | 34 ++++++++++++++++++---------------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 4a23e52fe0ee..ebc271bb6d8e 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -195,7 +195,7 @@ void snp_set_memory_shared(unsigned long vaddr, unsigned int npages); void snp_set_memory_private(unsigned long vaddr, unsigned int npages); void snp_set_wakeup_secondary_cpu(void); bool snp_init(struct boot_params *bp); -void snp_abort(void); +void __init __noreturn snp_abort(void); int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 4f84c3f11af5..a428c62330d3 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2112,7 +2112,7 @@ bool __init snp_init(struct boot_params *bp) return true; } -void __init snp_abort(void) +void __init __noreturn snp_abort(void) { sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); } diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 0cec74da7ffe..ad51689dfb41 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -162,32 +162,34 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func, /* * Unfortunately these have to be hard coded because the noreturn - * attribute isn't provided in ELF data. + * attribute isn't provided in ELF data. Keep 'em sorted. */ static const char * const global_noreturns[] = { + "__invalid_creds", + "__module_put_and_kthread_exit", + "__reiserfs_panic", "__stack_chk_fail", - "panic", + "__ubsan_handle_builtin_unreachable", + "cpu_bringup_and_idle", + "cpu_startup_entry", "do_exit", + "do_group_exit", "do_task_dead", - "kthread_exit", - "make_task_dead", - "__module_put_and_kthread_exit", + "ex_handler_msr_mce", + "fortify_panic", "kthread_complete_and_exit", - "__reiserfs_panic", + "kthread_exit", + "kunit_try_catch_throw", "lbug_with_loc", - "fortify_panic", - "usercopy_abort", "machine_real_restart", + "make_task_dead", + "panic", "rewind_stack_and_make_dead", - "kunit_try_catch_throw", - "xen_start_kernel", - "cpu_bringup_and_idle", - "do_group_exit", + "sev_es_terminate", + "snp_abort", "stop_this_cpu", - "__invalid_creds", - "cpu_startup_entry", - "__ubsan_handle_builtin_unreachable", - "ex_handler_msr_mce", + "usercopy_abort", + "xen_start_kernel", }; if (!func) -- 2.35.1 -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 15:24 [PATCH] x86/sev: Mark snp_abort() noreturn Borislav Petkov 2022-08-24 15:38 ` Peter Zijlstra @ 2022-08-24 17:29 ` Segher Boessenkool 2022-08-24 18:23 ` Borislav Petkov ` (2 more replies) 1 sibling, 3 replies; 17+ messages in thread From: Segher Boessenkool @ 2022-08-24 17:29 UTC (permalink / raw) To: Borislav Petkov; +Cc: X86 ML, Michael Matz, linux-toolchains, LKML Hi! On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > Mark both the function prototype and definition as noreturn in order to > prevent the compiler from doing transformations which confuse objtool > like so: > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction Would -Wmissing-noreturn have caught this? It sounds like you need this (and then fix all resulting warnings) to not upset objtool? It is nice to have this anyway (if there aren't a zillion false positives), but it seems objtool is very fragile. Segher ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool @ 2022-08-24 18:23 ` Borislav Petkov 2022-08-24 18:44 ` Nick Desaulniers 2022-08-24 20:45 ` Peter Zijlstra 2 siblings, 0 replies; 17+ messages in thread From: Borislav Petkov @ 2022-08-24 18:23 UTC (permalink / raw) To: Segher Boessenkool; +Cc: X86 ML, Michael Matz, linux-toolchains, LKML On Wed, Aug 24, 2022 at 12:29:29PM -0500, Segher Boessenkool wrote: > Would -Wmissing-noreturn have caught this? Yes, see below. Current linus master allmodconfig with gcc-12 (SUSE Linux) 12.1.1 20220517 [revision 325d82b08696da17fb26bd2e1b6ba607649357fb] > It sounds like you need this (and then fix all resulting warnings) to > not upset objtool? Yep. > It is nice to have this anyway (if there aren't a zillion false > positives), but it seems objtool is very fragile. Constant improvement, I'd guess. So, I'd say someone needs to sit down and fix those and then as last fix, enable the warning by default. All those use cases would need proper analysis tho. Thx. arch/x86/coco/tdx/tdx.c: In function ‘__tdx_hypercall_failed’: arch/x86/coco/tdx/tdx.c:56:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 56 | void __tdx_hypercall_failed(void) | ^~~~~~~~~~~~~~~~~~~~~~ arch/x86/hyperv/ivm.c: In function ‘hv_ghcb_terminate’: arch/x86/hyperv/ivm.c:130:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 130 | void hv_ghcb_terminate(unsigned int set, unsigned int reason) | ^~~~~~~~~~~~~~~~~ arch/x86/platform/pvh/enlighten.c: In function ‘xen_pvh_init’: arch/x86/platform/pvh/enlighten.c:104:20: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 104 | void __init __weak xen_pvh_init(struct boot_params *boot_params) | ^~~~~~~~~~~~ arch/x86/platform/pvh/enlighten.c: In function ‘mem_map_via_hcall’: arch/x86/platform/pvh/enlighten.c:39:20: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 39 | void __init __weak mem_map_via_hcall(struct boot_params *ptr __maybe_unused) | ^~~~~~~~~~~~~~~~~ arch/x86/xen/smp_pv.c: In function ‘cpu_bringup_and_idle’: arch/x86/xen/smp_pv.c:92:27: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 92 | asmlinkage __visible void cpu_bringup_and_idle(void) | ^~~~~~~~~~~~~~~~~~~~ init/main.c: In function ‘rest_init’: init/main.c:681:21: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 681 | noinline void __ref rest_init(void) | ^~~~~~~~~ In file included from ./arch/x86/include/asm/traps.h:9, from arch/x86/xen/enlighten_pv.c:61: arch/x86/xen/enlighten_pv.c: In function ‘exc_xen_unknown_trap’: arch/x86/xen/enlighten_pv.c:566:21: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 566 | DEFINE_IDTENTRY_RAW(exc_xen_unknown_trap) | ^~~~~~~~~~~~~~~~~~~~ ./arch/x86/include/asm/idtentry.h:138:24: note: in definition of macro ‘DEFINE_IDTENTRY_RAW’ 138 | __visible noinstr void func(struct pt_regs *regs) | ^~~~ init/main.c: In function ‘arch_call_rest_init’: init/main.c:881:20: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 881 | void __init __weak arch_call_rest_init(void) | ^~~~~~~~~~~~~~~~~~~ arch/x86/kernel/smpboot.c: In function ‘hlt_play_dead’: arch/x86/kernel/smpboot.c:1815:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 1815 | void hlt_play_dead(void) | ^~~~~~~~~~~~~ kernel/panic.c: In function ‘__stack_chk_fail’: kernel/panic.c:703:24: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 703 | __visible noinstr void __stack_chk_fail(void) | ^~~~~~~~~~~~~~~~ kernel/panic.c: In function ‘panic_smp_self_stop’: kernel/panic.c:112:13: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 112 | void __weak panic_smp_self_stop(void) | ^~~~~~~~~~~~~~~~~~~ arch/x86/kernel/reboot.c: In function ‘nmi_panic_self_stop’: arch/x86/kernel/reboot.c:885:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 885 | void nmi_panic_self_stop(struct pt_regs *regs) | ^~~~~~~~~~~~~~~~~~~ In file included from ./include/linux/compat.h:34, from ./arch/x86/include/asm/ftrace.h:111, from ./include/linux/ftrace.h:23, from ./include/linux/perf_event.h:51, from ./include/linux/trace_events.h:10, from ./include/trace/syscall.h:7, from ./include/linux/syscalls.h:88, from kernel/exit.c:42: kernel/exit.c: In function ‘__x64_sys_exit’: ./arch/x86/include/asm/syscall_wrapper.h:77:14: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 77 | long __##abi##_##name(const struct pt_regs *regs) \ | ^~ ./arch/x86/include/asm/syscall_wrapper.h:97:9: note: in expansion of macro ‘__SYS_STUBx’ 97 | __SYS_STUBx(x64, sys##name, \ | ^~~~~~~~~~~ ./arch/x86/include/asm/syscall_wrapper.h:231:9: note: in expansion of macro ‘__X64_SYS_STUBx’ 231 | __X64_SYS_STUBx(x, name, __VA_ARGS__) \ | ^~~~~~~~~~~~~~~ ./include/linux/syscalls.h:228:9: note: in expansion of macro ‘__SYSCALL_DEFINEx’ 228 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) | ^~~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:217:36: note: in expansion of macro ‘SYSCALL_DEFINEx’ 217 | #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__) | ^~~~~~~~~~~~~~~ kernel/exit.c:890:1: note: in expansion of macro ‘SYSCALL_DEFINE1’ 890 | SYSCALL_DEFINE1(exit, int, error_code) | ^~~~~~~~~~~~~~~ kernel/exit.c: In function ‘__ia32_sys_exit’: ./arch/x86/include/asm/syscall_wrapper.h:77:14: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 77 | long __##abi##_##name(const struct pt_regs *regs) \ | ^~ ./arch/x86/include/asm/syscall_wrapper.h:117:9: note: in expansion of macro ‘__SYS_STUBx’ 117 | __SYS_STUBx(ia32, sys##name, \ | ^~~~~~~~~~~ ./arch/x86/include/asm/syscall_wrapper.h:232:9: note: in expansion of macro ‘__IA32_SYS_STUBx’ 232 | __IA32_SYS_STUBx(x, name, __VA_ARGS__) \ | ^~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:228:9: note: in expansion of macro ‘__SYSCALL_DEFINEx’ 228 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) | ^~~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:217:36: note: in expansion of macro ‘SYSCALL_DEFINEx’ 217 | #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__) | ^~~~~~~~~~~~~~~ kernel/exit.c:890:1: note: in expansion of macro ‘SYSCALL_DEFINE1’ 890 | SYSCALL_DEFINE1(exit, int, error_code) | ^~~~~~~~~~~~~~~ kernel/exit.c: In function ‘__x64_sys_exit_group’: ./arch/x86/include/asm/syscall_wrapper.h:77:14: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 77 | long __##abi##_##name(const struct pt_regs *regs) \ | ^~ ./arch/x86/include/asm/syscall_wrapper.h:97:9: note: in expansion of macro ‘__SYS_STUBx’ 97 | __SYS_STUBx(x64, sys##name, \ | ^~~~~~~~~~~ ./arch/x86/include/asm/syscall_wrapper.h:231:9: note: in expansion of macro ‘__X64_SYS_STUBx’ 231 | __X64_SYS_STUBx(x, name, __VA_ARGS__) \ | ^~~~~~~~~~~~~~~ ./include/linux/syscalls.h:228:9: note: in expansion of macro ‘__SYSCALL_DEFINEx’ 228 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) | ^~~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:217:36: note: in expansion of macro ‘SYSCALL_DEFINEx’ 217 | #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__) | ^~~~~~~~~~~~~~~ kernel/exit.c:934:1: note: in expansion of macro ‘SYSCALL_DEFINE1’ 934 | SYSCALL_DEFINE1(exit_group, int, error_code) | ^~~~~~~~~~~~~~~ kernel/exit.c: In function ‘__ia32_sys_exit_group’: ./arch/x86/include/asm/syscall_wrapper.h:77:14: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 77 | long __##abi##_##name(const struct pt_regs *regs) \ | ^~ ./arch/x86/include/asm/syscall_wrapper.h:117:9: note: in expansion of macro ‘__SYS_STUBx’ 117 | __SYS_STUBx(ia32, sys##name, \ | ^~~~~~~~~~~ ./arch/x86/include/asm/syscall_wrapper.h:232:9: note: in expansion of macro ‘__IA32_SYS_STUBx’ 232 | __IA32_SYS_STUBx(x, name, __VA_ARGS__) \ | ^~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:228:9: note: in expansion of macro ‘__SYSCALL_DEFINEx’ 228 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__) | ^~~~~~~~~~~~~~~~~ ./include/linux/syscalls.h:217:36: note: in expansion of macro ‘SYSCALL_DEFINEx’ 217 | #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__) | ^~~~~~~~~~~~~~~ kernel/exit.c:934:1: note: in expansion of macro ‘SYSCALL_DEFINE1’ 934 | SYSCALL_DEFINE1(exit_group, int, error_code) | ^~~~~~~~~~~~~~~ arch/x86/kernel/paravirt.c: In function ‘paravirt_BUG’: arch/x86/kernel/paravirt.c:69:14: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 69 | noinstr void paravirt_BUG(void) | ^~~~~~~~~~~~ arch/x86/kernel/devicetree.c: In function ‘early_init_dt_add_memory_arch’: arch/x86/kernel/devicetree.c:34:13: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 34 | void __init early_init_dt_add_memory_arch(u64 base, u64 size) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/x86/kernel/sev.c: In function ‘snp_abort’: arch/x86/kernel/sev.c:2103:13: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 2103 | void __init snp_abort(void) | ^~~~~~~~~ kernel/kthread.c: In function ‘kthreadd’: kernel/kthread.c:717:5: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 717 | int kthreadd(void *unused) | ^~~~~~~~ net/ipv4/ping.c: In function ‘ping_hash’: net/ipv4/ping.c:144:5: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 144 | int ping_hash(struct sock *sk) | ^~~~~~~~~ crypto/jitterentropy-kcapi.c: In function ‘jent_panic’: crypto/jitterentropy-kcapi.c:62:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 62 | void jent_panic(char *s) | ^~~~~~~~~~ drivers/message/fusion/mptbase.c: In function ‘mpt_halt_firmware’: drivers/message/fusion/mptbase.c:6939:1: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 6939 | mpt_halt_firmware(MPT_ADAPTER *ioc) | ^~~~~~~~~~~~~~~~~ drivers/net/wireless/rsi/rsi_91x_sdio_ops.c: In function ‘rsi_sdio_rx_thread’: drivers/net/wireless/rsi/rsi_91x_sdio_ops.c:65:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 65 | void rsi_sdio_rx_thread(struct rsi_common *common) | ^~~~~~~~~~~~~~~~~~ drivers/net/wireless/rsi/rsi_91x_usb_ops.c: In function ‘rsi_usb_rx_thread’: drivers/net/wireless/rsi/rsi_91x_usb_ops.c:28:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 28 | void rsi_usb_rx_thread(struct rsi_common *common) | ^~~~~~~~~~~~~~~~~ drivers/clocksource/timer-ti-dm.c: In function ‘omap_dm_timer_modify_idlect_mask’: drivers/clocksource/timer-ti-dm.c:600:7: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 600 | __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/staging/media/atomisp/pci/ia_css_device_access.c: In function ‘ia_css_device_load_uint64’: drivers/staging/media/atomisp/pci/ia_css_device_access.c:51:1: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 51 | ia_css_device_load_uint64(const hrt_address addr) | ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/staging/media/atomisp/pci/ia_css_device_access.c: In function ‘ia_css_device_store_uint64’: drivers/staging/media/atomisp/pci/ia_css_device_access.c:78:1: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 78 | ia_css_device_store_uint64(const hrt_address addr, const uint64_t data) | ^~~~~~~~~~~~~~~~~~~~~~~~~~ lib/ubsan.c: In function ‘__ubsan_handle_builtin_unreachable’: lib/ubsan.c:329:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 329 | void __ubsan_handle_builtin_unreachable(void *_data) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c: In function ‘otx2_sqe_flush’: drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c:554:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 554 | void otx2_sqe_flush(void *dev, struct otx2_snd_queue *sq, | ^~~~~~~~~~~~~~ drivers/gpu/drm/radeon/r100.c: In function ‘r100_semaphore_ring_emit’: drivers/gpu/drm/radeon/r100.c:890:6: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn] 890 | bool r100_semaphore_ring_emit(struct radeon_device *rdev, | ^~~~~~~~~~~~~~~~~~~~~~~~ -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool 2022-08-24 18:23 ` Borislav Petkov @ 2022-08-24 18:44 ` Nick Desaulniers 2022-08-24 19:43 ` Segher Boessenkool 2022-08-24 20:45 ` Peter Zijlstra 2 siblings, 1 reply; 17+ messages in thread From: Nick Desaulniers @ 2022-08-24 18:44 UTC (permalink / raw) To: Segher Boessenkool Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML On Wed, Aug 24, 2022 at 10:33 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > > Mark both the function prototype and definition as noreturn in order to > > prevent the compiler from doing transformations which confuse objtool > > like so: > > > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction > > Would -Wmissing-noreturn have caught this? Oh, that's shiny. Is that relatively new? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 18:44 ` Nick Desaulniers @ 2022-08-24 19:43 ` Segher Boessenkool 0 siblings, 0 replies; 17+ messages in thread From: Segher Boessenkool @ 2022-08-24 19:43 UTC (permalink / raw) To: Nick Desaulniers Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML On Wed, Aug 24, 2022 at 11:44:35AM -0700, Nick Desaulniers wrote: > On Wed, Aug 24, 2022 at 10:33 AM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > > > Mark both the function prototype and definition as noreturn in order to > > > prevent the compiler from doing transformations which confuse objtool > > > like so: > > > > > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction > > > > Would -Wmissing-noreturn have caught this? > > Oh, that's shiny. Is that relatively new? Relatively, yes. commit 0ca3fb0a168e9db2bd5405855d3932f62b08b7c9 Author: Kaveh R. Ghazi <ghazi@caip.rutgers.edu> Date: Tue Oct 20 07:32:08 1998 +0000 New warning, `missing-noreturn': The alias -Wsuggest-attribute=noreturn is newer: commit 94ee697923f5a93881edbc5e1b3f810b290f0fc7 Author: Manuel López-Ibáñez <manu@gcc.gnu.org> Date: Sun Apr 29 09:15:30 2012 +0000 * common.opt (Wmissing-noreturn): Alias of -Wsuggest-attribute=noret Segher ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool 2022-08-24 18:23 ` Borislav Petkov 2022-08-24 18:44 ` Nick Desaulniers @ 2022-08-24 20:45 ` Peter Zijlstra 2022-08-24 22:41 ` Segher Boessenkool 2022-08-30 4:46 ` Josh Poimboeuf 2 siblings, 2 replies; 17+ messages in thread From: Peter Zijlstra @ 2022-08-24 20:45 UTC (permalink / raw) To: Segher Boessenkool Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 12:29:29PM -0500, Segher Boessenkool wrote: > Hi! > > On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > > Mark both the function prototype and definition as noreturn in order to > > prevent the compiler from doing transformations which confuse objtool > > like so: > > > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction > > Would -Wmissing-noreturn have caught this? It sounds like you need this > (and then fix all resulting warnings) to not upset objtool? > > It is nice to have this anyway (if there aren't a zillion false > positives), but it seems objtool is very fragile. Well, just like gcc has noreturn heuristics so has objtool, it just turns into pain when they don't agree with one another. Ideally noreturn would be reflected in the object file so we don't have to guess at it. STT_FUNC_NORETURN would do I suppose, except then all the tools will need to be taught how to deal with that, which is also very painful. Another options is something like .symtab.noreturn which is another symbol table explicitly listing the noreturn functions. Since it's an extra section tools that don't know about it can freely ignore it and carry on as usual. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 20:45 ` Peter Zijlstra @ 2022-08-24 22:41 ` Segher Boessenkool 2022-08-25 6:41 ` Peter Zijlstra 2022-08-30 4:46 ` Josh Poimboeuf 1 sibling, 1 reply; 17+ messages in thread From: Segher Boessenkool @ 2022-08-24 22:41 UTC (permalink / raw) To: Peter Zijlstra Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf Hi! On Wed, Aug 24, 2022 at 10:45:12PM +0200, Peter Zijlstra wrote: > On Wed, Aug 24, 2022 at 12:29:29PM -0500, Segher Boessenkool wrote: > > Would -Wmissing-noreturn have caught this? It sounds like you need this > > (and then fix all resulting warnings) to not upset objtool? > > > > It is nice to have this anyway (if there aren't a zillion false > > positives), but it seems objtool is very fragile. > > Well, just like gcc has noreturn heuristics so has objtool, it just > turns into pain when they don't agree with one another. > > Ideally noreturn would be reflected in the object file so we don't have > to guess at it. It is! A noreturn function (that doesn't warn like "warning: 'noreturn' function does return") does not have whatever your architecture uses for function returns in it. Just like most non-noreturn functions that do not return btw: the attribute affects code generation of the *caller* of such functions. > STT_FUNC_NORETURN would do I suppose, except then all > the tools will need to be taught how to deal with that, which is also > very painful. What is that? Even Google has no idea. Hrm. > Another options is something like .symtab.noreturn which is another > symbol table explicitly listing the noreturn functions. Since it's an > extra section tools that don't know about it can freely ignore it and > carry on as usual. The noreturn attribute, like any attribute, is informing the compiler about some attributes of the code it is compiling. Trying to use it at a completely different level, for a completely different purpose, will only end in tears. Say no to tears. The only thing a compiler does with it (not counting diagnostics) is it says that calls to such a function never return, are really just a jump, not a call. This allows the compiler to generate slightly faster code (smaller, and it won't realistically be in any hot path). - - - What fundamental problem does objtool have in dealing with any normal compiled code itself? Does it try to understand the semantics of the machine code (not very tractable), does it expect some magic markup to be generated together with the machine code, does it want to have compilers hamstrung wrt what kind of code they can generate? There is some serious disconnect here, and I'm not even completely sure what it is :-( Segher ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 22:41 ` Segher Boessenkool @ 2022-08-25 6:41 ` Peter Zijlstra 2022-08-25 12:29 ` Michael Matz ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Peter Zijlstra @ 2022-08-25 6:41 UTC (permalink / raw) To: Segher Boessenkool Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 05:41:44PM -0500, Segher Boessenkool wrote: > It is! A noreturn function (that doesn't warn like "warning: 'noreturn' > function does return") does not have whatever your architecture uses for > function returns in it. Just like most non-noreturn functions that do > not return btw: the attribute affects code generation of the *caller* of > such functions. Yeah, but objtool can't tell if the compiler just spazzed out and stopped generating code or if it was intentional. > > STT_FUNC_NORETURN would do I suppose, except then all > > the tools will need to be taught how to deal with that, which is also > > very painful. > > What is that? Even Google has no idea. Hrm. Something I just made up :-) A new symbol type for noreturn functions would be very useful. > What fundamental problem does objtool have in dealing with any normal > compiled code itself? Does it try to understand the semantics of the > machine code (not very tractable), does it expect some magic markup to > be generated together with the machine code, does it want to have > compilers hamstrung wrt what kind of code they can generate? > > There is some serious disconnect here, and I'm not even completely sure > what it is :-( Objtool follows control flow. As you said above, noreturn functions behave differently and code-gen after a call to a noreturn function stops. Typically objtool expects a call instruction to return and continue on the next instruction; if all of a sudden there's nothing there, it gets suspicious and says the compiler messed up. (FWIW, we've found a fair number of actual compiler bugs with this) Now, as mentioned we have heuristics that try and detect if a function is noreturn or not; but all those fail horribly if the function is in another translation unit for example. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-25 6:41 ` Peter Zijlstra @ 2022-08-25 12:29 ` Michael Matz 2022-08-25 12:58 ` Segher Boessenkool 2022-08-25 13:12 ` David Laight 2 siblings, 0 replies; 17+ messages in thread From: Michael Matz @ 2022-08-25 12:29 UTC (permalink / raw) To: Peter Zijlstra Cc: Segher Boessenkool, Borislav Petkov, X86 ML, linux-toolchains, LKML, Josh Poimboeuf Hello, On Thu, 25 Aug 2022, Peter Zijlstra wrote: > > > STT_FUNC_NORETURN would do I suppose, except then all > > > the tools will need to be taught how to deal with that, which is also > > > very painful. > > > > What is that? Even Google has no idea. Hrm. > > Something I just made up :-) A new symbol type for noreturn functions > would be very useful. But very, very, very painful for everyone else :-) And: there are only 15 symbol types, and in the OS-specific range only two are left (GNU_IFUNC is the taken one in that range). That's too few to waste it on this very specific purpose. A second symtab-like structure might work out, but then, what about attribute(frobnicate)? Or one of the other actually existing ones that influence codegen? Extending debuginfo might be an idea, but will bloat it even more. Tears everywhere. Just stay with probalistic hacking? :-) (And see below) > Now, as mentioned we have heuristics that try and detect if a function > is noreturn or not; but all those fail horribly if the function is in > another translation unit for example. You could add another mode of operation in objtool that merely scans all disassembled function bodies and emits a new list of candidate noreturn functions. You run it from time to time to see if the hard-coded list is still sensible. Ciao, Michael. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-25 6:41 ` Peter Zijlstra 2022-08-25 12:29 ` Michael Matz @ 2022-08-25 12:58 ` Segher Boessenkool 2022-08-25 13:12 ` David Laight 2 siblings, 0 replies; 17+ messages in thread From: Segher Boessenkool @ 2022-08-25 12:58 UTC (permalink / raw) To: Peter Zijlstra Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Thu, Aug 25, 2022 at 08:41:19AM +0200, Peter Zijlstra wrote: > On Wed, Aug 24, 2022 at 05:41:44PM -0500, Segher Boessenkool wrote: > > > It is! A noreturn function (that doesn't warn like "warning: 'noreturn' > > function does return") does not have whatever your architecture uses for > > function returns in it. Just like most non-noreturn functions that do > > not return btw: the attribute affects code generation of the *caller* of > > such functions. > > Yeah, but objtool can't tell if the compiler just spazzed out and > stopped generating code or if it was intentional. I don't understand what you mean. If the compiler malfunctioned, all bets are off. If not, then the compiler correctly decided the function does not return in a normal way, and it generated machine code accordingly. Or you mean something else by "stopped generating code"? > > What fundamental problem does objtool have in dealing with any normal > > compiled code itself? Does it try to understand the semantics of the > > machine code (not very tractable), does it expect some magic markup to > > be generated together with the machine code, does it want to have > > compilers hamstrung wrt what kind of code they can generate? > > > > There is some serious disconnect here, and I'm not even completely sure > > what it is :-( > > Objtool follows control flow. As you said above, noreturn functions > behave differently and code-gen after a call to a noreturn function > stops. The noreturn attribute only informs the compiler that this function is one that does not return. There are other functions that do the same. Most (or hopefully all!) functions flagged by -Wmissing-noreturn for example. You cannot require all functions that do not return have the attribute. > Typically objtool expects a call instruction to return and continue on > the next instruction; if all of a sudden there's nothing there, it gets > suspicious and says the compiler messed up. That is a shortcoming in objtool then. The fundamental problem is that you cannot really parse machine code as much as you apparently want to. And limiting yourself to "machine code a compiler would generate" breaks down if a) the compiler changes, or b) your assumptions of what compilers do or do not generate are faulty. Segher ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-25 6:41 ` Peter Zijlstra 2022-08-25 12:29 ` Michael Matz 2022-08-25 12:58 ` Segher Boessenkool @ 2022-08-25 13:12 ` David Laight 2022-08-30 4:48 ` Josh Poimboeuf 2 siblings, 1 reply; 17+ messages in thread From: David Laight @ 2022-08-25 13:12 UTC (permalink / raw) To: 'Peter Zijlstra', Segher Boessenkool Cc: Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf ... > Objtool follows control flow. As you said above, noreturn functions > behave differently and code-gen after a call to a noreturn function > stops. Thinks.... How near is objtool to outputting a list of functions and the stack offset at which they call something else? With the 'magic numbers' to track indirect jumps that is very nearly enough information to do full static stack depth checking. Recursive calls are a problem - but they ought to be banned in kernel anyway. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-25 13:12 ` David Laight @ 2022-08-30 4:48 ` Josh Poimboeuf 0 siblings, 0 replies; 17+ messages in thread From: Josh Poimboeuf @ 2022-08-30 4:48 UTC (permalink / raw) To: David Laight Cc: 'Peter Zijlstra', Segher Boessenkool, Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Thu, Aug 25, 2022 at 01:12:42PM +0000, David Laight wrote: > ... > > Objtool follows control flow. As you said above, noreturn functions > > behave differently and code-gen after a call to a noreturn function > > stops. > > Thinks.... > > How near is objtool to outputting a list of functions and the > stack offset at which they call something else? It basically already does this. > With the 'magic numbers' to track indirect jumps that is very > nearly enough information to do full static stack depth checking. What do you mean by 'magic numbers'? -- Josh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/sev: Mark snp_abort() noreturn 2022-08-24 20:45 ` Peter Zijlstra 2022-08-24 22:41 ` Segher Boessenkool @ 2022-08-30 4:46 ` Josh Poimboeuf 1 sibling, 0 replies; 17+ messages in thread From: Josh Poimboeuf @ 2022-08-30 4:46 UTC (permalink / raw) To: Peter Zijlstra Cc: Segher Boessenkool, Borislav Petkov, X86 ML, Michael Matz, linux-toolchains, LKML, Josh Poimboeuf On Wed, Aug 24, 2022 at 10:45:12PM +0200, Peter Zijlstra wrote: > On Wed, Aug 24, 2022 at 12:29:29PM -0500, Segher Boessenkool wrote: > > Hi! > > > > On Wed, Aug 24, 2022 at 05:24:20PM +0200, Borislav Petkov wrote: > > > Mark both the function prototype and definition as noreturn in order to > > > prevent the compiler from doing transformations which confuse objtool > > > like so: > > > > > > vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction > > > > Would -Wmissing-noreturn have caught this? It sounds like you need this > > (and then fix all resulting warnings) to not upset objtool? > > > > It is nice to have this anyway (if there aren't a zillion false > > positives), but it seems objtool is very fragile. > > Well, just like gcc has noreturn heuristics so has objtool, it just > turns into pain when they don't agree with one another. > > Ideally noreturn would be reflected in the object file so we don't have > to guess at it. STT_FUNC_NORETURN would do I suppose, except then all > the tools will need to be taught how to deal with that, which is also > very painful. > > Another options is something like .symtab.noreturn which is another > symbol table explicitly listing the noreturn functions. Since it's an > extra section tools that don't know about it can freely ignore it and > carry on as usual. We're planning to talk about this at the LPC toolchains microconference. My proposal is similar to yours except I called it .annotate.noreturn. It would be enabled with a --annotate=noreturn compiler option. It would report both explicit noreturns (with the "noreturn" function attribute) and implicit noreturns (static functions which only call other noreturn functions). There would also be an --annotate=jump_table option which creates an .annotate.jump_table to describe switch statement jump table flow. Those are the two biggest challenges for objtool. -- Josh ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-30 4:48 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-24 15:24 [PATCH] x86/sev: Mark snp_abort() noreturn Borislav Petkov 2022-08-24 15:38 ` Peter Zijlstra 2022-08-24 15:48 ` Borislav Petkov 2022-08-24 16:16 ` Peter Zijlstra 2022-08-24 16:43 ` [PATCH v2] " Borislav Petkov 2022-08-24 17:29 ` [PATCH] " Segher Boessenkool 2022-08-24 18:23 ` Borislav Petkov 2022-08-24 18:44 ` Nick Desaulniers 2022-08-24 19:43 ` Segher Boessenkool 2022-08-24 20:45 ` Peter Zijlstra 2022-08-24 22:41 ` Segher Boessenkool 2022-08-25 6:41 ` Peter Zijlstra 2022-08-25 12:29 ` Michael Matz 2022-08-25 12:58 ` Segher Boessenkool 2022-08-25 13:12 ` David Laight 2022-08-30 4:48 ` Josh Poimboeuf 2022-08-30 4:46 ` Josh Poimboeuf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).