I noticed the number of objtool warnings in randconfig kernels have gone down
recently, maybe it's possible to eliminate the remaining ones?
Here are the ones I ran into recently, using gcc-9.3:
==> build/x86/0x3D2B5D6D_defconfig/log <==
arch/x86/kvm/vmx/vmx.o: warning: objtool:
vmx_handle_exit_irqoff()+0x24: unreachable instruction
==> build/x86/0xE0F2ACFF_defconfig/log <==
kernel/time/posix-stubs.o: warning: objtool:
__x64_sys_timer_create()+0x23: sibling call from callable instruction
with modified stack frame
==> build/x86/0xFD7B7323_defconfig/log <==
arch/x86/entry/entry_64.o: warning: objtool: .entry.text+0x991:
unreachable instruction
==> build/x86/0x2EA4CE4F_defconfig/log <==
kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to
check_kcov_mode() with UACCESS enabled
kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call
to check_kcov_mode() with UACCESS enabled
==> build/x86/0x500B1B82_defconfig/log <==
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x1269: call
without frame pointer save/setup
==> build/x86/0x7942F24A_defconfig/log <==
samples/ftrace/ftrace-direct.o: warning: objtool: .text+0x0:
unreachable instruction
samples/ftrace/ftrace-direct-too.o: warning: objtool: .text+0x0:
unreachable instruction
samples/ftrace/ftrace-direct-modify.o: warning: objtool: .text+0x0:
unreachable instruction
I've uploaded the .config and .o files for each one to
https://drive.google.com/open?id=0B_XQwQ5KlfJAbUtrTVJaRmJIZUk
Let me know if you need any additional information.
Note that I used a modified linux-next tree (including many build
fixes) for creating
these, some of them may not actually happen with mainline or unpatched
linux-next
kernels.
Arnd
On Tue, Apr 28, 2020 at 04:49:15PM +0200, Arnd Bergmann wrote:
> I noticed the number of objtool warnings in randconfig kernels have gone down
> recently, maybe it's possible to eliminate the remaining ones?
>
> Here are the ones I ran into recently, using gcc-9.3:
>
> ==> build/x86/0x3D2B5D6D_defconfig/log <==
> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_handle_exit_irqoff()+0x24: unreachable instruction
This is a legit warning caused by CONFIG_UBSAN_TRAP +
CONFIG_UBSAN_ALIGNMENT, I think Kees is going to do a fix.
> ==> build/x86/0xFD7B7323_defconfig/log <==
> arch/x86/entry/entry_64.o: warning: objtool: .entry.text+0x991: unreachable instruction
This warning looks correct, did you make a change to entry_64.S?
> ==> build/x86/0x7942F24A_defconfig/log <==
> samples/ftrace/ftrace-direct.o: warning: objtool: .text+0x0: unreachable instruction
> samples/ftrace/ftrace-direct-too.o: warning: objtool: .text+0x0: unreachable instruction
> samples/ftrace/ftrace-direct-modify.o: warning: objtool: .text+0x0: unreachable instruction
I posted a fix for these a few days ago:
https://lkml.kernel.org/r/86c1cbca67cb353da9f335643ef5fd19bd82988f.1587761369.git.jpoimboe@redhat.com
> ==> build/x86/0xE0F2ACFF_defconfig/log <==
> kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
>
> ==> build/x86/0x2EA4CE4F_defconfig/log <==
> kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
> kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
>
> ==> build/x86/0x500B1B82_defconfig/log <==
> kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x1269: call
> without frame pointer save/setup
I'll look into these.
--
Josh
On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 04:49:15PM +0200, Arnd Bergmann wrote:
> > I noticed the number of objtool warnings in randconfig kernels have gone down
> > recently, maybe it's possible to eliminate the remaining ones?
> >
> > Here are the ones I ran into recently, using gcc-9.3:
> >
> > ==> build/x86/0x3D2B5D6D_defconfig/log <==
> > arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_handle_exit_irqoff()+0x24: unreachable instruction
>
> This is a legit warning caused by CONFIG_UBSAN_TRAP +
> CONFIG_UBSAN_ALIGNMENT, I think Kees is going to do a fix.
Yup, I'm still on the hook for this; just a bit distracted. I'll get it
landed soon.
--
Kees Cook
On Tue, Apr 28, 2020 at 6:10 PM Josh Poimboeuf <[email protected]> wrote:
> On Tue, Apr 28, 2020 at 04:49:15PM +0200, Arnd Bergmann wrote:
> > ==> build/x86/0xFD7B7323_defconfig/log <==
> > arch/x86/entry/entry_64.o: warning: objtool: .entry.text+0x991: unreachable instruction
>
> This warning looks correct, did you make a change to entry_64.S?
I bisected my local patches and found that I had a local hack that turned
off CONFIG_RETPOLINE for testing something unrelated. I can reproduce
it on linux-next with this patch:
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -447,8 +447,7 @@ config GOLDFISH
depends on X86_GOLDFISH
config RETPOLINE
- bool "Avoid speculative indirect branches in kernel"
- default y
+ def_bool n
select STACK_VALIDATION if HAVE_STACK_VALIDATION
help
Compile kernel with the retpoline compiler options to guard against
> > ==> build/x86/0xE0F2ACFF_defconfig/log <==
> > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
> >
> > ==> build/x86/0x2EA4CE4F_defconfig/log <==
> > kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
> > kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
> >
> > ==> build/x86/0x500B1B82_defconfig/log <==
> > kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x1269: call
> > without frame pointer save/setup
>
> I'll look into these.
Thanks!
Arnd
On Tue, Apr 28, 2020 at 10:19:46PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 28, 2020 at 6:10 PM Josh Poimboeuf <[email protected]> wrote:
> > On Tue, Apr 28, 2020 at 04:49:15PM +0200, Arnd Bergmann wrote:
> > > ==> build/x86/0xFD7B7323_defconfig/log <==
> > > arch/x86/entry/entry_64.o: warning: objtool: .entry.text+0x991: unreachable instruction
> >
> > This warning looks correct, did you make a change to entry_64.S?
>
> I bisected my local patches and found that I had a local hack that turned
> off CONFIG_RETPOLINE for testing something unrelated. I can reproduce
> it on linux-next with this patch:
>
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -447,8 +447,7 @@ config GOLDFISH
> depends on X86_GOLDFISH
>
> config RETPOLINE
> - bool "Avoid speculative indirect branches in kernel"
> - default y
> + def_bool n
> select STACK_VALIDATION if HAVE_STACK_VALIDATION
> help
> Compile kernel with the retpoline compiler options to guard against
Thanks, that worked.
This one makes no sense to me. It looks like the assembler is inserting
a jump as part of the alignment padding??? WTH.
0000000000000980 <common_spurious>:
980: 48 83 04 24 80 addq $0xffffffffffffff80,(%rsp)
985: e8 00 00 00 00 callq 98a <common_spurious+0xa>
986: R_X86_64_PLT32 interrupt_entry-0x4
98a: e8 00 00 00 00 callq 98f <common_spurious+0xf>
98b: R_X86_64_PLT32 smp_spurious_interrupt-0x4
98f: eb 7e jmp a0f <ret_from_intr>
991: eb 6d jmp a00 <common_interrupt>
993: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
99a: 00 00 00 00
99e: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9a5: 00 00 00 00
9a9: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9b0: 00 00 00 00
9b4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9bb: 00 00 00 00
9bf: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9c6: 00 00 00 00
9ca: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9d1: 00 00 00 00
9d5: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9dc: 00 00 00 00
9e0: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9e7: 00 00 00 00
9eb: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
9f2: 00 00 00 00
9f6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
9fd: 00 00 00
0000000000000a00 <common_interrupt>:
a00: 48 83 04 24 80 addq $0xffffffffffffff80,(%rsp)
--
Josh
On Tue, Apr 28, 2020 at 03:38:55PM -0500, Josh Poimboeuf wrote:
> This one makes no sense to me. It looks like the assembler is inserting
> a jump as part of the alignment padding??? WTH.
>
> 0000000000000980 <common_spurious>:
> 980: 48 83 04 24 80 addq $0xffffffffffffff80,(%rsp)
> 985: e8 00 00 00 00 callq 98a <common_spurious+0xa>
> 986: R_X86_64_PLT32 interrupt_entry-0x4
> 98a: e8 00 00 00 00 callq 98f <common_spurious+0xf>
> 98b: R_X86_64_PLT32 smp_spurious_interrupt-0x4
> 98f: eb 7e jmp a0f <ret_from_intr>
> 991: eb 6d jmp a00 <common_interrupt>
> 993: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 99a: 00 00 00 00
> 99e: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9a5: 00 00 00 00
> 9a9: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9b0: 00 00 00 00
> 9b4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9bb: 00 00 00 00
> 9bf: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9c6: 00 00 00 00
> 9ca: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9d1: 00 00 00 00
> 9d5: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9dc: 00 00 00 00
> 9e0: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9e7: 00 00 00 00
> 9eb: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 9f2: 00 00 00 00
> 9f6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> 9fd: 00 00 00
binutils.git/gas/configure/tc-i386.c:i386_generate_nops
When there's too many NOPs (as here) it generates a JMP across the NOPS.
It makes some sort of sense, at some point executing NOPs is going to be
more expensive than a branch.. But shees..
On Tue, Apr 28, 2020 at 11:55:54PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 03:38:55PM -0500, Josh Poimboeuf wrote:
> > This one makes no sense to me. It looks like the assembler is inserting
> > a jump as part of the alignment padding??? WTH.
> >
> > 0000000000000980 <common_spurious>:
> > 980: 48 83 04 24 80 addq $0xffffffffffffff80,(%rsp)
> > 985: e8 00 00 00 00 callq 98a <common_spurious+0xa>
> > 986: R_X86_64_PLT32 interrupt_entry-0x4
> > 98a: e8 00 00 00 00 callq 98f <common_spurious+0xf>
> > 98b: R_X86_64_PLT32 smp_spurious_interrupt-0x4
> > 98f: eb 7e jmp a0f <ret_from_intr>
> > 991: eb 6d jmp a00 <common_interrupt>
> > 993: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 99a: 00 00 00 00
> > 99e: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9a5: 00 00 00 00
> > 9a9: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9b0: 00 00 00 00
> > 9b4: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9bb: 00 00 00 00
> > 9bf: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9c6: 00 00 00 00
> > 9ca: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9d1: 00 00 00 00
> > 9d5: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9dc: 00 00 00 00
> > 9e0: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9e7: 00 00 00 00
> > 9eb: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> > 9f2: 00 00 00 00
> > 9f6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> > 9fd: 00 00 00
>
> binutils.git/gas/configure/tc-i386.c:i386_generate_nops
>
> When there's too many NOPs (as here) it generates a JMP across the NOPS.
> It makes some sort of sense, at some point executing NOPs is going to be
> more expensive than a branch.. But shees..
Urgh. Even if I tell it specifically to pad with NOPs, it still does
this "trick". I have no idea how to deal with this in objtool.
--
Josh
On Tue, Apr 28, 2020 at 05:03:53PM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 11:55:54PM +0200, Peter Zijlstra wrote:
> > binutils.git/gas/configure/tc-i386.c:i386_generate_nops
> >
> > When there's too many NOPs (as here) it generates a JMP across the NOPS.
> > It makes some sort of sense, at some point executing NOPs is going to be
> > more expensive than a branch.. But shees..
>
> Urgh. Even if I tell it specifically to pad with NOPs, it still does
> this "trick". I have no idea how to deal with this in objtool.
This is horrible... but it _might_ just work.
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 0e9504fabe52..545f50f5de56 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -599,7 +599,18 @@ SYM_CODE_END(common_spurious)
_ASM_NOKPROBE(common_spurious)
/* common_interrupt is a hotpath. Align it */
+SYM_FUNC_START_LOCAL_NOALIGN(__ignore_me)
.p2align CONFIG_X86_L1_CACHE_SHIFT
+SYM_FUNC_END(__ignore_me)
+
+.pushsection .discard.func_stack_frame_non_standard
+ .align 8
+ .type __func_stack_frame_non_standard___ignore_me, @object
+ .size __func_stack_frame_non_standard___ignore_me, 8
+__func_stack_frame_non_standard___ignore_me:
+ .quad __ignore_me
+.popsection
+
SYM_CODE_START_LOCAL(common_interrupt)
addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */
call interrupt_entry
On Wed, Apr 29, 2020 at 12:33:27AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 05:03:53PM -0500, Josh Poimboeuf wrote:
> > On Tue, Apr 28, 2020 at 11:55:54PM +0200, Peter Zijlstra wrote:
>
> > > binutils.git/gas/configure/tc-i386.c:i386_generate_nops
> > >
> > > When there's too many NOPs (as here) it generates a JMP across the NOPS.
> > > It makes some sort of sense, at some point executing NOPs is going to be
> > > more expensive than a branch.. But shees..
> >
> > Urgh. Even if I tell it specifically to pad with NOPs, it still does
> > this "trick". I have no idea how to deal with this in objtool.
>
> This is horrible... but it _might_ just work.
HAHA, nice.
This seems to work:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 3063aa9090f9..afdf43c9bac1 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -597,8 +597,13 @@ SYM_CODE_START_LOCAL(common_spurious)
SYM_CODE_END(common_spurious)
_ASM_NOKPROBE(common_spurious)
+.macro P2ALIGN_NOPS shift
+ .p2align \shift-1
+ .p2align \shift
+.endm
+
/* common_interrupt is a hotpath. Align it */
- .p2align CONFIG_X86_L1_CACHE_SHIFT
+P2ALIGN_NOPS shift=CONFIG_X86_L1_CACHE_SHIFT
SYM_CODE_START_LOCAL(common_interrupt)
addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */
call interrupt_entry
On Tue, Apr 28, 2020 at 05:48:38PM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 29, 2020 at 12:33:27AM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 28, 2020 at 05:03:53PM -0500, Josh Poimboeuf wrote:
> > > On Tue, Apr 28, 2020 at 11:55:54PM +0200, Peter Zijlstra wrote:
> >
> > > > binutils.git/gas/configure/tc-i386.c:i386_generate_nops
> > > >
> > > > When there's too many NOPs (as here) it generates a JMP across the NOPS.
> > > > It makes some sort of sense, at some point executing NOPs is going to be
> > > > more expensive than a branch.. But shees..
> > >
> > > Urgh. Even if I tell it specifically to pad with NOPs, it still does
> > > this "trick". I have no idea how to deal with this in objtool.
> >
> > This is horrible... but it _might_ just work.
>
> HAHA, nice.
>
> This seems to work:
More sophisticated version:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 3063aa9090f9..f9082673f84c 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -597,8 +597,22 @@ SYM_CODE_START_LOCAL(common_spurious)
SYM_CODE_END(common_spurious)
_ASM_NOKPROBE(common_spurious)
+/*
+ * For .p2align NOP padding with shift >= 7, if the gap is big enough, the GNU
+ * assembler decides to insert a JMP in the padding, which makes objtool sad.
+ * Force it to NOPs only, by splitting it into smaller alignments if necessary.
+ */
+.macro P2ALIGN shift
+ tmp=6
+ .rept \shift-6
+ .p2align tmp
+ tmp=tmp+1
+ .endr
+ .p2align \shift
+.endm
+
/* common_interrupt is a hotpath. Align it */
- .p2align CONFIG_X86_L1_CACHE_SHIFT
+P2ALIGN shift=CONFIG_X86_L1_CACHE_SHIFT
SYM_CODE_START_LOCAL(common_interrupt)
addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */
call interrupt_entry
On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > ==> build/x86/0xE0F2ACFF_defconfig/log <==
> > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
This one is fixed with the following cleanup:
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] linkage: Convert syscall alias macros to C
There's no need to use inline asm to create ELF alias symbols.
Annotated C function declarations can be used instead.
This also makes the ordering of the ELF symbol table more logical, with
the real function now always coming before the aliases. This makes it
easier for objtool, objdump and other tools to differentiate them.
This fixes the following warning:
kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/linkage.h | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index d796ec20d114..2d7dd6361f91 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -22,18 +22,12 @@
#define asmlinkage CPP_ASMLINKAGE
#endif
-#ifndef cond_syscall
-#define cond_syscall(x) asm( \
- ".weak " __stringify(x) "\n\t" \
- ".set " __stringify(x) "," \
- __stringify(sys_ni_syscall))
+#ifndef SYSCALL_ALIAS
+#define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
#endif
-#ifndef SYSCALL_ALIAS
-#define SYSCALL_ALIAS(alias, name) asm( \
- ".globl " __stringify(alias) "\n\t" \
- ".set " __stringify(alias) "," \
- __stringify(name))
+#ifndef cond_syscall
+#define cond_syscall(x) __weak SYSCALL_ALIAS(x, sys_ni_syscall)
#endif
#define __page_aligned_data __section(.data..page_aligned) __aligned(PAGE_SIZE)
--
2.21.1
On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > ==> build/x86/0x2EA4CE4F_defconfig/log <==
> > kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
> > kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] objtool: Add check_kcov_mode() to the uaccess safelist
check_kcov_mode() is called by write_comp_data() and
__sanitizer_cov_trace_pc(), which are already on the uaccess safe list.
Might as well add check_kcov_mode() to the party.
This fixes the following warnings:
kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0c732d586924..fec890547e04 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -516,6 +516,7 @@ static const char *uaccess_safe_builtin[] = {
"__tsan_write16",
/* KCOV */
"write_comp_data",
+ "check_kcov_mode",
"__sanitizer_cov_trace_pc",
"__sanitizer_cov_trace_const_cmp1",
"__sanitizer_cov_trace_const_cmp2",
--
2.21.1
On Wed, Apr 29, 2020 at 8:55 PM Josh Poimboeuf <[email protected]> wrote:
>
> On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > > ==> build/x86/0xE0F2ACFF_defconfig/log <==
> > > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
>
> This one is fixed with the following cleanup:
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] linkage: Convert syscall alias macros to C
>
> There's no need to use inline asm to create ELF alias symbols.
> Annotated C function declarations can be used instead.
>
> This also makes the ordering of the ELF symbol table more logical, with
> the real function now always coming before the aliases. This makes it
> easier for objtool, objdump and other tools to differentiate them.
>
> This fixes the following warning:
>
> kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> include/linux/linkage.h | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
Unfortunately, this patch leads to new warnings when
CONFIG_POSIX_TIMERS is disabled:
In file included from /git/arm-soc/kernel/time/posix-stubs.c:9:
/git/arm-soc/kernel/time/posix-stubs.c:35:37: error: conflicting types
for 'sys_timer_create'
35 | #define SYS_NI(name) SYSCALL_ALIAS(sys_##name, sys_ni_posix_timers)
| ^~~~
/git/arm-soc/include/linux/linkage.h:26:63: note: in definition of
macro 'SYSCALL_ALIAS'
26 | #define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
| ^~~~~
/git/arm-soc/kernel/time/posix-stubs.c:42:1: note: in expansion of
macro 'SYS_NI'
42 | SYS_NI(timer_create);
| ^~~~~~
In file included from /git/arm-soc/kernel/time/posix-stubs.c:13:
/git/arm-soc/include/linux/syscalls.h:616:17: note: previous
declaration of 'sys_timer_create' was here
616 | asmlinkage long sys_timer_create(clockid_t which_clock,
| ^~~~~~~~~~~~~~~~
We can probably move those SYS_NI() instances to kernel/sys_ni.c,
which does not include the header, but it's still a bit ugly. I'll try
that tomorrow
unless you come up with a better suggestion first.
Arnd
On Thu, Apr 30, 2020 at 12:46:57AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 29, 2020 at 8:55 PM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > > > ==> build/x86/0xE0F2ACFF_defconfig/log <==
> > > > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
> >
> > This one is fixed with the following cleanup:
> >
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] linkage: Convert syscall alias macros to C
> >
> > There's no need to use inline asm to create ELF alias symbols.
> > Annotated C function declarations can be used instead.
> >
> > This also makes the ordering of the ELF symbol table more logical, with
> > the real function now always coming before the aliases. This makes it
> > easier for objtool, objdump and other tools to differentiate them.
> >
> > This fixes the following warning:
> >
> > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
> >
> > Reported-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > include/linux/linkage.h | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
>
> Unfortunately, this patch leads to new warnings when
> CONFIG_POSIX_TIMERS is disabled:
Hm, at least on x86 it seems to build fine without CONFIG_POSIX_TIMERS.
Must be some syscall quirk differences between the arches.
> In file included from /git/arm-soc/kernel/time/posix-stubs.c:9:
> /git/arm-soc/kernel/time/posix-stubs.c:35:37: error: conflicting types
> for 'sys_timer_create'
> 35 | #define SYS_NI(name) SYSCALL_ALIAS(sys_##name, sys_ni_posix_timers)
> | ^~~~
> /git/arm-soc/include/linux/linkage.h:26:63: note: in definition of
> macro 'SYSCALL_ALIAS'
> 26 | #define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
> | ^~~~~
> /git/arm-soc/kernel/time/posix-stubs.c:42:1: note: in expansion of
> macro 'SYS_NI'
> 42 | SYS_NI(timer_create);
> | ^~~~~~
> In file included from /git/arm-soc/kernel/time/posix-stubs.c:13:
> /git/arm-soc/include/linux/syscalls.h:616:17: note: previous
> declaration of 'sys_timer_create' was here
> 616 | asmlinkage long sys_timer_create(clockid_t which_clock,
> | ^~~~~~~~~~~~~~~~
>
> We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> which does not include the header, but it's still a bit ugly. I'll try
> that tomorrow
> unless you come up with a better suggestion first.
>
> Arnd
>
--
Josh
On Thu, Apr 30, 2020 at 12:46:57AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 29, 2020 at 8:55 PM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > > > ==> build/x86/0xE0F2ACFF_defconfig/log <==
> > > > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
> >
> > This one is fixed with the following cleanup:
> >
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] linkage: Convert syscall alias macros to C
> >
> > There's no need to use inline asm to create ELF alias symbols.
> > Annotated C function declarations can be used instead.
> >
> > This also makes the ordering of the ELF symbol table more logical, with
> > the real function now always coming before the aliases. This makes it
> > easier for objtool, objdump and other tools to differentiate them.
> >
> > This fixes the following warning:
> >
> > kernel/time/posix-stubs.o: warning: objtool: __x64_sys_timer_create()+0x23: sibling call from callable instruction with modified stack frame
> >
> > Reported-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > include/linux/linkage.h | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
>
> Unfortunately, this patch leads to new warnings when
> CONFIG_POSIX_TIMERS is disabled:
>
> In file included from /git/arm-soc/kernel/time/posix-stubs.c:9:
> /git/arm-soc/kernel/time/posix-stubs.c:35:37: error: conflicting types
> for 'sys_timer_create'
> 35 | #define SYS_NI(name) SYSCALL_ALIAS(sys_##name, sys_ni_posix_timers)
> | ^~~~
> /git/arm-soc/include/linux/linkage.h:26:63: note: in definition of
> macro 'SYSCALL_ALIAS'
> 26 | #define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
> | ^~~~~
> /git/arm-soc/kernel/time/posix-stubs.c:42:1: note: in expansion of
> macro 'SYS_NI'
> 42 | SYS_NI(timer_create);
> | ^~~~~~
> In file included from /git/arm-soc/kernel/time/posix-stubs.c:13:
> /git/arm-soc/include/linux/syscalls.h:616:17: note: previous
> declaration of 'sys_timer_create' was here
> 616 | asmlinkage long sys_timer_create(clockid_t which_clock,
> | ^~~~~~~~~~~~~~~~
>
> We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> which does not include the header, but it's still a bit ugly. I'll try
> that tomorrow
> unless you come up with a better suggestion first.
Oh I guess arm32 doesn't have SYS_NI defined. All this syscall aliasing
stuff is a total mystery to me.
--
Josh
On Wed, Apr 29, 2020 at 06:11:15PM -0500, Josh Poimboeuf wrote:
> > We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> > which does not include the header, but it's still a bit ugly. I'll try
> > that tomorrow
> > unless you come up with a better suggestion first.
>
> Oh I guess arm32 doesn't have SYS_NI defined. All this syscall aliasing
> stuff is a total mystery to me.
Another idea would be to split up syscalls.h into two files: one for
SYSCALL_* macros and one for sys_*() function prototypes. It sounds
like the latter aren't needed by most header files anyway.
* Please note that these prototypes here are only provided for information
* purposes, for static analysis, and for linking from the syscall table.
* These functions should not be called elsewhere from kernel code.
--
Josh
On Thu, Apr 30, 2020 at 1:28 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Wed, Apr 29, 2020 at 06:11:15PM -0500, Josh Poimboeuf wrote:
> > > We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> > > which does not include the header, but it's still a bit ugly. I'll try
> > > that tomorrow
> > > unless you come up with a better suggestion first.
> >
> > Oh I guess arm32 doesn't have SYS_NI defined. All this syscall aliasing
> > stuff is a total mystery to me.
>
> Another idea would be to split up syscalls.h into two files: one for
> SYSCALL_* macros and one for sys_*() function prototypes. It sounds
> like the latter aren't needed by most header files anyway.
>
> * Please note that these prototypes here are only provided for information
> * purposes, for static analysis, and for linking from the syscall table.
> * These functions should not be called elsewhere from kernel code.
To me the main purpose of the header is to ensure the calling conventions
are sane, so I'd definitely want to see the declarations included whenever
a syscall is defined. I would also expect to see a warnig from sparse, or
from gcc with "make W=1" when an extern function is defined with no
prior declaration.
How hard would it be to change objtool instead of changing the sources?
Arnd
On Tue, Apr 28, 2020 at 4:49 PM Arnd Bergmann <[email protected]> wrote:
>
> I noticed the number of objtool warnings in randconfig kernels have gone down
> recently, maybe it's possible to eliminate the remaining ones?
>
> Here are the ones I ran into recently, using gcc-9.3:
I now cross-checked with gcc-10.0, which leads to a few additional warnings
for randconfig builds:
drivers/gpu/drm/vmwgfx/vmwgfx_msg.o: warning: objtool:
vmw_port_hb_in()+0x12f: stack state mismatch: cfa1=6+16 cfa2=7+8
drivers/gpu/drm/vmwgfx/vmwgfx_msg.o: warning: objtool:
vmw_port_hb_in()+0xca: return with modified stack frame
drivers/media/dvb-frontends/rtl2832_sdr.o: warning: objtool:
.text.unlikely: unexpected end of section
drivers/media/dvb-frontends/rtl2832_sdr.o: warning: objtool:
rtl2832_sdr_try_fmt_sdr_cap() falls through to next function
rtl2832_sdr_s_fmt_sdr_cap.cold()
kernel/kexec_core.o: warning: objtool: crash_kexec()+0x2b: unreachable
instruction
lib/locking-selftest.o: warning: objtool: locking_selftest()+0x117f:
PUSHF stack exhausted
lib/locking-selftest.o: warning: objtool: ww_tests()+0x1083: PUSHF
stack exhausted
lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133:
call to do_strncpy_from_user() with UACCESS enabled
lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to
do_strnlen_user() with UACCESS enabled
I uploaded these as well now, see
https://drive.google.com/open?id=1k10cO9OkFKaMVnK7uX-prplY-13CAvW1
Arnd
On Thu, Apr 30, 2020 at 03:41:56PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 1:28 AM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Wed, Apr 29, 2020 at 06:11:15PM -0500, Josh Poimboeuf wrote:
> > > > We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> > > > which does not include the header, but it's still a bit ugly. I'll try
> > > > that tomorrow
> > > > unless you come up with a better suggestion first.
> > >
> > > Oh I guess arm32 doesn't have SYS_NI defined. All this syscall aliasing
> > > stuff is a total mystery to me.
> >
> > Another idea would be to split up syscalls.h into two files: one for
> > SYSCALL_* macros and one for sys_*() function prototypes. It sounds
> > like the latter aren't needed by most header files anyway.
> >
> > * Please note that these prototypes here are only provided for information
> > * purposes, for static analysis, and for linking from the syscall table.
> > * These functions should not be called elsewhere from kernel code.
>
> To me the main purpose of the header is to ensure the calling conventions
> are sane, so I'd definitely want to see the declarations included whenever
> a syscall is defined. I would also expect to see a warnig from sparse, or
> from gcc with "make W=1" when an extern function is defined with no
> prior declaration.
Yup, makes sense. I think I've been getting confused by the syscall
wrappers.
> How hard would it be to change objtool instead of changing the sources?
It might be a little tricky, but I can look into it.
--
Josh
On Thu, Apr 30, 2020 at 09:33:50AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 30, 2020 at 03:41:56PM +0200, Arnd Bergmann wrote:
> > On Thu, Apr 30, 2020 at 1:28 AM Josh Poimboeuf <[email protected]> wrote:
> > >
> > > On Wed, Apr 29, 2020 at 06:11:15PM -0500, Josh Poimboeuf wrote:
> > > > > We can probably move those SYS_NI() instances to kernel/sys_ni.c,
> > > > > which does not include the header, but it's still a bit ugly. I'll try
> > > > > that tomorrow
> > > > > unless you come up with a better suggestion first.
> > > >
> > > > Oh I guess arm32 doesn't have SYS_NI defined. All this syscall aliasing
> > > > stuff is a total mystery to me.
> > >
> > > Another idea would be to split up syscalls.h into two files: one for
> > > SYSCALL_* macros and one for sys_*() function prototypes. It sounds
> > > like the latter aren't needed by most header files anyway.
> > >
> > > * Please note that these prototypes here are only provided for information
> > > * purposes, for static analysis, and for linking from the syscall table.
> > > * These functions should not be called elsewhere from kernel code.
> >
> > To me the main purpose of the header is to ensure the calling conventions
> > are sane, so I'd definitely want to see the declarations included whenever
> > a syscall is defined. I would also expect to see a warnig from sparse, or
> > from gcc with "make W=1" when an extern function is defined with no
> > prior declaration.
>
> Yup, makes sense. I think I've been getting confused by the syscall
> wrappers.
>
> > How hard would it be to change objtool instead of changing the sources?
>
> It might be a little tricky, but I can look into it.
So there's an easy fix below, just define an x86-specific SYSCALL_ALIAS.
It also requries moving the syscall alias macros to syscalls.h, but
that's probably where they belong anyway.
But the objtool .cold parent alias function detection is a little
smelly, so I might end up cleaning that up instead if I can figure out a
good way to do it.
diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
index a84333adeef2..abe6e633f8dc 100644
--- a/arch/x86/include/asm/syscall_wrapper.h
+++ b/arch/x86/include/asm/syscall_wrapper.h
@@ -79,6 +79,8 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
return __se_##name(__VA_ARGS__); \
}
+#define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
+
#define __COND_SYSCALL(abi, name) \
__weak long __##abi##_##name(const struct pt_regs *__unused) \
{ \
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index d796ec20d114..369c65d4386c 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -22,20 +22,6 @@
#define asmlinkage CPP_ASMLINKAGE
#endif
-#ifndef cond_syscall
-#define cond_syscall(x) asm( \
- ".weak " __stringify(x) "\n\t" \
- ".set " __stringify(x) "," \
- __stringify(sys_ni_syscall))
-#endif
-
-#ifndef SYSCALL_ALIAS
-#define SYSCALL_ALIAS(alias, name) asm( \
- ".globl " __stringify(alias) "\n\t" \
- ".set " __stringify(alias) "," \
- __stringify(name))
-#endif
-
#define __page_aligned_data __section(.data..page_aligned) __aligned(PAGE_SIZE)
#define __page_aligned_bss __section(.bss..page_aligned) __aligned(PAGE_SIZE)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 1815065d52f3..dc93d7e595af 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -252,6 +252,20 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* __SYSCALL_DEFINEx */
+#ifndef SYSCALL_ALIAS
+#define SYSCALL_ALIAS(alias, name) asm( \
+ ".globl " __stringify(alias) "\n\t" \
+ ".set " __stringify(alias) "," \
+ __stringify(name))
+#endif
+
+#ifndef cond_syscall
+#define cond_syscall(x) asm( \
+ ".weak " __stringify(x) "\n\t" \
+ ".set " __stringify(x) "," \
+ __stringify(sys_ni_syscall))
+#endif
+
/*
* Called before coming back to user-mode. Returning to user-mode with an
* address limit different than USER_DS can allow to overwrite kernel memory.
On Thu, Apr 30, 2020 at 9:46 PM Josh Poimboeuf <[email protected]> wrote:
> On Thu, Apr 30, 2020 at 09:33:50AM -0500, Josh Poimboeuf wrote:
>
> So there's an easy fix below, just define an x86-specific SYSCALL_ALIAS.
> It also requries moving the syscall alias macros to syscalls.h, but
> that's probably where they belong anyway.
>
> But the objtool .cold parent alias function detection is a little
> smelly, so I might end up cleaning that up instead if I can figure out a
> good way to do it.
>
> diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
> index a84333adeef2..abe6e633f8dc 100644
> --- a/arch/x86/include/asm/syscall_wrapper.h
> +++ b/arch/x86/include/asm/syscall_wrapper.h
> @@ -79,6 +79,8 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> return __se_##name(__VA_ARGS__); \
> }
>
> +#define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
> +
Right, this should work in principle, though I suspect it needs to be
changed to include the ABI name for x86, as there are separate
entry points for 32 and 64 bit.
Arnd
On Thu, Apr 30, 2020 at 02:46:30PM -0500, Josh Poimboeuf wrote:
> > Yup, makes sense. I think I've been getting confused by the syscall
> > wrappers.
> >
> > > How hard would it be to change objtool instead of changing the sources?
So I just realized this is an objtool bug after all. Or at least a new
GCC quirk. The quick fix is:
sed -si 's/cold./cold/' tools/objtool/check.c
However... after studying how all this works, I'm actually thinking that
it makes sense to move the SYS_NI usage into kernel/sys_ni.c, like you
originally suggested. That seems cleaner to me: all the syscall
aliasing code together in one file. SYS_NI is similar to COND_SYSCALL,
except it has a custom ENOSYS handler. Having "NI" in the name is
another clue it belongs in sys_ni.c.
Alternatively, I could do the x86-specific SYSCALL_ALIAS, which is easy
enough, but I really prefer the sys_ni.c approach.
Either of those would allow the removal of some hacky objtool code,
which only ever existed in the first place because of posix-stubs.c and
that inline asm SYSCALL_ALIAS macro.
One of objtool's goals is to standardize ELF data, and it seems
reasonable to require the use of C-based aliases. And I think it would
be a nice cleanup anyway.
--
Josh
On Thu, Apr 30, 2020 at 10:59:44PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 9:46 PM Josh Poimboeuf <[email protected]> wrote:
> > On Thu, Apr 30, 2020 at 09:33:50AM -0500, Josh Poimboeuf wrote:
>
> >
> > So there's an easy fix below, just define an x86-specific SYSCALL_ALIAS.
> > It also requries moving the syscall alias macros to syscalls.h, but
> > that's probably where they belong anyway.
> >
> > But the objtool .cold parent alias function detection is a little
> > smelly, so I might end up cleaning that up instead if I can figure out a
> > good way to do it.
> >
> > diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
> > index a84333adeef2..abe6e633f8dc 100644
> > --- a/arch/x86/include/asm/syscall_wrapper.h
> > +++ b/arch/x86/include/asm/syscall_wrapper.h
> > @@ -79,6 +79,8 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> > return __se_##name(__VA_ARGS__); \
> > }
> >
> > +#define SYSCALL_ALIAS(alias, name) __alias(name) typeof(name) alias
> > +
>
> Right, this should work in principle, though I suspect it needs to be
> changed to include the ABI name for x86, as there are separate
> entry points for 32 and 64 bit.
As far as I can tell it should actually work, because of the
x86-specific SYS_NI macro.
--
Josh
On Thu, Apr 30, 2020 at 04:08:05PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 30, 2020 at 02:46:30PM -0500, Josh Poimboeuf wrote:
> > > Yup, makes sense. I think I've been getting confused by the syscall
> > > wrappers.
> > >
> > > > How hard would it be to change objtool instead of changing the sources?
>
> So I just realized this is an objtool bug after all. Or at least a new
> GCC quirk. The quick fix is:
>
> sed -si 's/cold./cold/' tools/objtool/check.c
>
> However... after studying how all this works, I'm actually thinking that
> it makes sense to move the SYS_NI usage into kernel/sys_ni.c, like you
> originally suggested. That seems cleaner to me: all the syscall
> aliasing code together in one file. SYS_NI is similar to COND_SYSCALL,
> except it has a custom ENOSYS handler. Having "NI" in the name is
> another clue it belongs in sys_ni.c.
>
> Alternatively, I could do the x86-specific SYSCALL_ALIAS, which is easy
> enough, but I really prefer the sys_ni.c approach.
>
> Either of those would allow the removal of some hacky objtool code,
> which only ever existed in the first place because of posix-stubs.c and
> that inline asm SYSCALL_ALIAS macro.
>
> One of objtool's goals is to standardize ELF data, and it seems
> reasonable to require the use of C-based aliases. And I think it would
> be a nice cleanup anyway.
It's a really nice diffstat. The only "downside" I can tell is that now
there's no printk when calling one of the undefined timer syscalls with
CONFIG_POSIX_TIMERS=n.
Thoughts?
diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 94e4cde8071a..a28ed880790e 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1013,8 +1013,6 @@ SYSCALL_DEFINE2(osf_settimeofday, struct timeval32 __user *, tv,
return do_sys_settimeofday64(tv ? &kts : NULL, tz ? &ktz : NULL);
}
-asmlinkage long sys_ni_posix_timers(void);
-
SYSCALL_DEFINE2(osf_utimes, const char __user *, filename,
struct timeval32 __user *, tvs)
{
diff --git a/arch/arm64/include/asm/syscall_wrapper.h b/arch/arm64/include/asm/syscall_wrapper.h
index b383b4802a7b..44226f09e398 100644
--- a/arch/arm64/include/asm/syscall_wrapper.h
+++ b/arch/arm64/include/asm/syscall_wrapper.h
@@ -43,9 +43,6 @@ struct pt_regs;
return sys_ni_syscall(); \
}
-#define COMPAT_SYS_NI(name) \
- SYSCALL_ALIAS(__arm64_compat_sys_##name, sys_ni_posix_timers);
-
#endif /* CONFIG_COMPAT */
#define __SYSCALL_DEFINEx(x, name, ...) \
@@ -78,6 +75,4 @@ struct pt_regs;
return sys_ni_syscall(); \
}
-#define SYS_NI(name) SYSCALL_ALIAS(__arm64_sys_##name, sys_ni_posix_timers);
-
#endif /* __ASM_SYSCALL_WRAPPER_H */
diff --git a/arch/s390/include/asm/syscall_wrapper.h b/arch/s390/include/asm/syscall_wrapper.h
index 3c3d6fe8e2f0..16f52683fa98 100644
--- a/arch/s390/include/asm/syscall_wrapper.h
+++ b/arch/s390/include/asm/syscall_wrapper.h
@@ -61,10 +61,6 @@
cond_syscall(__s390x_sys_##name); \
cond_syscall(__s390_sys_##name)
-#define SYS_NI(name) \
- SYSCALL_ALIAS(__s390x_sys_##name, sys_ni_posix_timers); \
- SYSCALL_ALIAS(__s390_sys_##name, sys_ni_posix_timers)
-
#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
__diag_push(); \
__diag_ignore(GCC, 8, "-Wattribute-alias", \
@@ -86,15 +82,11 @@
/*
* As some compat syscalls may not be implemented, we need to expand
- * COND_SYSCALL_COMPAT in kernel/sys_ni.c and COMPAT_SYS_NI in
- * kernel/time/posix-stubs.c to cover this case as well.
+ * COND_SYSCALL_COMPAT in kernel/sys_ni.c to cover this case as well.
*/
#define COND_SYSCALL_COMPAT(name) \
cond_syscall(__s390_compat_sys_##name)
-#define COMPAT_SYS_NI(name) \
- SYSCALL_ALIAS(__s390_compat_sys_##name, sys_ni_posix_timers)
-
#else /* CONFIG_COMPAT */
#define __S390_SYS_STUBx(x, fullname, name, ...)
@@ -108,9 +100,6 @@
#define COND_SYSCALL(name) \
cond_syscall(__s390x_sys_##name)
-#define SYS_NI(name) \
- SYSCALL_ALIAS(__s390x_sys_##name, sys_ni_posix_timers);
-
#endif /* CONFIG_COMPAT */
#define __SYSCALL_DEFINEx(x, name, ...) \
diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
index a84333adeef2..655e7832fc29 100644
--- a/arch/x86/include/asm/syscall_wrapper.h
+++ b/arch/x86/include/asm/syscall_wrapper.h
@@ -85,9 +85,6 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
return sys_ni_syscall(); \
}
-#define __SYS_NI(abi, name) \
- SYSCALL_ALIAS(__##abi##_##name, sys_ni_posix_timers);
-
#ifdef CONFIG_X86_64
#define __X64_SYS_STUB0(name) \
__SYS_STUB0(x64, sys_##name)
@@ -99,13 +96,10 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
#define __X64_COND_SYSCALL(name) \
__COND_SYSCALL(x64, sys_##name)
-#define __X64_SYS_NI(name) \
- __SYS_NI(x64, sys_##name)
#else /* CONFIG_X86_64 */
#define __X64_SYS_STUB0(name)
#define __X64_SYS_STUBx(x, name, ...)
#define __X64_COND_SYSCALL(name)
-#define __X64_SYS_NI(name)
#endif /* CONFIG_X86_64 */
#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
@@ -119,13 +113,10 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
#define __IA32_COND_SYSCALL(name) \
__COND_SYSCALL(ia32, sys_##name)
-#define __IA32_SYS_NI(name) \
- __SYS_NI(ia32, sys_##name)
#else /* CONFIG_X86_32 || CONFIG_IA32_EMULATION */
#define __IA32_SYS_STUB0(name)
#define __IA32_SYS_STUBx(x, name, ...)
#define __IA32_COND_SYSCALL(name)
-#define __IA32_SYS_NI(name)
#endif /* CONFIG_X86_32 || CONFIG_IA32_EMULATION */
#ifdef CONFIG_IA32_EMULATION
@@ -134,8 +125,7 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
* additional wrappers (aptly named __ia32_sys_xyzzy) which decode the
* ia32 regs in the proper order for shared or "common" syscalls. As some
* syscalls may not be implemented, we need to expand COND_SYSCALL in
- * kernel/sys_ni.c and SYS_NI in kernel/time/posix-stubs.c to cover this
- * case as well.
+ * kernel/sys_ni.c to cover this case as well.
*/
#define __IA32_COMPAT_SYS_STUB0(name) \
__SYS_STUB0(ia32, compat_sys_##name)
@@ -147,14 +137,10 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
#define __IA32_COMPAT_COND_SYSCALL(name) \
__COND_SYSCALL(ia32, compat_sys_##name)
-#define __IA32_COMPAT_SYS_NI(name) \
- __SYS_NI(ia32, compat_sys_##name)
-
#else /* CONFIG_IA32_EMULATION */
#define __IA32_COMPAT_SYS_STUB0(name)
#define __IA32_COMPAT_SYS_STUBx(x, name, ...)
#define __IA32_COMPAT_COND_SYSCALL(name)
-#define __IA32_COMPAT_SYS_NI(name)
#endif /* CONFIG_IA32_EMULATION */
@@ -174,13 +160,10 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
#define __X32_COMPAT_COND_SYSCALL(name) \
__COND_SYSCALL(x32, compat_sys_##name)
-#define __X32_COMPAT_SYS_NI(name) \
- __SYS_NI(x32, compat_sys_##name)
#else /* CONFIG_X86_X32 */
#define __X32_COMPAT_SYS_STUB0(name)
#define __X32_COMPAT_SYS_STUBx(x, name, ...)
#define __X32_COMPAT_COND_SYSCALL(name)
-#define __X32_COMPAT_SYS_NI(name)
#endif /* CONFIG_X86_X32 */
@@ -211,17 +194,12 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
/*
* As some compat syscalls may not be implemented, we need to expand
- * COND_SYSCALL_COMPAT in kernel/sys_ni.c and COMPAT_SYS_NI in
- * kernel/time/posix-stubs.c to cover this case as well.
+ * COND_SYSCALL_COMPAT in kernel/sys_ni.c to cover this case as well.
*/
#define COND_SYSCALL_COMPAT(name) \
__IA32_COMPAT_COND_SYSCALL(name) \
__X32_COMPAT_COND_SYSCALL(name)
-#define COMPAT_SYS_NI(name) \
- __IA32_COMPAT_SYS_NI(name) \
- __X32_COMPAT_SYS_NI(name)
-
#endif /* CONFIG_COMPAT */
#define __SYSCALL_DEFINEx(x, name, ...) \
@@ -242,8 +220,8 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
* As the generic SYSCALL_DEFINE0() macro does not decode any parameters for
* obvious reasons, and passing struct pt_regs *regs to it in %rdi does not
* hurt, we only need to re-define it here to keep the naming congruent to
- * SYSCALL_DEFINEx() -- which is essential for the COND_SYSCALL() and SYS_NI()
- * macros to work correctly.
+ * SYSCALL_DEFINEx() -- which is essential for the COND_SYSCALL() macro to
+ * work correctly.
*/
#define SYSCALL_DEFINE0(sname) \
SYSCALL_METADATA(_##sname, 0); \
@@ -256,11 +234,6 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
__X64_COND_SYSCALL(name) \
__IA32_COND_SYSCALL(name)
-#define SYS_NI(name) \
- __X64_SYS_NI(name) \
- __IA32_SYS_NI(name)
-
-
/*
* For VSYSCALLS, we need to declare these three syscalls with the new
* pt_regs-based calling convention for in-kernel use.
diff --git a/init/Kconfig b/init/Kconfig
index 9e22ee8fbd75..6a24a6d8f906 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2286,8 +2286,7 @@ config ARCH_HAS_SYNC_CORE_BEFORE_USERMODE
# SYSCALL_DEFINE() and __SYSCALL_DEFINEx() macros in <linux/syscalls.h>
# and the COMPAT_ variants in <linux/compat.h>, in particular to use a
# different calling convention for syscalls. They can also override the
-# macros for not-implemented syscalls in kernel/sys_ni.c and
-# kernel/time/posix-stubs.c. All these overrides need to be available in
-# <asm/syscall_wrapper.h>.
+# macros for not-implemented syscalls in kernel/sys_ni.c. All these
+# overrides need to be available in <asm/syscall_wrapper.h>.
config ARCH_HAS_SYSCALL_WRAPPER
def_bool n
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 3b69a560a7ac..8b3fc3a5d4df 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -151,6 +151,10 @@ COND_SYSCALL_COMPAT(get_robust_list);
/* kernel/hrtimer.c */
/* kernel/itimer.c */
+COND_SYSCALL(getitimer);
+COND_SYSCALL_COMPAT(getitimer);
+COND_SYSCALL(setitimer);
+COND_SYSCALL_COMPAT(setitimer);
/* kernel/kexec.c */
COND_SYSCALL(kexec_load);
@@ -161,6 +165,12 @@ COND_SYSCALL(init_module);
COND_SYSCALL(delete_module);
/* kernel/posix-timers.c */
+COND_SYSCALL(timer_create);
+COND_SYSCALL_COMPAT(timer_create);
+COND_SYSCALL(timer_gettime);
+COND_SYSCALL(timer_getoverrun);
+COND_SYSCALL(timer_settime);
+COND_SYSCALL(timer_delete);
/* kernel/printk.c */
COND_SYSCALL(syslog);
@@ -312,6 +322,8 @@ COND_SYSCALL(name_to_handle_at);
COND_SYSCALL(open_by_handle_at);
COND_SYSCALL_COMPAT(open_by_handle_at);
+COND_SYSCALL(clock_adjtime);
+
COND_SYSCALL(sendmmsg);
COND_SYSCALL_COMPAT(sendmmsg);
COND_SYSCALL(process_vm_readv);
@@ -349,7 +361,6 @@ COND_SYSCALL(pkey_mprotect);
COND_SYSCALL(pkey_alloc);
COND_SYSCALL(pkey_free);
-
/*
* Architecture specific weak syscall entries.
*/
@@ -449,6 +460,9 @@ COND_SYSCALL(sysfs);
COND_SYSCALL(ipc);
COND_SYSCALL_COMPAT(ipc);
+/* obsolete: __ARCH_WANT_SYS_ALARM */
+COND_SYSCALL(alarm);
+
/* obsolete: UID16 */
COND_SYSCALL(chown16);
COND_SYSCALL(fchown16);
diff --git a/kernel/time/posix-stubs.c b/kernel/time/posix-stubs.c
index fcb3b21d8bdc..e6f6f29e0c4f 100644
--- a/kernel/time/posix-stubs.c
+++ b/kernel/time/posix-stubs.c
@@ -17,40 +17,6 @@
#include <linux/time_namespace.h>
#include <linux/compat.h>
-#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
-/* Architectures may override SYS_NI and COMPAT_SYS_NI */
-#include <asm/syscall_wrapper.h>
-#endif
-
-asmlinkage long sys_ni_posix_timers(void)
-{
- pr_err_once("process %d (%s) attempted a POSIX timer syscall "
- "while CONFIG_POSIX_TIMERS is not set\n",
- current->pid, current->comm);
- return -ENOSYS;
-}
-
-#ifndef SYS_NI
-#define SYS_NI(name) SYSCALL_ALIAS(sys_##name, sys_ni_posix_timers)
-#endif
-
-#ifndef COMPAT_SYS_NI
-#define COMPAT_SYS_NI(name) SYSCALL_ALIAS(compat_sys_##name, sys_ni_posix_timers)
-#endif
-
-SYS_NI(timer_create);
-SYS_NI(timer_gettime);
-SYS_NI(timer_getoverrun);
-SYS_NI(timer_settime);
-SYS_NI(timer_delete);
-SYS_NI(clock_adjtime);
-SYS_NI(getitimer);
-SYS_NI(setitimer);
-SYS_NI(clock_adjtime32);
-#ifdef __ARCH_WANT_SYS_ALARM
-SYS_NI(alarm);
-#endif
-
/*
* We preserve minimal support for CLOCK_REALTIME and CLOCK_MONOTONIC
* as it is easy to remain compatible with little code. CLOCK_BOOTTIME
@@ -156,18 +122,7 @@ SYSCALL_DEFINE4(clock_nanosleep, const clockid_t, which_clock, int, flags,
which_clock);
}
-#ifdef CONFIG_COMPAT
-COMPAT_SYS_NI(timer_create);
-#endif
-
-#if defined(CONFIG_COMPAT) || defined(CONFIG_ALPHA)
-COMPAT_SYS_NI(getitimer);
-COMPAT_SYS_NI(setitimer);
-#endif
-
#ifdef CONFIG_COMPAT_32BIT_TIME
-SYS_NI(timer_settime32);
-SYS_NI(timer_gettime32);
SYSCALL_DEFINE2(clock_settime32, const clockid_t, which_clock,
struct old_timespec32 __user *, tp)
@@ -248,4 +203,5 @@ SYSCALL_DEFINE4(clock_nanosleep_time32, clockid_t, which_clock, int, flags,
HRTIMER_MODE_ABS : HRTIMER_MODE_REL,
which_clock);
}
-#endif
+
+#endif /* CONFIG_COMPAT_32BIT_TIME */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index fec890547e04..d3f5efee9f4d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -655,38 +655,12 @@ static int add_jump_destinations(struct objtool_file *file)
return -1;
}
- /*
- * Cross-function jump.
- */
if (insn->func && insn->jump_dest->func &&
- insn->func != insn->jump_dest->func) {
+ insn->func->pfunc != insn->jump_dest->func->pfunc &&
+ insn->jump_dest->offset == insn->jump_dest->func->offset) {
- /*
- * For GCC 8+, create parent/child links for any cold
- * subfunctions. This is _mostly_ redundant with a
- * similar initialization in read_symbols().
- *
- * If a function has aliases, we want the *first* such
- * function in the symbol table to be the subfunction's
- * parent. In that case we overwrite the
- * initialization done in read_symbols().
- *
- * However this code can't completely replace the
- * read_symbols() code because this doesn't detect the
- * case where the parent function's only reference to a
- * subfunction is through a jump table.
- */
- if (!strstr(insn->func->name, ".cold.") &&
- strstr(insn->jump_dest->func->name, ".cold.")) {
- insn->func->cfunc = insn->jump_dest->func;
- insn->jump_dest->func->pfunc = insn->func;
-
- } else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
- insn->jump_dest->offset == insn->jump_dest->func->offset) {
-
- /* internal sibling call */
- insn->call_dest = insn->jump_dest->func;
- }
+ /* internal sibling call */
+ insn->call_dest = insn->jump_dest->func;
}
}
On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> lib/locking-selftest.o: warning: objtool: locking_selftest()+0x117f: PUSHF stack exhausted
> lib/locking-selftest.o: warning: objtool: ww_tests()+0x1083: PUSHF stack exhausted
Peter,
These functions have a bunch of irqs_disabled() checks, which means a
bunch of PUSHFs with no POPFs.
Am I reading it correctly that objtool assumes PUSHF is always paired
with POPF? irqs_disabled() doesn't do that.
--
Josh
On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133: call to do_strncpy_from_user() with UACCESS enabled
> lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to do_strnlen_user() with UACCESS enabled
Does this fix it?
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 706020b06617..cb3ae7a093c3 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -30,6 +30,9 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
unsigned long res = 0;
+ if (!user_access_begin(src, max))
+ return -EFAULT;
+
if (IS_UNALIGNED(src, dst))
goto byte_at_a_time;
@@ -43,7 +46,8 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
if (has_zero(c, &data, &constants)) {
data = prep_zero_mask(c, data, &constants);
data = create_zero_mask(data);
- return res + find_zero(data);
+ res += find_zero(data);
+ goto done;
}
res += sizeof(unsigned long);
max -= sizeof(unsigned long);
@@ -56,7 +60,7 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
unsafe_get_user(c,src+res, efault);
dst[res] = c;
if (!c)
- return res;
+ goto done;
res++;
max--;
}
@@ -65,14 +69,20 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
* Uhhuh. We hit 'max'. But was that the user-specified maximum
* too? If so, that's ok - we got as much as the user asked for.
*/
- if (res >= count)
- return res;
+ if (res < count) {
+ /*
+ * Nope: we hit the address space limit, and we still had more
+ * characters the caller would have wanted. That's an EFAULT.
+ */
+ goto efault;
+ }
+
+done:
+ user_access_end();
+ return res;
- /*
- * Nope: we hit the address space limit, and we still had more
- * characters the caller would have wanted. That's an EFAULT.
- */
efault:
+ user_access_end();
return -EFAULT;
}
@@ -105,7 +115,6 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
src_addr = (unsigned long)untagged_addr(src);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
- long retval;
/*
* Truncate 'max' to the user-specified limit, so that
@@ -116,11 +125,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
kasan_check_write(dst, count);
check_object_size(dst, count, false);
- if (user_access_begin(src, max)) {
- retval = do_strncpy_from_user(dst, src, count, max);
- user_access_end();
- return retval;
- }
+ return do_strncpy_from_user(dst, src, count, max);
}
return -EFAULT;
}
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 41670d4a5816..f1e9e447bef1 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -26,6 +26,9 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
unsigned long align, res = 0;
unsigned long c;
+ if (!user_access_begin(src, max))
+ return 0;
+
/*
* Do everything aligned. But that means that we
* need to also expand the maximum..
@@ -39,10 +42,12 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
for (;;) {
unsigned long data;
+
if (has_zero(c, &data, &constants)) {
data = prep_zero_mask(c, data, &constants);
data = create_zero_mask(data);
- return res + find_zero(data) + 1 - align;
+ res += find_zero(data) + 1 - align;
+ goto done;
}
res += sizeof(unsigned long);
/* We already handled 'unsigned long' bytes. Did we do it all ? */
@@ -58,13 +63,21 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
* too? If so, return the marker for "too long".
*/
if (res >= count)
- return count+1;
+ res = count + 1;
+ else {
+ /*
+ * Nope: we hit the address space limit, and we still had more
+ * characters the caller would have wanted. That's 0.
+ */
+ goto efault;
+ }
+
+done:
+ user_access_end();
+ return res;
- /*
- * Nope: we hit the address space limit, and we still had more
- * characters the caller would have wanted. That's 0.
- */
efault:
+ user_access_end();
return 0;
}
@@ -100,7 +113,6 @@ long strnlen_user(const char __user *str, long count)
src_addr = (unsigned long)untagged_addr(str);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
- long retval;
/*
* Truncate 'max' to the user-specified limit, so that
@@ -109,11 +121,7 @@ long strnlen_user(const char __user *str, long count)
if (max > count)
max = count;
- if (user_access_begin(str, max)) {
- retval = do_strnlen_user(str, count, max);
- user_access_end();
- return retval;
- }
+ return do_strnlen_user(str, count, max);
}
return 0;
}
On Fri, May 1, 2020 at 3:07 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133: call to do_strncpy_from_user() with UACCESS enabled
> > lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to do_strnlen_user() with UACCESS enabled
>
> Does this fix it?
Yes, it does. Thanks!
Arnd
On Fri, May 1, 2020 at 2:29 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > lib/locking-selftest.o: warning: objtool: locking_selftest()+0x117f: PUSHF stack exhausted
> > lib/locking-selftest.o: warning: objtool: ww_tests()+0x1083: PUSHF stack exhausted
>
> Peter,
>
> These functions have a bunch of irqs_disabled() checks, which means a
> bunch of PUSHFs with no POPFs.
>
> Am I reading it correctly that objtool assumes PUSHF is always paired
> with POPF? irqs_disabled() doesn't do that.
I played around with this one a little more, it seems that the warning
is related to
the number of dotest() calls getting inlined into the functions. If I
comment out
a few of them (any five or more, occording to my non-scientific
tests), the warning
disappears, and it also goes away if I disable inlining that function:
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -1134,7 +1134,7 @@ static int testcase_successes;
static int expected_testcase_failures;
static int unexpected_testcase_failures;
-static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
+static noinline void dotest(void (*testcase_fn)(void), int expected,
int lockclass_mask)
{
unsigned long saved_preempt_count = preempt_count();
Arnd
On Wed, Apr 29, 2020 at 02:18:46PM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 11:10:44AM -0500, Josh Poimboeuf wrote:
> > > ==> build/x86/0x2EA4CE4F_defconfig/log <==
> > > kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
> > > kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] objtool: Add check_kcov_mode() to the uaccess safelist
>
> check_kcov_mode() is called by write_comp_data() and
> __sanitizer_cov_trace_pc(), which are already on the uaccess safe list.
> Might as well add check_kcov_mode() to the party.
Hurmph, the actual reason it's ok is because it's notrace and doesn't
call out to other stuff, that's the reason those other two functions got
away without having user_access_save()/restore() on.
The alternative fix would be to mark that check_kcov_mode() thing as
__always_inline, it's puny anyway.
> This fixes the following warnings:
>
> kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0x15: call to check_kcov_mode() with UACCESS enabled
> kernel/kcov.o: warning: objtool: write_comp_data()+0x1b: call to check_kcov_mode() with UACCESS enabled
>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> tools/objtool/check.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0c732d586924..fec890547e04 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -516,6 +516,7 @@ static const char *uaccess_safe_builtin[] = {
> "__tsan_write16",
> /* KCOV */
> "write_comp_data",
> + "check_kcov_mode",
> "__sanitizer_cov_trace_pc",
> "__sanitizer_cov_trace_const_cmp1",
> "__sanitizer_cov_trace_const_cmp2",
> --
> 2.21.1
>
On Thu, Apr 30, 2020 at 07:28:58PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > lib/locking-selftest.o: warning: objtool: locking_selftest()+0x117f: PUSHF stack exhausted
> > lib/locking-selftest.o: warning: objtool: ww_tests()+0x1083: PUSHF stack exhausted
>
> Peter,
>
> These functions have a bunch of irqs_disabled() checks, which means a
> bunch of PUSHFs with no POPFs.
>
> Am I reading it correctly that objtool assumes PUSHF is always paired
> with POPF? irqs_disabled() doesn't do that.
Right, it sorta does. I wonder why this shows up with GCC-10 though, and
not before.
Anyway, the sneaky fix here would be something like this.
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
index 14f44f59e733..510656c776d9 100644
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -1138,7 +1138,7 @@ static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
{
unsigned long saved_preempt_count = preempt_count();
- WARN_ON(irqs_disabled());
+ lockdep_assert_irqs_enabled();
testcase_fn();
/*
On Thu, Apr 30, 2020 at 08:07:33PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133: call to do_strncpy_from_user() with UACCESS enabled
> > lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to do_strnlen_user() with UACCESS enabled
>
> Does this fix it?
>
GCC is a moron, a static function with inline and a single callsite,
let's out-of-line it, shees!, try this instead:
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 706020b06617..be420c8c0fdd 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -24,7 +24,7 @@
* hit it), 'max' is the address space maximum (and we return
* -EFAULT if we hit it).
*/
-static inline long do_strncpy_from_user(char *dst, const char __user *src,
+static __always_inline long do_strncpy_from_user(char *dst, const char __user *src,
unsigned long count, unsigned long max)
{
const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 41670d4a5816..c996b745733e 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -20,7 +20,7 @@
* if it fits in a aligned 'long'. The caller needs to check
* the return value against "> max".
*/
-static inline long do_strnlen_user(const char __user *src, unsigned long count, unsigned long max)
+static __always_inline long do_strnlen_user(const char __user *src, unsigned long count, unsigned long max)
{
const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
unsigned long align, res = 0;
On Fri, May 1, 2020 at 2:33 PM Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Apr 30, 2020 at 08:07:33PM -0500, Josh Poimboeuf wrote:
> > On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > > lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133: call to do_strncpy_from_user() with UACCESS enabled
> > > lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to do_strnlen_user() with UACCESS enabled
> >
> > Does this fix it?
> >
>
> GCC is a moron, a static function with inline and a single callsite,
> let's out-of-line it, shees!, try this instead:
I suppose we were kind-of asking for it by passing
-fno-inline-functions-called-once
when CONFIG_DEBUG_SECTION_MISMATCH=y is set ;-)
Arnd
On Fri, May 01, 2020 at 02:33:19PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 30, 2020 at 08:07:33PM -0500, Josh Poimboeuf wrote:
> > On Thu, Apr 30, 2020 at 04:05:07PM +0200, Arnd Bergmann wrote:
> > > lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x133: call to do_strncpy_from_user() with UACCESS enabled
> > > lib/strnlen_user.o: warning: objtool: strnlen_user()+0x122: call to do_strnlen_user() with UACCESS enabled
> >
> > Does this fix it?
> >
>
> GCC is a moron, a static function with inline and a single callsite,
> let's out-of-line it, shees!, try this instead:
Yeah, that's easier :-)
--
Josh
On Fri, May 01, 2020 at 01:41:30PM +0200, Arnd Bergmann wrote:
> disappears, and it also goes away if I disable inlining that function:
Yes, makes sense. The state objtool tracks is strictly per function.
And I suppose GCC-10 just changed around the inline heuristc a bit and
we got lucky.
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1134,7 +1134,7 @@ static int testcase_successes;
> static int expected_testcase_failures;
> static int unexpected_testcase_failures;
>
> -static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
> +static noinline void dotest(void (*testcase_fn)(void), int expected,
> int lockclass_mask)
> {
> unsigned long saved_preempt_count = preempt_count();
>
> Arnd
On Thu, Apr 30, 2020 at 4:05 PM Arnd Bergmann <[email protected]> wrote:
> On Tue, Apr 28, 2020 at 4:49 PM Arnd Bergmann <[email protected]> wrote:
> drivers/media/dvb-frontends/rtl2832_sdr.o: warning: objtool: .text.unlikely: unexpected end of section
> drivers/media/dvb-frontends/rtl2832_sdr.o: warning: objtool: rtl2832_sdr_try_fmt_sdr_cap() falls through to next function
> rtl2832_sdr_s_fmt_sdr_cap.cold()
I had a look at this one and found this happens when gcc optimizes this loop
memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
for (i = 0; i < dev->num_formats; i++) {
if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
f->fmt.sdr.buffersize = formats[i].buffersize;
formats[] is a static array, so if num_formats is larger than
ARRAY_SIZE(formats),
it gets into undefined behavior and stops emitting code after the call to
__sanitizer_cov_trace_pc.
https://godbolt.org/z/h9Gx3S shows a reduced test case:
struct v4l2_sdr_format {
int pixelformat;
int buffersize;
};
struct rtl2832_sdr_format {
int pixelformat;
int buffersize;
};
static struct rtl2832_sdr_format formats[] = {{}, {}};
struct rtl2832_sdr_dev {
int num_formats;
};
void rtl2832_sdr_try_fmt_sdr_cap(struct v4l2_sdr_format *f,
struct rtl2832_sdr_dev *dev) {
int i = 0;
for (; i < dev->num_formats; i++)
if (formats[i].pixelformat)
f->buffersize = 0;
}
With this source change, the warning goes away:
diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c
b/drivers/media/dvb-frontends/rtl2832_sdr.c
index 60d1e59d2292..faae510985e0 100644
--- a/drivers/media/dvb-frontends/rtl2832_sdr.c
+++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
@@ -1150,7 +1150,7 @@ static int rtl2832_sdr_s_fmt_sdr_cap(struct file
*file, void *priv,
return -EBUSY;
memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
- for (i = 0; i < dev->num_formats; i++) {
+ for (i = 0; i < min(dev->num_formats, NUM_FORMATS); i++) {
if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
dev->pixelformat = formats[i].pixelformat;
dev->buffersize = formats[i].buffersize;
@@ -1178,7 +1178,7 @@ static int rtl2832_sdr_try_fmt_sdr_cap(struct
file *file, void *priv,
(char *)&f->fmt.sdr.pixelformat);
memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
- for (i = 0; i < dev->num_formats; i++) {
+ for (i = 0; i < min(dev->num_formats, NUM_FORMATS); i++) {
if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
f->fmt.sdr.buffersize = formats[i].buffersize;
return 0;
Do we consider this expected behavior on gcc's side, or is it something
that should not happen and needs a gcc bug report?
Arnd
On Fri, May 01, 2020 at 07:21:31PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 4:05 PM Arnd Bergmann <[email protected]> wrote:
> it gets into undefined behavior and stops emitting code after the call to
> Do we consider this expected behavior on gcc's side, or is it something
> that should not happen and needs a gcc bug report?
When it hits UB it is of course free to do whatever it damn well
pleases, but just stopping code gen seems a little extreme, at least
issue a WARN that something is up or so.
Not sure how the GCC folks feel about this though.
On Fri, May 01, 2020 at 07:26:16PM +0200, Peter Zijlstra wrote:
> On Fri, May 01, 2020 at 07:21:31PM +0200, Arnd Bergmann wrote:
> > On Thu, Apr 30, 2020 at 4:05 PM Arnd Bergmann <[email protected]> wrote:
> > it gets into undefined behavior and stops emitting code after the call to
>
> > Do we consider this expected behavior on gcc's side, or is it something
> > that should not happen and needs a gcc bug report?
>
> When it hits UB it is of course free to do whatever it damn well
> pleases, but just stopping code gen seems a little extreme, at least
> issue a WARN that something is up or so.
>
> Not sure how the GCC folks feel about this though.
When we've seen truncated code flow like this in the past, it's either
been a code bug (undefined behavior) or a GCC bug. So this is new.
Is it only seen with GCC_PLUGIN_SANCOV enabled? Maybe (hopefully) it's
an issue with the plugin and how it interacts with GCC 10.
--
Josh
On Fri, May 1, 2020 at 7:50 PM Josh Poimboeuf <[email protected]> wrote:
>
> On Fri, May 01, 2020 at 07:26:16PM +0200, Peter Zijlstra wrote:
> > On Fri, May 01, 2020 at 07:21:31PM +0200, Arnd Bergmann wrote:
> > > On Thu, Apr 30, 2020 at 4:05 PM Arnd Bergmann <[email protected]> wrote:
> > > it gets into undefined behavior and stops emitting code after the call to
> >
> > > Do we consider this expected behavior on gcc's side, or is it something
> > > that should not happen and needs a gcc bug report?
> >
> > When it hits UB it is of course free to do whatever it damn well
> > pleases, but just stopping code gen seems a little extreme, at least
> > issue a WARN that something is up or so.
> >
> > Not sure how the GCC folks feel about this though.
>
> When we've seen truncated code flow like this in the past, it's either
> been a code bug (undefined behavior) or a GCC bug. So this is new.
>
> Is it only seen with GCC_PLUGIN_SANCOV enabled? Maybe (hopefully) it's
> an issue with the plugin and how it interacts with GCC 10.
This is not the plugin but the built-in -fsanitize-coverage=trace-pc. With
the reduced test case, I can also reproduce it on gcc-9.2 and gcc-9.3 but
not on gcc-8.4.
So far I have not been able to reproduce it without
-fsanitize-coverage=trace-pc,
as the automated creduce tends to run off into undefined behavior unless
I tell it to ignore runs that produce the objtool warning without the
-fsanitize-coverage=trace-pc flag.
Arnd