[PATCH] x86/sev: Mark snp_abort() noreturn

[PATCH] x86/sev: Mark snp_abort() noreturn

[Borislav Petkov]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Peter Zijlstra]

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.

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Borislav Petkov]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Peter Zijlstra]

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

[PATCH v2] x86/sev: Mark snp_abort() noreturn

[Borislav Petkov]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Segher Boessenkool]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Borislav Petkov]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Nick Desaulniers]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Segher Boessenkool]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Peter Zijlstra]

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.

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Segher Boessenkool]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Peter Zijlstra]

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.

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Michael Matz]

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.

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[Segher Boessenkool]

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

RE: [PATCH] x86/sev: Mark snp_abort() noreturn

[David Laight]

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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[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

Re: [PATCH] x86/sev: Mark snp_abort() noreturn

[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