Annotate the function prototype as noreturn to prevent objtool
warnings like:
vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
As a comparison, an objdump output without the annotation:
[...]
1b63: mov $0x1,%esi
1b68: xor %edi,%edi
1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
[...]
Now, after adding the __noreturn to the function prototype:
[...]
17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
17e4: test %al,%al
17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
[...] <many insns>
1bb9: mov $0x1,%esi
1bbe: xor %edi,%edi
1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
Reported-by: Arnd Bergmann <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
Hey folks, after getting the warning myself a quick search led me to Arnd's
thorough report - investigating a bit, this seems to be the proper solution.
Notice I didn't add the function to objtool's static list, seems this is
unnecessary in this case - lemme know otherwise!
Thanks in advance for reviews,
Guilherme
arch/x86/include/asm/mshyperv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 4c4c0ec3b62e..09c26e658bcc 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible);
void hv_ghcb_msr_write(u64 msr, u64 value);
void hv_ghcb_msr_read(u64 msr, u64 *value);
bool hv_ghcb_negotiate_protocol(void);
-void hv_ghcb_terminate(unsigned int set, unsigned int reason);
+void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
#else
static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
--
2.39.2
On Fri, Mar 10, 2023 at 11:02:52AM -0300, Guilherme G. Piccoli wrote:
> Hey folks, after getting the warning myself a quick search led me to Arnd's
> thorough report - investigating a bit, this seems to be the proper solution.
>
> Notice I didn't add the function to objtool's static list, seems this is
> unnecessary in this case - lemme know otherwise!
> Thanks in advance for reviews,
I'd recommend also adding it to the objtool global_noreturns list,
otherwise this patch will probably trigger warnings with other non-IBT
configs, in cases where the function is called from another translation
unit, where GCC knows the function is noreturn but objtool doesn't.
We're looking at ways of eliminating global_noreturns, but it's
unfortunately still a necessary evil at this point.
Also, FWIW, I have a change coming soon which make these warnings much
easier to diagnose.
--
Josh
On 10/03/2023 12:24, Josh Poimboeuf wrote:
> On Fri, Mar 10, 2023 at 11:02:52AM -0300, Guilherme G. Piccoli wrote:
> I'd recommend also adding it to the objtool global_noreturns list,
> otherwise this patch will probably trigger warnings with other non-IBT
> configs, in cases where the function is called from another translation
> unit, where GCC knows the function is noreturn but objtool doesn't.
>
> We're looking at ways of eliminating global_noreturns, but it's
> unfortunately still a necessary evil at this point.
>
Hi Josh, thanks! Makes sense, I'll respond here with a V2 doing that.
> Also, FWIW, I have a change coming soon which make these warnings much
> easier to diagnose.
>
Cool =)
Annotate the function prototype as noreturn to prevent objtool
warnings like:
vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
Also, as per Josh's suggestion, add it to the global_noreturns list.
As a comparison, an objdump output without the annotation:
[...]
1b63: mov $0x1,%esi
1b68: xor %edi,%edi
1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
[...]
Now, after adding the __noreturn to the function prototype:
[...]
17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
17e4: test %al,%al
17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
[...] <many insns>
1bb9: mov $0x1,%esi
1bbe: xor %edi,%edi
1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
Reported-by: Arnd Bergmann <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Cc: Josh Poimboeuf <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
V2:
- Per Josh's suggestion (thanks!), added the function to the objtool global
table as well.
arch/x86/include/asm/mshyperv.h | 2 +-
tools/objtool/check.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 4c4c0ec3b62e..09c26e658bcc 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible);
void hv_ghcb_msr_write(u64 msr, u64 value);
void hv_ghcb_msr_read(u64 msr, u64 *value);
bool hv_ghcb_negotiate_protocol(void);
-void hv_ghcb_terminate(unsigned int set, unsigned int reason);
+void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
#else
static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f937be1afe65..4b5e03f61f1f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -209,6 +209,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"do_task_dead",
"ex_handler_msr_mce",
"fortify_panic",
+ "hv_ghcb_terminate",
"kthread_complete_and_exit",
"kthread_exit",
"kunit_try_catch_throw",
--
2.39.2
From: Guilherme G. Piccoli <[email protected]>
>
> Annotate the function prototype as noreturn to prevent objtool
> warnings like:
Just curious: Should the actual function also be updated with
__noreturn? In similar situations, such as snp_abort(), the
__noreturn attribute is both places. I don't know what the
guidance is on this question.
In any case, thanks for doing this cleanup!
Michael
>
>
> Also, as per Josh's suggestion, add it to the global_noreturns list.
> As a comparison, an objdump output without the annotation:
>
> vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
> [...]
> 1b63: mov $0x1,%esi
> 1b68: xor %edi,%edi
> 1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
> 1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
> [...]
>
> Now, after adding the __noreturn to the function prototype:
>
> [...]
> 17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
> 17e4: test %al,%al
> 17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
> [...] <many insns>
> 1bb9: mov $0x1,%esi
> 1bbe: xor %edi,%edi
> 1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
>
> Reported-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Cc: Josh Poimboeuf <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
>
>
> V2:
> - Per Josh's suggestion (thanks!), added the function to the objtool global
> table as well.
>
>
> arch/x86/include/asm/mshyperv.h | 2 +-
> tools/objtool/check.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 4c4c0ec3b62e..09c26e658bcc 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int
> numpages, bool visible);
> void hv_ghcb_msr_write(u64 msr, u64 value);
> void hv_ghcb_msr_read(u64 msr, u64 *value);
> bool hv_ghcb_negotiate_protocol(void);
> -void hv_ghcb_terminate(unsigned int set, unsigned int reason);
> +void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
> #else
> static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
> static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index f937be1afe65..4b5e03f61f1f 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -209,6 +209,7 @@ static bool __dead_end_function(struct objtool_file *file, struct
> symbol *func,
> "do_task_dead",
> "ex_handler_msr_mce",
> "fortify_panic",
> + "hv_ghcb_terminate",
> "kthread_complete_and_exit",
> "kthread_exit",
> "kunit_try_catch_throw",
> --
> 2.39.2
On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <[email protected]>
>>
>> Annotate the function prototype as noreturn to prevent objtool
>> warnings like:
>
> Just curious: Should the actual function also be updated with
> __noreturn? In similar situations, such as snp_abort(), the
> __noreturn attribute is both places. I don't know what the
> guidance is on this question.
>
> In any case, thanks for doing this cleanup!
>
> Michael
Thanks Michael!
In my understanding (anybody please correct me if I'm wrong) any user of
this function that rely on this header will "inherit" the attribute -
hence, if this function is not used in any other header or statically
inside it's own definition file, it's not necessary.
But I'm glad in submitting a V3 with that if it's better, let me know.
Cheers,
Guilherme
On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> [...]
> Just curious: Should the actual function also be updated with
> __noreturn? In similar situations, such as snp_abort(), the
> __noreturn attribute is both places. I don't know what the
> guidance is on this question.
>
Hi Michael / Josh (and all), lemme know if you want me to submit a V3
doing that. The function is not called inside this own definition file
nor exported, so I'm not sure that'd be necessary, but glad to do so if
you prefer.
Thanks,
Guilherme
From: Guilherme G. Piccoli <[email protected]> Sent: Thursday, March 16, 2023 2:24 PM
>
> On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> > [...]
> > Just curious: Should the actual function also be updated with
> > __noreturn? In similar situations, such as snp_abort(), the
> > __noreturn attribute is both places. I don't know what the
> > guidance is on this question.
> >
>
> Hi Michael / Josh (and all), lemme know if you want me to submit a V3
> doing that. The function is not called inside this own definition file
> nor exported, so I'm not sure that'd be necessary, but glad to do so if
> you prefer.
>
I don't have a preference. I was just trying to make sure the details
are all correct. I'll defer to those with more knowledge of the
__noreturn attribute than I have.
Michael
On Fri, Mar 17, 2023 at 01:40:25PM +0000, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <[email protected]> Sent: Thursday, March 16, 2023 2:24 PM
> >
> > On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> > > [...]
> > > Just curious: Should the actual function also be updated with
> > > __noreturn? In similar situations, such as snp_abort(), the
> > > __noreturn attribute is both places. I don't know what the
> > > guidance is on this question.
> > >
> >
> > Hi Michael / Josh (and all), lemme know if you want me to submit a V3
> > doing that. The function is not called inside this own definition file
> > nor exported, so I'm not sure that'd be necessary, but glad to do so if
> > you prefer.
> >
>
> I don't have a preference. I was just trying to make sure the details
> are all correct. I'll defer to those with more knowledge of the
> __noreturn attribute than I have.
It's not required, but probably good practice to put __noreturn in both
places to make it more self-documenting.
--
Josh
On 17/03/2023 11:53, Josh Poimboeuf wrote:
> [...]
>>> Hi Michael / Josh (and all), lemme know if you want me to submit a V3
>>> doing that. The function is not called inside this own definition file
>>> nor exported, so I'm not sure that'd be necessary, but glad to do so if
>>> you prefer.
>>>
>>
>> I don't have a preference. I was just trying to make sure the details
>> are all correct. I'll defer to those with more knowledge of the
>> __noreturn attribute than I have.
>
> It's not required, but probably good practice to put __noreturn in both
> places to make it more self-documenting.
>
Thanks Josh and Michael, will submit a V3 shortly with this improvement!
Cheers,
Guilherme
Annotate the function prototype and definition as noreturn to prevent
objtool warnings like:
vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
Also, as per Josh's suggestion, add it to the global_noreturns list.
As a comparison, an objdump output without the annotation:
[...]
1b63: mov $0x1,%esi
1b68: xor %edi,%edi
1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
[...]
Now, after adding the __noreturn to the function prototype:
[...]
17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
17e4: test %al,%al
17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
[...] <many insns>
1bb9: mov $0x1,%esi
1bbe: xor %edi,%edi
1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
Reported-by: Arnd Bergmann <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Cc: Josh Poimboeuf <[email protected]>
Cc: Michael Kelley <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
V3:
- As per Michael / Josh advice (thanks!), added __noreturn to the
function definition as well.
V2:
- Per Josh's suggestion (thanks!), added the function name to the
objtool global table.
Thanks in advance for reviews/comments!
Cheers,
Guilherme
arch/x86/hyperv/ivm.c | 2 +-
arch/x86/include/asm/mshyperv.h | 2 +-
tools/objtool/check.c | 1 +
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 1dbcbd9da74d..4f79dc76042d 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -127,7 +127,7 @@ static enum es_result hv_ghcb_hv_call(struct ghcb *ghcb, u64 exit_code,
return ES_OK;
}
-void hv_ghcb_terminate(unsigned int set, unsigned int reason)
+void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason)
{
u64 val = GHCB_MSR_TERM_REQ;
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 4c4c0ec3b62e..09c26e658bcc 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible);
void hv_ghcb_msr_write(u64 msr, u64 value);
void hv_ghcb_msr_read(u64 msr, u64 *value);
bool hv_ghcb_negotiate_protocol(void);
-void hv_ghcb_terminate(unsigned int set, unsigned int reason);
+void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
#else
static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f937be1afe65..4b5e03f61f1f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -209,6 +209,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"do_task_dead",
"ex_handler_msr_mce",
"fortify_panic",
+ "hv_ghcb_terminate",
"kthread_complete_and_exit",
"kthread_exit",
"kunit_try_catch_throw",
--
2.39.2
On Fri, Mar 17, 2023 at 01:05:46PM -0300, Guilherme G. Piccoli wrote:
> Annotate the function prototype and definition as noreturn to prevent
> objtool warnings like:
>
> vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
>
> Also, as per Josh's suggestion, add it to the global_noreturns list.
> As a comparison, an objdump output without the annotation:
>
> [...]
> 1b63: mov $0x1,%esi
> 1b68: xor %edi,%edi
> 1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
> 1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
> [...]
>
> Now, after adding the __noreturn to the function prototype:
>
> [...]
> 17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
> 17e4: test %al,%al
> 17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
> [...] <many insns>
> 1bb9: mov $0x1,%esi
> 1bbe: xor %edi,%edi
> 1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
>
> Reported-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]/
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
Looks good to me. I've got some other noreturn fixes pending, so I can
add this patch to the pile unless somebody else wants to take it.
--
Josh
From: Guilherme G. Piccoli <[email protected]> Sent: Friday, March 17, 2023 9:06 AM
>
> Annotate the function prototype and definition as noreturn to prevent
> objtool warnings like:
>
> vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction
>
> Also, as per Josh's suggestion, add it to the global_noreturns list.
> As a comparison, an objdump output without the annotation:
>
> [...]
> 1b63: mov $0x1,%esi
> 1b68: xor %edi,%edi
> 1b6a: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1b6f: jmpq ffffffff82f217ec <hyperv_init+0x9c> # unreachable
> 1b74: cmpq $0xffffffffffffffff,-0x702a24(%rip)
> [...]
>
> Now, after adding the __noreturn to the function prototype:
>
> [...]
> 17df: callq ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
> 17e4: test %al,%al
> 17e6: je ffffffff82f21bb9 <hyperv_init+0x469>
> [...] <many insns>
> 1bb9: mov $0x1,%esi
> 1bbe: xor %edi,%edi
> 1bc0: callq ffffffff8102f680 <hv_ghcb_terminate>
> 1bc5: nopw %cs:0x0(%rax,%rax,1) # end of function
>
> Reported-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Michael Kelley <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
>
>
> V3:
> - As per Michael / Josh advice (thanks!), added __noreturn to the
> function definition as well.
>
> V2:
> - Per Josh's suggestion (thanks!), added the function name to the
> objtool global table.
>
> Thanks in advance for reviews/comments!
> Cheers,
>
> Guilherme
>
>
> arch/x86/hyperv/ivm.c | 2 +-
> arch/x86/include/asm/mshyperv.h | 2 +-
> tools/objtool/check.c | 1 +
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 1dbcbd9da74d..4f79dc76042d 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -127,7 +127,7 @@ static enum es_result hv_ghcb_hv_call(struct ghcb *ghcb, u64
> exit_code,
> return ES_OK;
> }
>
> -void hv_ghcb_terminate(unsigned int set, unsigned int reason)
> +void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason)
> {
> u64 val = GHCB_MSR_TERM_REQ;
>
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 4c4c0ec3b62e..09c26e658bcc 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int
> numpages, bool visible);
> void hv_ghcb_msr_write(u64 msr, u64 value);
> void hv_ghcb_msr_read(u64 msr, u64 *value);
> bool hv_ghcb_negotiate_protocol(void);
> -void hv_ghcb_terminate(unsigned int set, unsigned int reason);
> +void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
> #else
> static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
> static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index f937be1afe65..4b5e03f61f1f 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -209,6 +209,7 @@ static bool __dead_end_function(struct objtool_file *file, struct
> symbol *func,
> "do_task_dead",
> "ex_handler_msr_mce",
> "fortify_panic",
> + "hv_ghcb_terminate",
> "kthread_complete_and_exit",
> "kthread_exit",
> "kunit_try_catch_throw",
> --
> 2.39.2
Reviewed-by: Michael Kelley <[email protected]>
On 17/03/2023 13:24, Josh Poimboeuf wrote:
> [...]
>
> Looks good to me. I've got some other noreturn fixes pending, so I can
> add this patch to the pile unless somebody else wants to take it.
Thanks, that'd be great. And thanks Michael for your review =)