diff mbox series

[02/11] init: Mark start_kernel() __noreturn

Message ID cb5dab6038dfe5156f5d68424cf372f7eed1b934.1680912057.git.jpoimboe@kernel.org (mailing list archive)
State New, archived
Headers show
Series Sprinkle more __noreturn | expand

Commit Message

Josh Poimboeuf April 8, 2023, 12:09 a.m. UTC
Fixes the following warning:

  vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 include/linux/start_kernel.h | 2 +-
 init/main.c                  | 2 +-
 tools/objtool/check.c        | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

Comments

Nick Desaulniers April 12, 2023, 8:29 p.m. UTC | #1
On Fri, Apr 07, 2023 at 05:09:55PM -0700, Josh Poimboeuf wrote:
> Fixes the following warning:
> 
>   vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>

Ah, I just realized that my series will conflict with this.
https://lore.kernel.org/llvm/20230412-no_stackp-v1-1-46a69b507a4b@google.com/
Perhaps if my series gets positive feedback; I can rebase it on top of
this and it can become part of your series?

For this patch,
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Though I'm curious, it does look like it's necessary because of 01/11 in
the series? Any idea how the 0day bot report happened before 1/11
existed?

(Surely gcc isn't assuming a weak function is implicitly noreturn and
make optimizations based on that (that's one hazard I'm worried about)?)

It looks like perhaps the link to
https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/
on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your
kernel tree
https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn
, which had 1/11 in it, IIUC?  Perhaps this link should go on 1/11
rather than 2/11?

Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day
reports.  Are you able to flesh out more info how/what/when such objtool
warnings are observed?  Are the warnings ever results of patches earlier
in the series?
Josh Poimboeuf April 12, 2023, 9:57 p.m. UTC | #2
On Wed, Apr 12, 2023 at 01:29:49PM -0700, Nick Desaulniers wrote:
> On Fri, Apr 07, 2023 at 05:09:55PM -0700, Josh Poimboeuf wrote:
> > Fixes the following warning:
> > 
> >   vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/
> > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> 
> Ah, I just realized that my series will conflict with this.
> https://lore.kernel.org/llvm/20230412-no_stackp-v1-1-46a69b507a4b@google.com/
> Perhaps if my series gets positive feedback; I can rebase it on top of
> this and it can become part of your series?

Sure, I can take these on top.

> For this patch,
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks!

> Though I'm curious, it does look like it's necessary because of 01/11 in
> the series? Any idea how the 0day bot report happened before 1/11
> existed?
>
> 
> (Surely gcc isn't assuming a weak function is implicitly noreturn and
> make optimizations based on that (that's one hazard I'm worried about)?)

As far as I can tell, GCC has been doing the right thing here, and it's
instead been objtool getting confused by weak noreturns.  That gets
fixed later in patch 9.

> It looks like perhaps the link to
> https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/
> on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your
> kernel tree
> https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn
> , which had 1/11 in it, IIUC?  Perhaps this link should go on 1/11
> rather than 2/11?

Good catch, patch 1 does introduce the warning.  I think I'll just
squash patches 1 and 2 so as not to break bisection.

> Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day
> reports.  Are you able to flesh out more info how/what/when such objtool
> warnings are observed?  Are the warnings ever results of patches earlier
> in the series?

Hopefully not, it's best to not introduce warnings even temporarily.  I
was doing a lot of build testing at the time with various branches, so
it's possible.  I'll see if I can figure out how I triggered those
warnings and document that in the commit logs if possible.
Josh Poimboeuf April 12, 2023, 11:26 p.m. UTC | #3
On Wed, Apr 12, 2023 at 02:57:57PM -0700, Josh Poimboeuf wrote:
> > It looks like perhaps the link to
> > https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/
> > on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your
> > kernel tree
> > https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn
> > , which had 1/11 in it, IIUC?  Perhaps this link should go on 1/11
> > rather than 2/11?
> 
> Good catch, patch 1 does introduce the warning.  I think I'll just
> squash patches 1 and 2 so as not to break bisection.
> 
> > Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day
> > reports.  Are you able to flesh out more info how/what/when such objtool
> > warnings are observed?  Are the warnings ever results of patches earlier
> > in the series?
> 
> Hopefully not, it's best to not introduce warnings even temporarily.  I
> was doing a lot of build testing at the time with various branches, so
> it's possible.  I'll see if I can figure out how I triggered those
> warnings and document that in the commit logs if possible.

On second thought I won't squash, keeping them separate is useful for
both patches 1 & 2 and patches 5-7.  The patch order goes up the call
stack, i.e. fix callees before callers.  The opposite order would
trigger actual compiler warnings rather than measly objtool warnings :-)

I agree some of the commit logs are indeed confusing and sometimes even
wrong.  I'll clarify the justifications, and remove references to build
bot warnings if they apply to previous patches in the set.
diff mbox series

Patch

diff --git a/include/linux/start_kernel.h b/include/linux/start_kernel.h
index 864921e54c92..a9806a44a605 100644
--- a/include/linux/start_kernel.h
+++ b/include/linux/start_kernel.h
@@ -8,7 +8,7 @@ 
 /* Define the prototype for start_kernel here, rather than cluttering
    up something else. */
 
-extern asmlinkage void __init start_kernel(void);
+extern asmlinkage void __init __noreturn start_kernel(void);
 extern void __init __noreturn arch_call_rest_init(void);
 extern void __ref __noreturn rest_init(void);
 
diff --git a/init/main.c b/init/main.c
index 161ed956d738..65aab4e9bb49 100644
--- a/init/main.c
+++ b/init/main.c
@@ -937,7 +937,7 @@  static void __init print_unknown_bootoptions(void)
 	memblock_free(unknown_options, len);
 }
 
-asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
+asmlinkage __visible void __init __no_sanitize_address __noreturn start_kernel(void)
 {
 	char *command_line;
 	char *after_dashes;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e7442e7ad676..a6f9a4aeb77b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -222,6 +222,7 @@  static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 		"rewind_stack_and_make_dead",
 		"sev_es_terminate",
 		"snp_abort",
+		"start_kernel",
 		"stop_this_cpu",
 		"usercopy_abort",
 		"xen_cpu_bringup_again",