* [PATCH v2 00/22] x86, objtool: several fixes/improvements
@ 2019-07-18 1:36 Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
` (21 more replies)
0 siblings, 22 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
v2:
- Use ud2 instead of calling kvm_spurious_fault() in vmx_vmenter() [Paolo]
- Fix ____kvm_handle_fault_on_reboot() comment [Paolo]
- Tweak "switch jump table" comment [Peter]
- Add review tags
Patches also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git objtool-many-fixes-v2
v1 was posted here:
https://lkml.kernel.org/r/cover.1563150885.git.jpoimboe@redhat.com
-------------------
There have been a lot of objtool bug reports lately, mainly related to
Clang and BPF. As part of fixing those bugs, I added some improvements
to objtool which uncovered yet more bugs (some kernel, some objtool).
I've given these patches a lot of testing with both GCC and Clang. More
compile testing of objtool would be appreciated, as the kbuild test
robot doesn't seem to be paying much attention to my branches lately.
There are still at least three outstanding issues:
1) With clang I see:
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x88: redundant UACCESS disable
I haven't dug into it yet.
2) There's also an issue in clang where a large switch table had a bunch
of unused (bad) entries. It's not a code correctness issue, but
hopefully it can get fixed in clang anyway. See patch 20/22 for more
details.
3) CONFIG_LIVEPATCH is causing some objtool "unreachable instruction"
warnings due to the new -flive-patching flag. I have some fixes
pending, but this patch set is already long enough.
Jann Horn (1):
objtool: Support repeated uses of the same C jump table
Josh Poimboeuf (21):
x86/paravirt: Fix callee-saved function ELF sizes
x86/kvm: Fix fastop function ELF metadata
x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2
x86/kvm: Don't call kvm_spurious_fault() from .fixup
x86/entry: Fix thunk function ELF sizes
x86/head/64: Annotate start_cpu0() as non-callable
x86/uaccess: Remove ELF function annotation from
copy_user_handle_tail()
x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()
x86/uaccess: Remove redundant CLACs in getuser/putuser error paths
bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
objtool: Add mcsafe_handle_tail() to the uaccess safe list
objtool: Track original function across branches
objtool: Refactor function alias logic
objtool: Warn on zero-length functions
objtool: Change dead_end_function() to return boolean
objtool: Do frame pointer check before dead end check
objtool: Refactor sibling call detection logic
objtool: Refactor jump table code
objtool: Fix seg fault on bad switch table entry
objtool: convert insn type to enum
objtool: Support conditional retpolines
arch/x86/entry/thunk_64.S | 5 +-
arch/x86/include/asm/kvm_host.h | 34 ++--
arch/x86/include/asm/paravirt.h | 1 +
arch/x86/kernel/head_64.S | 4 +-
arch/x86/kernel/kvm.c | 1 +
arch/x86/kvm/emulate.c | 44 +++--
arch/x86/kvm/vmx/vmenter.S | 6 +-
arch/x86/lib/copy_user_64.S | 2 +-
arch/x86/lib/getuser.S | 20 +--
arch/x86/lib/putuser.S | 29 +--
arch/x86/lib/usercopy_64.c | 2 +-
include/linux/compiler-gcc.h | 2 +
include/linux/compiler_types.h | 4 +
kernel/bpf/core.c | 2 +-
tools/objtool/arch.h | 36 ++--
tools/objtool/arch/x86/decode.c | 2 +-
tools/objtool/check.c | 308 ++++++++++++++++----------------
tools/objtool/check.h | 3 +-
tools/objtool/elf.c | 4 +-
tools/objtool/elf.h | 3 +-
20 files changed, 278 insertions(+), 234 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:07 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
` (20 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Juergen Gross
The __raw_callee_save_*() functions have an ELF symbol size of zero,
which confuses objtool and other tools.
Fixes a bunch of warnings like the following:
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_pte_val() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_pgd_val() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_make_pte() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_make_pgd() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Cc: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt.h | 1 +
arch/x86/kernel/kvm.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index c25c38a05c1c..d6f5ae2c79ab 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -746,6 +746,7 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
PV_RESTORE_ALL_CALLER_REGS \
FRAME_END \
"ret;" \
+ ".size " PV_THUNK_NAME(func) ", .-" PV_THUNK_NAME(func) ";" \
".popsection")
/* Get a reference to a callee-save function */
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 82caf01b63dd..6661bd2f08a6 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -838,6 +838,7 @@ asm(
"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
"setne %al;"
"ret;"
+".size __raw_callee_save___kvm_vcpu_is_preempted, .-__raw_callee_save___kvm_vcpu_is_preempted;"
".popsection");
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 02/22] x86/kvm: Fix fastop function ELF metadata
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:08 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2 Josh Poimboeuf
` (19 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Paolo Bonzini,
Radim Krčmář
Some of the fastop functions, e.g. em_setcc(), are actually just used as
global labels which point to blocks of functions. The global labels are
incorrectly annotated as functions. Also the functions themselves don't
have size annotations.
Fixes a bunch of warnings like the following:
arch/x86/kvm/emulate.o: warning: objtool: seto() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: em_setcc() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: setno() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: setc() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
---
arch/x86/kvm/emulate.c | 44 +++++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8e409ad448f9..718f7d9afedc 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -312,29 +312,42 @@ static void invalidate_registers(struct x86_emulate_ctxt *ctxt)
static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
-#define FOP_FUNC(name) \
+#define __FOP_FUNC(name) \
".align " __stringify(FASTOP_SIZE) " \n\t" \
".type " name ", @function \n\t" \
name ":\n\t"
-#define FOP_RET "ret \n\t"
+#define FOP_FUNC(name) \
+ __FOP_FUNC(#name)
+
+#define __FOP_RET(name) \
+ "ret \n\t" \
+ ".size " name ", .-" name "\n\t"
+
+#define FOP_RET(name) \
+ __FOP_RET(#name)
#define FOP_START(op) \
extern void em_##op(struct fastop *fake); \
asm(".pushsection .text, \"ax\" \n\t" \
".global em_" #op " \n\t" \
- FOP_FUNC("em_" #op)
+ ".align " __stringify(FASTOP_SIZE) " \n\t" \
+ "em_" #op ":\n\t"
#define FOP_END \
".popsection")
+#define __FOPNOP(name) \
+ __FOP_FUNC(name) \
+ __FOP_RET(name)
+
#define FOPNOP() \
- FOP_FUNC(__stringify(__UNIQUE_ID(nop))) \
- FOP_RET
+ __FOPNOP(__stringify(__UNIQUE_ID(nop)))
#define FOP1E(op, dst) \
- FOP_FUNC(#op "_" #dst) \
- "10: " #op " %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst) \
+ "10: " #op " %" #dst " \n\t" \
+ __FOP_RET(#op "_" #dst)
#define FOP1EEX(op, dst) \
FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
@@ -366,8 +379,9 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
FOP_END
#define FOP2E(op, dst, src) \
- FOP_FUNC(#op "_" #dst "_" #src) \
- #op " %" #src ", %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst "_" #src) \
+ #op " %" #src ", %" #dst " \n\t" \
+ __FOP_RET(#op "_" #dst "_" #src)
#define FASTOP2(op) \
FOP_START(op) \
@@ -405,8 +419,9 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
FOP_END
#define FOP3E(op, dst, src, src2) \
- FOP_FUNC(#op "_" #dst "_" #src "_" #src2) \
- #op " %" #src2 ", %" #src ", %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst "_" #src "_" #src2) \
+ #op " %" #src2 ", %" #src ", %" #dst " \n\t"\
+ __FOP_RET(#op "_" #dst "_" #src "_" #src2)
/* 3-operand, word-only, src2=cl */
#define FASTOP3WCL(op) \
@@ -423,7 +438,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
".type " #op ", @function \n\t" \
#op ": \n\t" \
#op " %al \n\t" \
- FOP_RET
+ __FOP_RET(#op)
asm(".pushsection .fixup, \"ax\"\n"
".global kvm_fastop_exception \n"
@@ -449,7 +464,10 @@ FOP_SETCC(setle)
FOP_SETCC(setnle)
FOP_END;
-FOP_START(salc) "pushf; sbb %al, %al; popf \n\t" FOP_RET
+FOP_START(salc)
+FOP_FUNC(salc)
+"pushf; sbb %al, %al; popf \n\t"
+FOP_RET(salc)
FOP_END;
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 8:17 ` Paolo Bonzini
2019-07-18 19:08 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
` (18 subsequent siblings)
21 siblings, 2 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Paolo Bonzini,
Radim Krčmář
Objtool reports the following:
arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
But frame pointers are necessarily broken anyway, because
__vmx_vcpu_run() clobbers RBP with the guest's value before calling
vmx_vmenter(). So calling without a frame pointer doesn't make things
any worse.
Make objtool happy by changing the call to a UD2.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
v2: ud2 instead of kvm_spurious_fault() [Paolo]
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
---
arch/x86/kvm/vmx/vmenter.S | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index d4cb1945b2e3..4010d519eb8c 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -54,9 +54,9 @@ ENTRY(vmx_vmenter)
ret
3: cmpb $0, kvm_rebooting
- jne 4f
- call kvm_spurious_fault
-4: ret
+ je 4f
+ ret
+4: ud2
.pushsection .fixup, "ax"
5: jmp 3b
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (2 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2 Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 8:22 ` Paolo Bonzini
2019-07-18 19:09 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
` (17 subsequent siblings)
21 siblings, 2 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Paolo Bonzini,
Radim Krčmář
After making a change to improve objtool's sibling call detection, it
started showing the following warning:
arch/x86/kvm/vmx/nested.o: warning: objtool: .fixup+0x15: sibling call from callable instruction with modified stack frame
The problem is the ____kvm_handle_fault_on_reboot() macro. It does a
fake call by pushing a fake RIP and doing a jump. That tricks the
unwinder into printing the function which triggered the exception,
rather than the .fixup code.
Instead of the hack to make it look like the original function made the
call, just change the macro so that the original function actually does
make the call. This allows removal of the hack, and also makes objtool
happy.
I triggered a vmx instruction exception and verified that the stack
trace is still sane:
kernel BUG at arch/x86/kvm/x86.c:358!
invalid opcode: 0000 [#1] SMP PTI
CPU: 28 PID: 4096 Comm: qemu-kvm Not tainted 5.2.0+ #16
Hardware name: Lenovo THINKSYSTEM SD530 -[7X2106Z000]-/-[7X2106Z000]-, BIOS -[TEE113Z-1.00]- 07/17/2017
RIP: 0010:kvm_spurious_fault+0x5/0x10
Code: 00 00 00 00 00 8b 44 24 10 89 d2 45 89 c9 48 89 44 24 10 8b 44 24 08 48 89 44 24 08 e9 d4 40 22 00 0f 1f 40 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 49 89 fd 41
RSP: 0018:ffffbf91c683bd00 EFLAGS: 00010246
RAX: 000061f040000000 RBX: ffff9e159c77bba0 RCX: ffff9e15a5c87000
RDX: 0000000665c87000 RSI: ffff9e15a5c87000 RDI: ffff9e159c77bba0
RBP: 0000000000000000 R08: 0000000000000000 R09: ffff9e15a5c87000
R10: 0000000000000000 R11: fffff8f2d99721c0 R12: ffff9e159c77bba0
R13: ffffbf91c671d960 R14: ffff9e159c778000 R15: 0000000000000000
FS: 00007fa341cbe700(0000) GS:ffff9e15b7400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fdd38356804 CR3: 00000006759de003 CR4: 00000000007606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
loaded_vmcs_init+0x4f/0xe0
alloc_loaded_vmcs+0x38/0xd0
vmx_create_vcpu+0xf7/0x600
kvm_vm_ioctl+0x5e9/0x980
? __switch_to_asm+0x40/0x70
? __switch_to_asm+0x34/0x70
? __switch_to_asm+0x40/0x70
? __switch_to_asm+0x34/0x70
? free_one_page+0x13f/0x4e0
do_vfs_ioctl+0xa4/0x630
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x55/0x1c0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fa349b1ee5b
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
v2: Fix ____kvm_handle_fault_on_reboot() comment [Paolo]
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
---
arch/x86/include/asm/kvm_host.h | 34 ++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0cc5b611a113..8282b8d41209 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1496,25 +1496,29 @@ enum {
#define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
#define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
+asmlinkage void __noreturn kvm_spurious_fault(void);
+
/*
* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
- * Trap the fault and ignore the instruction if that happens.
+ * Usually after catching the fault we just panic; during reboot
+ * instead the instruction is ignored.
*/
-asmlinkage void kvm_spurious_fault(void);
-
-#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
- "666: " insn "\n\t" \
- "668: \n\t" \
- ".pushsection .fixup, \"ax\" \n" \
- "667: \n\t" \
- cleanup_insn "\n\t" \
- "cmpb $0, kvm_rebooting \n\t" \
- "jne 668b \n\t" \
- __ASM_SIZE(push) " $666b \n\t" \
- "jmp kvm_spurious_fault \n\t" \
- ".popsection \n\t" \
- _ASM_EXTABLE(666b, 667b)
+#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
+ "666: \n\t" \
+ insn "\n\t" \
+ "jmp 668f \n\t" \
+ "667: \n\t" \
+ "call kvm_spurious_fault \n\t" \
+ "668: \n\t" \
+ ".pushsection .fixup, \"ax\" \n\t" \
+ "700: \n\t" \
+ cleanup_insn "\n\t" \
+ "cmpb $0, kvm_rebooting\n\t" \
+ "je 667b \n\t" \
+ "jmp 668b \n\t" \
+ ".popsection \n\t" \
+ _ASM_EXTABLE(666b, 700b)
#define __kvm_handle_fault_on_reboot(insn) \
____kvm_handle_fault_on_reboot(insn, "")
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 05/22] x86/entry: Fix thunk function ELF sizes
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (3 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:10 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
` (16 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
Fix the following warnings:
arch/x86/entry/thunk_64.o: warning: objtool: trace_hardirqs_on_thunk() is missing an ELF size annotation
arch/x86/entry/thunk_64.o: warning: objtool: trace_hardirqs_off_thunk() is missing an ELF size annotation
arch/x86/entry/thunk_64.o: warning: objtool: lockdep_sys_exit_thunk() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/thunk_64.S | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index cfdca8b42c70..cc20465b2867 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -12,9 +12,7 @@
/* rdi: arg1 ... normal C conventions. rax is saved/restored. */
.macro THUNK name, func, put_ret_addr_in_rdi=0
- .globl \name
- .type \name, @function
-\name:
+ ENTRY(\name)
pushq %rbp
movq %rsp, %rbp
@@ -35,6 +33,7 @@
call \func
jmp .L_restore
+ ENDPROC(\name)
_ASM_NOKPROBE(\name)
.endm
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 06/22] x86/head/64: Annotate start_cpu0() as non-callable
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (4 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:11 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
` (15 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
After an objtool improvement, it complains about the fact that
start_cpu0() jumps to code which has an LRET instruction.
arch/x86/kernel/head_64.o: warning: objtool: .head.text+0xe4: unsupported instruction in callable function
Technically, start_cpu0() is callable, but it acts nothing like a
callable function. Prevent objtool from treating it like one by
removing its ELF function annotation.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/head_64.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index bcd206c8ac90..66b4a7757397 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -253,10 +253,10 @@ END(secondary_startup_64)
* start_secondary() via .Ljump_to_C_code.
*/
ENTRY(start_cpu0)
- movq initial_stack(%rip), %rsp
UNWIND_HINT_EMPTY
+ movq initial_stack(%rip), %rsp
jmp .Ljump_to_C_code
-ENDPROC(start_cpu0)
+END(start_cpu0)
#endif
/* Both SMP bootup and ACPI suspend change these variables */
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (5 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:11 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
` (14 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
After an objtool improvement, it's complaining about the CLAC in
copy_user_handle_tail():
arch/x86/lib/copy_user_64.o: warning: objtool: .altinstr_replacement+0x12: redundant UACCESS disable
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x6: (alt)
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x2: (alt)
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x0: <=== (func)
copy_user_handle_tail() is incorrectly marked as a callable function, so
objtool is rightfully concerned about the CLAC with no corresponding
STAC.
Remove the ELF function annotation. The copy_user_handle_tail() code
path is already verified by objtool because it's jumped to by other
callable asm code (which does the corresponding STAC).
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/lib/copy_user_64.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index 378a1f70ae7d..4fe1601dbc5d 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -239,7 +239,7 @@ copy_user_handle_tail:
ret
_ASM_EXTABLE_UA(1b, 2b)
-ENDPROC(copy_user_handle_tail)
+END(copy_user_handle_tail)
/*
* copy_user_nocache - Uncached memory copy with exception handling
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (6 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:12 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
` (13 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
After adding mcsafe_handle_tail() to the objtool uaccess safe list,
objtool reports:
arch/x86/lib/usercopy_64.o: warning: objtool: mcsafe_handle_tail()+0x0: call to __fentry__() with UACCESS enabled
With SMAP, this function is called with AC=1, so it needs to be careful
about which functions it calls. Disable the ftrace entry hook, which
can potentially pull in a lot of extra code.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/lib/usercopy_64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index e0e006f1624e..fff28c6f73a2 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -60,7 +60,7 @@ EXPORT_SYMBOL(clear_user);
* but reuse __memcpy_mcsafe in case a new read error is encountered.
* clac() is handled in _copy_to_iter_mcsafe().
*/
-__visible unsigned long
+__visible notrace unsigned long
mcsafe_handle_tail(char *to, char *from, unsigned len)
{
for (; len; --len, to++, from++) {
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (7 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:13 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
` (12 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
The same getuser/putuser error paths are used regardless of whether AC
is set. In non-exception failure cases, this results in an unnecessary
CLAC.
Fixes the following warnings:
arch/x86/lib/getuser.o: warning: objtool: .altinstr_replacement+0x18: redundant UACCESS disable
arch/x86/lib/putuser.o: warning: objtool: .altinstr_replacement+0x18: redundant UACCESS disable
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/lib/getuser.S | 20 ++++++++++----------
arch/x86/lib/putuser.S | 29 ++++++++++++++++-------------
2 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index 74fdff968ea3..304f958c27b2 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -115,29 +115,29 @@ ENDPROC(__get_user_8)
EXPORT_SYMBOL(__get_user_8)
+bad_get_user_clac:
+ ASM_CLAC
bad_get_user:
xor %edx,%edx
mov $(-EFAULT),%_ASM_AX
- ASM_CLAC
ret
-END(bad_get_user)
#ifdef CONFIG_X86_32
+bad_get_user_8_clac:
+ ASM_CLAC
bad_get_user_8:
xor %edx,%edx
xor %ecx,%ecx
mov $(-EFAULT),%_ASM_AX
- ASM_CLAC
ret
-END(bad_get_user_8)
#endif
- _ASM_EXTABLE_UA(1b, bad_get_user)
- _ASM_EXTABLE_UA(2b, bad_get_user)
- _ASM_EXTABLE_UA(3b, bad_get_user)
+ _ASM_EXTABLE_UA(1b, bad_get_user_clac)
+ _ASM_EXTABLE_UA(2b, bad_get_user_clac)
+ _ASM_EXTABLE_UA(3b, bad_get_user_clac)
#ifdef CONFIG_X86_64
- _ASM_EXTABLE_UA(4b, bad_get_user)
+ _ASM_EXTABLE_UA(4b, bad_get_user_clac)
#else
- _ASM_EXTABLE_UA(4b, bad_get_user_8)
- _ASM_EXTABLE_UA(5b, bad_get_user_8)
+ _ASM_EXTABLE_UA(4b, bad_get_user_8_clac)
+ _ASM_EXTABLE_UA(5b, bad_get_user_8_clac)
#endif
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index d2e5c9c39601..14bf78341d3c 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -32,8 +32,6 @@
*/
#define ENTER mov PER_CPU_VAR(current_task), %_ASM_BX
-#define EXIT ASM_CLAC ; \
- ret
.text
ENTRY(__put_user_1)
@@ -43,7 +41,8 @@ ENTRY(__put_user_1)
ASM_STAC
1: movb %al,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_1)
EXPORT_SYMBOL(__put_user_1)
@@ -56,7 +55,8 @@ ENTRY(__put_user_2)
ASM_STAC
2: movw %ax,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_2)
EXPORT_SYMBOL(__put_user_2)
@@ -69,7 +69,8 @@ ENTRY(__put_user_4)
ASM_STAC
3: movl %eax,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_4)
EXPORT_SYMBOL(__put_user_4)
@@ -85,19 +86,21 @@ ENTRY(__put_user_8)
5: movl %edx,4(%_ASM_CX)
#endif
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ RET
ENDPROC(__put_user_8)
EXPORT_SYMBOL(__put_user_8)
+bad_put_user_clac:
+ ASM_CLAC
bad_put_user:
movl $-EFAULT,%eax
- EXIT
-END(bad_put_user)
+ RET
- _ASM_EXTABLE_UA(1b, bad_put_user)
- _ASM_EXTABLE_UA(2b, bad_put_user)
- _ASM_EXTABLE_UA(3b, bad_put_user)
- _ASM_EXTABLE_UA(4b, bad_put_user)
+ _ASM_EXTABLE_UA(1b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(2b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(3b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(4b, bad_put_user_clac)
#ifdef CONFIG_X86_32
- _ASM_EXTABLE_UA(5b, bad_put_user)
+ _ASM_EXTABLE_UA(5b, bad_put_user_clac)
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (8 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:14 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
` (11 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Alexei Starovoitov,
Daniel Borkmann
On x86-64, with CONFIG_RETPOLINE=n, GCC's "global common subexpression
elimination" optimization results in ___bpf_prog_run()'s jumptable code
changing from this:
select_insn:
jmp *jumptable(, %rax, 8)
...
ALU64_ADD_X:
...
jmp *jumptable(, %rax, 8)
ALU_ADD_X:
...
jmp *jumptable(, %rax, 8)
to this:
select_insn:
mov jumptable, %r12
jmp *(%r12, %rax, 8)
...
ALU64_ADD_X:
...
jmp *(%r12, %rax, 8)
ALU_ADD_X:
...
jmp *(%r12, %rax, 8)
The jumptable address is placed in a register once, at the beginning of
the function. The function execution can then go through multiple
indirect jumps which rely on that same register value. This has a few
issues:
1) Objtool isn't smart enough to be able to track such a register value
across multiple recursive indirect jumps through the jump table.
2) With CONFIG_RETPOLINE enabled, this optimization actually results in
a small slowdown. I measured a ~4.7% slowdown in the test_bpf
"tcpdump port 22" selftest.
This slowdown is actually predicted by the GCC manual:
Note: When compiling a program using computed gotos, a GCC
extension, you may get better run-time performance if you
disable the global common subexpression elimination pass by
adding -fno-gcse to the command line.
So just disable the optimization for this function.
Fixes: e55a73251da3 ("bpf: Fix ORC unwinding in non-JIT BPF code")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
include/linux/compiler-gcc.h | 2 ++
include/linux/compiler_types.h | 4 ++++
kernel/bpf/core.c | 2 +-
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e8579412ad21..d7ee4c6bad48 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -170,3 +170,5 @@
#else
#define __diag_GCC_8(s)
#endif
+
+#define __no_fgcse __attribute__((optimize("-fno-gcse")))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 095d55c3834d..599c27b56c29 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -189,6 +189,10 @@ struct ftrace_likely_data {
#define asm_volatile_goto(x...) asm goto(x)
#endif
+#ifndef __no_fgcse
+# define __no_fgcse
+#endif
+
/* Are two types/vars the same type (ignoring qualifiers)? */
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 7e98f36a14e2..8191a7db2777 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1295,7 +1295,7 @@ bool bpf_opcode_in_insntable(u8 code)
*
* Decode and execute eBPF instructions.
*/
-static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
+static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
{
#define BPF_INSN_2_LBL(x, y) [BPF_##x | BPF_##y] = &&x##_##y
#define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (9 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:14 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 12/22] objtool: Track original function across branches Josh Poimboeuf
` (10 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
After an objtool improvement, it's reporting that __memcpy_mcsafe() is
calling mcsafe_handle_tail() with AC=1:
arch/x86/lib/memcpy_64.o: warning: objtool: .fixup+0x13: call to mcsafe_handle_tail() with UACCESS enabled
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0x34: (alt)
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0xb: (branch)
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0x0: <=== (func)
mcsafe_handle_tail() is basically an extension of __memcpy_mcsafe(), so
AC=1 is supposed to be set. Add mcsafe_handle_tail() to the uaccess
safe list.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 27818a93f0b1..fd8827114c74 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -490,6 +490,7 @@ static const char *uaccess_safe_builtin[] = {
/* misc */
"csum_partial_copy_generic",
"__memcpy_mcsafe",
+ "mcsafe_handle_tail",
"ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
NULL
};
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 12/22] objtool: Track original function across branches
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (10 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:15 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 13/22] objtool: Refactor function alias logic Josh Poimboeuf
` (9 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
If 'insn->func' is NULL, objtool skips some important checks, including
sibling call validation. So if some .fixup code does an invalid sibling
call, objtool ignores it.
Treat all code branches (including alts) as part of the original
function by keeping track of the original func value from
validate_functions().
This improves the usefulness of some clang function fallthrough
warnings, and exposes some additional kernel bugs in the process.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index fd8827114c74..bb9cfda670fd 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1934,13 +1934,12 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
* each instruction and validate all the rules described in
* tools/objtool/Documentation/stack-validation.txt.
*/
-static int validate_branch(struct objtool_file *file, struct instruction *first,
- struct insn_state state)
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *first, struct insn_state state)
{
struct alternative *alt;
struct instruction *insn, *next_insn;
struct section *sec;
- struct symbol *func = NULL;
int ret;
insn = first;
@@ -1961,9 +1960,6 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
return 1;
}
- if (insn->func)
- func = insn->func->pfunc;
-
if (func && insn->ignore) {
WARN_FUNC("BUG: why am I validating an ignored function?",
sec, insn->offset);
@@ -1985,7 +1981,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
i = insn;
save_insn = NULL;
- func_for_each_insn_continue_reverse(file, insn->func, i) {
+ func_for_each_insn_continue_reverse(file, func, i) {
if (i->save) {
save_insn = i;
break;
@@ -2031,7 +2027,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
if (alt->skip_orig)
skip_orig = true;
- ret = validate_branch(file, alt->insn, state);
+ ret = validate_branch(file, func, alt->insn, state);
if (ret) {
if (backtrace)
BT_FUNC("(alt)", insn);
@@ -2069,7 +2065,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
if (state.bp_scratch) {
WARN("%s uses BP as a scratch register",
- insn->func->name);
+ func->name);
return 1;
}
@@ -2109,8 +2105,8 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
} else if (insn->jump_dest &&
(!func || !insn->jump_dest->func ||
insn->jump_dest->func->pfunc == func)) {
- ret = validate_branch(file, insn->jump_dest,
- state);
+ ret = validate_branch(file, func,
+ insn->jump_dest, state);
if (ret) {
if (backtrace)
BT_FUNC("(branch)", insn);
@@ -2176,7 +2172,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
break;
case INSN_CLAC:
- if (!state.uaccess && insn->func) {
+ if (!state.uaccess && func) {
WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
return 1;
}
@@ -2197,7 +2193,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
break;
case INSN_CLD:
- if (!state.df && insn->func)
+ if (!state.df && func)
WARN_FUNC("redundant CLD", sec, insn->offset);
state.df = false;
@@ -2236,7 +2232,7 @@ static int validate_unwind_hints(struct objtool_file *file)
for_each_insn(file, insn) {
if (insn->hint && !insn->visited) {
- ret = validate_branch(file, insn, state);
+ ret = validate_branch(file, insn->func, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (hint)", insn);
warnings += ret;
@@ -2363,12 +2359,12 @@ static int validate_functions(struct objtool_file *file)
continue;
insn = find_insn(file, sec, func->offset);
- if (!insn || insn->ignore)
+ if (!insn || insn->ignore || insn->visited)
continue;
state.uaccess = func->alias->uaccess_safe;
- ret = validate_branch(file, insn, state);
+ ret = validate_branch(file, func, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (func)", insn);
warnings += ret;
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 13/22] objtool: Refactor function alias logic
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (11 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 12/22] objtool: Track original function across branches Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:16 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
` (8 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
- Add an alias check in validate_functions(). With this change, aliases
no longer need uaccess_safe set.
- Add an alias check in decode_instructions(). With this change, the
"if (!insn->func)" check is no longer needed.
- Don't create aliases for zero-length functions, as it can have
unexpected results. The next patch will spit out a warning for
zero-length functions anyway.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 16 +++++++++-------
tools/objtool/elf.c | 2 +-
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bb9cfda670fd..9bf4844d9226 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -276,7 +276,7 @@ static int decode_instructions(struct objtool_file *file)
}
list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC)
+ if (func->type != STT_FUNC || func->alias != func)
continue;
if (!find_insn(file, sec, func->offset)) {
@@ -286,8 +286,7 @@ static int decode_instructions(struct objtool_file *file)
}
func_for_each_insn(file, func, insn)
- if (!insn->func)
- insn->func = func;
+ insn->func = func;
}
}
@@ -508,7 +507,7 @@ static void add_uaccess_safe(struct objtool_file *file)
if (!func)
continue;
- func->alias->uaccess_safe = true;
+ func->uaccess_safe = true;
}
}
@@ -1887,7 +1886,7 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state)
static inline bool func_uaccess_safe(struct symbol *func)
{
if (func)
- return func->alias->uaccess_safe;
+ return func->uaccess_safe;
return false;
}
@@ -2355,14 +2354,17 @@ static int validate_functions(struct objtool_file *file)
for_each_sec(file, sec) {
list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC || func->pfunc != func)
+ if (func->type != STT_FUNC)
+ continue;
+
+ if (func->pfunc != func || func->alias != func)
continue;
insn = find_insn(file, sec, func->offset);
if (!insn || insn->ignore || insn->visited)
continue;
- state.uaccess = func->alias->uaccess_safe;
+ state.uaccess = func->uaccess_safe;
ret = validate_branch(file, func, insn, state);
if (ret && backtrace)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index e99e1be19ad9..d2c211b0a5a0 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -278,7 +278,7 @@ static int read_symbols(struct elf *elf)
}
if (sym->offset == s->offset) {
- if (sym->len == s->len && alias == sym)
+ if (sym->len && sym->len == s->len && alias == sym)
alias = s;
if (sym->len >= s->len) {
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 14/22] objtool: Warn on zero-length functions
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (12 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 13/22] objtool: Refactor function alias logic Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:17 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
` (7 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
All callable functions should have an ELF size.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9bf4844d9226..16454cbca679 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2357,6 +2357,12 @@ static int validate_functions(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
+ if (!func->len) {
+ WARN("%s() is missing an ELF size annotation",
+ func->name);
+ warnings++;
+ }
+
if (func->pfunc != func || func->alias != func)
continue;
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 15/22] objtool: Change dead_end_function() to return boolean
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (13 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:17 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
` (6 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
dead_end_function() can no longer return an error. Simplify its
interface by making it return boolean.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 36 ++++++++++++++----------------------
1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 16454cbca679..970dfeac841d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -105,14 +105,9 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
*
* For local functions, we have to detect them manually by simply looking for
* the lack of a return instruction.
- *
- * Returns:
- * -1: error
- * 0: no dead end
- * 1: dead end
*/
-static int __dead_end_function(struct objtool_file *file, struct symbol *func,
- int recursion)
+static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
+ int recursion)
{
int i;
struct instruction *insn;
@@ -139,29 +134,29 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
};
if (func->bind == STB_WEAK)
- return 0;
+ return false;
if (func->bind == STB_GLOBAL)
for (i = 0; i < ARRAY_SIZE(global_noreturns); i++)
if (!strcmp(func->name, global_noreturns[i]))
- return 1;
+ return true;
if (!func->len)
- return 0;
+ return false;
insn = find_insn(file, func->sec, func->offset);
if (!insn->func)
- return 0;
+ return false;
func_for_each_insn_all(file, func, insn) {
empty = false;
if (insn->type == INSN_RETURN)
- return 0;
+ return false;
}
if (empty)
- return 0;
+ return false;
/*
* A function can have a sibling call instead of a return. In that
@@ -174,7 +169,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
if (!dest)
/* sibling call to another file */
- return 0;
+ return false;
if (dest->func && dest->func->pfunc != insn->func->pfunc) {
@@ -186,7 +181,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
* This is a very rare case. It means
* they aren't dead ends.
*/
- return 0;
+ return false;
}
return __dead_end_function(file, dest->func,
@@ -196,13 +191,13 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
if (insn->type == INSN_JUMP_DYNAMIC && list_empty(&insn->alts))
/* sibling call */
- return 0;
+ return false;
}
- return 1;
+ return true;
}
-static int dead_end_function(struct objtool_file *file, struct symbol *func)
+static bool dead_end_function(struct objtool_file *file, struct symbol *func)
{
return __dead_end_function(file, func, 0);
}
@@ -2080,11 +2075,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (is_fentry_call(insn))
break;
- ret = dead_end_function(file, insn->call_dest);
- if (ret == 1)
+ if (dead_end_function(file, insn->call_dest))
return 0;
- if (ret == -1)
- return 1;
}
if (!no_fp && func && !has_valid_stack_frame(&state)) {
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 16/22] objtool: Do frame pointer check before dead end check
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (14 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:18 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
` (5 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
Even calls to __noreturn functions need the frame pointer setup first.
Such functions often dump the stack.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 970dfeac841d..3fb656ea96b9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -133,6 +133,9 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"rewind_stack_do_exit",
};
+ if (!func)
+ return false;
+
if (func->bind == STB_WEAK)
return false;
@@ -2071,19 +2074,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (ret)
return ret;
- if (insn->type == INSN_CALL) {
- if (is_fentry_call(insn))
- break;
-
- if (dead_end_function(file, insn->call_dest))
- return 0;
- }
-
- if (!no_fp && func && !has_valid_stack_frame(&state)) {
+ if (!no_fp && func && !is_fentry_call(insn) &&
+ !has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
return 1;
}
+
+ if (dead_end_function(file, insn->call_dest))
+ return 0;
+
break;
case INSN_JUMP_CONDITIONAL:
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 17/22] objtool: Refactor sibling call detection logic
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (15 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:19 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 18/22] objtool: Refactor jump table code Josh Poimboeuf
` (4 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
Simplify the sibling call detection logic a bit.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 65 ++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 32 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3fb656ea96b9..a190a6e79a91 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -97,6 +97,20 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
for (insn = next_insn_same_sec(file, insn); insn; \
insn = next_insn_same_sec(file, insn))
+static bool is_sibling_call(struct instruction *insn)
+{
+ /* An indirect jump is either a sibling call or a jump to a table. */
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return list_empty(&insn->alts);
+
+ if (insn->type != INSN_JUMP_CONDITIONAL &&
+ insn->type != INSN_JUMP_UNCONDITIONAL)
+ return false;
+
+ /* add_jump_destinations() sets insn->call_dest for sibling calls. */
+ return !!insn->call_dest;
+}
+
/*
* This checks to see if the given function is a "noreturn" function.
*
@@ -167,34 +181,25 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
* of the sibling call returns.
*/
func_for_each_insn_all(file, func, insn) {
- if (insn->type == INSN_JUMP_UNCONDITIONAL) {
+ if (is_sibling_call(insn)) {
struct instruction *dest = insn->jump_dest;
if (!dest)
/* sibling call to another file */
return false;
- if (dest->func && dest->func->pfunc != insn->func->pfunc) {
-
- /* local sibling call */
- if (recursion == 5) {
- /*
- * Infinite recursion: two functions
- * have sibling calls to each other.
- * This is a very rare case. It means
- * they aren't dead ends.
- */
- return false;
- }
-
- return __dead_end_function(file, dest->func,
- recursion + 1);
+ /* local sibling call */
+ if (recursion == 5) {
+ /*
+ * Infinite recursion: two functions have
+ * sibling calls to each other. This is a very
+ * rare case. It means they aren't dead ends.
+ */
+ return false;
}
- }
- if (insn->type == INSN_JUMP_DYNAMIC && list_empty(&insn->alts))
- /* sibling call */
- return false;
+ return __dead_end_function(file, dest->func, recursion+1);
+ }
}
return true;
@@ -581,9 +586,8 @@ static int add_jump_destinations(struct objtool_file *file)
insn->retpoline_safe = true;
continue;
} else {
- /* sibling call */
+ /* external sibling call */
insn->call_dest = rela->sym;
- insn->jump_dest = NULL;
continue;
}
@@ -633,9 +637,8 @@ static int add_jump_destinations(struct objtool_file *file)
} else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
insn->jump_dest->offset == insn->jump_dest->func->offset) {
- /* sibling class */
+ /* internal sibling call */
insn->call_dest = insn->jump_dest->func;
- insn->jump_dest = NULL;
}
}
}
@@ -1889,7 +1892,7 @@ static inline bool func_uaccess_safe(struct symbol *func)
return false;
}
-static inline const char *insn_dest_name(struct instruction *insn)
+static inline const char *call_dest_name(struct instruction *insn)
{
if (insn->call_dest)
return insn->call_dest->name;
@@ -1901,13 +1904,13 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
{
if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
WARN_FUNC("call to %s() with UACCESS enabled",
- insn->sec, insn->offset, insn_dest_name(insn));
+ insn->sec, insn->offset, call_dest_name(insn));
return 1;
}
if (state->df) {
WARN_FUNC("call to %s() with DF set",
- insn->sec, insn->offset, insn_dest_name(insn));
+ insn->sec, insn->offset, call_dest_name(insn));
return 1;
}
@@ -2088,14 +2091,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
case INSN_JUMP_CONDITIONAL:
case INSN_JUMP_UNCONDITIONAL:
- if (func && !insn->jump_dest) {
+ if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
- } else if (insn->jump_dest &&
- (!func || !insn->jump_dest->func ||
- insn->jump_dest->func->pfunc == func)) {
+ } else if (insn->jump_dest) {
ret = validate_branch(file, func,
insn->jump_dest, state);
if (ret) {
@@ -2111,7 +2112,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_JUMP_DYNAMIC:
- if (func && list_empty(&insn->alts)) {
+ if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 18/22] objtool: Refactor jump table code
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (16 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:20 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
` (3 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
Now that C jump tables are supported, call them "jump tables" instead of
"switch tables". Also rename some other variables, add comments, and
simplify the code flow a bit.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
v2:
- comment tweak: "switch jump table" -> "jump table" [PeterZ]
---
tools/objtool/check.c | 82 +++++++++++++++++++++++--------------------
tools/objtool/elf.c | 2 +-
tools/objtool/elf.h | 2 +-
3 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a190a6e79a91..a904f98236b4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -627,7 +627,7 @@ static int add_jump_destinations(struct objtool_file *file)
* 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 switch table.
+ * subfunction is through a jump table.
*/
if (!strstr(insn->func->name, ".cold.") &&
strstr(insn->jump_dest->func->name, ".cold.")) {
@@ -899,20 +899,24 @@ static int add_special_section_alts(struct objtool_file *file)
return ret;
}
-static int add_switch_table(struct objtool_file *file, struct instruction *insn,
+static int add_jump_table(struct objtool_file *file, struct instruction *insn,
struct rela *table, struct rela *next_table)
{
struct rela *rela = table;
- struct instruction *alt_insn;
+ struct instruction *dest_insn;
struct alternative *alt;
struct symbol *pfunc = insn->func->pfunc;
unsigned int prev_offset = 0;
- list_for_each_entry_from(rela, &table->rela_sec->rela_list, list) {
+ /*
+ * Each @rela is a switch table relocation which points to the target
+ * instruction.
+ */
+ list_for_each_entry_from(rela, &table->sec->rela_list, list) {
if (rela == next_table)
break;
- /* Make sure the switch table entries are consecutive: */
+ /* Make sure the table entries are consecutive: */
if (prev_offset && rela->offset != prev_offset + 8)
break;
@@ -921,12 +925,12 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
rela->addend == pfunc->offset)
break;
- alt_insn = find_insn(file, rela->sym->sec, rela->addend);
- if (!alt_insn)
+ dest_insn = find_insn(file, rela->sym->sec, rela->addend);
+ if (!dest_insn)
break;
- /* Make sure the jmp dest is in the function or subfunction: */
- if (alt_insn->func->pfunc != pfunc)
+ /* Make sure the destination is in the same function: */
+ if (dest_insn->func->pfunc != pfunc)
break;
alt = malloc(sizeof(*alt));
@@ -935,7 +939,7 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
return -1;
}
- alt->insn = alt_insn;
+ alt->insn = dest_insn;
list_add_tail(&alt->list, &insn->alts);
prev_offset = rela->offset;
}
@@ -950,7 +954,7 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
}
/*
- * find_switch_table() - Given a dynamic jump, find the switch jump table in
+ * find_jump_table() - Given a dynamic jump, find the switch jump table in
* .rodata associated with it.
*
* There are 3 basic patterns:
@@ -992,13 +996,13 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
*
* NOTE: RETPOLINE made it harder still to decode dynamic jumps.
*/
-static struct rela *find_switch_table(struct objtool_file *file,
+static struct rela *find_jump_table(struct objtool_file *file,
struct symbol *func,
struct instruction *insn)
{
- struct rela *text_rela, *rodata_rela;
+ struct rela *text_rela, *table_rela;
struct instruction *orig_insn = insn;
- struct section *rodata_sec;
+ struct section *table_sec;
unsigned long table_offset;
/*
@@ -1031,7 +1035,7 @@ static struct rela *find_switch_table(struct objtool_file *file,
continue;
table_offset = text_rela->addend;
- rodata_sec = text_rela->sym->sec;
+ table_sec = text_rela->sym->sec;
if (text_rela->type == R_X86_64_PC32)
table_offset += 4;
@@ -1045,29 +1049,31 @@ static struct rela *find_switch_table(struct objtool_file *file,
* need to be placed in the C_JUMP_TABLE_SECTION section. They
* have symbols associated with them.
*/
- if (find_symbol_containing(rodata_sec, table_offset) &&
- strcmp(rodata_sec->name, C_JUMP_TABLE_SECTION))
+ if (find_symbol_containing(table_sec, table_offset) &&
+ strcmp(table_sec->name, C_JUMP_TABLE_SECTION))
continue;
- rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
- if (rodata_rela) {
- /*
- * Use of RIP-relative switch jumps is quite rare, and
- * indicates a rare GCC quirk/bug which can leave dead
- * code behind.
- */
- if (text_rela->type == R_X86_64_PC32)
- file->ignore_unreachables = true;
+ /* Each table entry has a rela associated with it. */
+ table_rela = find_rela_by_dest(table_sec, table_offset);
+ if (!table_rela)
+ continue;
- return rodata_rela;
- }
+ /*
+ * Use of RIP-relative switch jumps is quite rare, and
+ * indicates a rare GCC quirk/bug which can leave dead code
+ * behind.
+ */
+ if (text_rela->type == R_X86_64_PC32)
+ file->ignore_unreachables = true;
+
+ return table_rela;
}
return NULL;
}
-static int add_func_switch_tables(struct objtool_file *file,
+static int add_func_jump_tables(struct objtool_file *file,
struct symbol *func)
{
struct instruction *insn, *last = NULL, *prev_jump = NULL;
@@ -1080,7 +1086,7 @@ static int add_func_switch_tables(struct objtool_file *file,
/*
* Store back-pointers for unconditional forward jumps such
- * that find_switch_table() can back-track using those and
+ * that find_jump_table() can back-track using those and
* avoid some potentially confusing code.
*/
if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->jump_dest &&
@@ -1095,17 +1101,17 @@ static int add_func_switch_tables(struct objtool_file *file,
if (insn->type != INSN_JUMP_DYNAMIC)
continue;
- rela = find_switch_table(file, func, insn);
+ rela = find_jump_table(file, func, insn);
if (!rela)
continue;
/*
- * We found a switch table, but we don't know yet how big it
+ * We found a jump table, but we don't know yet how big it
* is. Don't add it until we reach the end of the function or
- * the beginning of another switch table in the same function.
+ * the beginning of another jump table in the same function.
*/
if (prev_jump) {
- ret = add_switch_table(file, prev_jump, prev_rela, rela);
+ ret = add_jump_table(file, prev_jump, prev_rela, rela);
if (ret)
return ret;
}
@@ -1115,7 +1121,7 @@ static int add_func_switch_tables(struct objtool_file *file,
}
if (prev_jump) {
- ret = add_switch_table(file, prev_jump, prev_rela, NULL);
+ ret = add_jump_table(file, prev_jump, prev_rela, NULL);
if (ret)
return ret;
}
@@ -1128,7 +1134,7 @@ static int add_func_switch_tables(struct objtool_file *file,
* section which contains a list of addresses within the function to jump to.
* This finds these jump tables and adds them to the insn->alts lists.
*/
-static int add_switch_table_alts(struct objtool_file *file)
+static int add_jump_table_alts(struct objtool_file *file)
{
struct section *sec;
struct symbol *func;
@@ -1142,7 +1148,7 @@ static int add_switch_table_alts(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
- ret = add_func_switch_tables(file, func);
+ ret = add_func_jump_tables(file, func);
if (ret)
return ret;
}
@@ -1339,7 +1345,7 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
- ret = add_switch_table_alts(file);
+ ret = add_jump_table_alts(file);
if (ret)
return ret;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index d2c211b0a5a0..4650f5d3fff2 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -385,7 +385,7 @@ static int read_relas(struct elf *elf)
rela->offset = rela->rela.r_offset;
symndx = GELF_R_SYM(rela->rela.r_info);
rela->sym = find_symbol_by_index(elf, symndx);
- rela->rela_sec = sec;
+ rela->sec = sec;
if (!rela->sym) {
WARN("can't find rela entry symbol %d for %s",
symndx, sec->name);
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index e44ca5d51871..1b638de4e7c0 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -57,7 +57,7 @@ struct rela {
struct list_head list;
struct hlist_node hash;
GElf_Rela rela;
- struct section *rela_sec;
+ struct section *sec;
struct symbol *sym;
unsigned int type;
unsigned long offset;
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 19/22] objtool: Support repeated uses of the same C jump table
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (17 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 18/22] objtool: Refactor jump table code Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:20 ` [tip:core/urgent] " tip-bot for Jann Horn
2019-07-18 1:36 ` [PATCH v2 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
` (2 subsequent siblings)
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
From: Jann Horn <jannh@google.com>
This fixes objtool for both a GCC issue and a Clang issue:
1) GCC issue:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x8d5: sibling call from callable instruction with modified stack frame
With CONFIG_RETPOLINE=n, GCC is doing the following optimization in
___bpf_prog_run().
Before:
select_insn:
jmp *jumptable(,%rax,8)
...
ALU64_ADD_X:
...
jmp select_insn
ALU_ADD_X:
...
jmp select_insn
After:
select_insn:
jmp *jumptable(, %rax, 8)
...
ALU64_ADD_X:
...
jmp *jumptable(, %rax, 8)
ALU_ADD_X:
...
jmp *jumptable(, %rax, 8)
This confuses objtool. It has never seen multiple indirect jump
sites which use the same jump table.
For GCC switch tables, the only way of detecting the size of a table
is by continuing to scan for more tables. The size of the previous
table can only be determined after another switch table is found, or
when the scan reaches the end of the function.
That logic was reused for C jump tables, and was based on the
assumption that each jump table only has a single jump site. The
above optimization breaks that assumption.
2) Clang issue:
drivers/usb/misc/sisusbvga/sisusb.o: warning: objtool: sisusb_write_mem_bulk()+0x588: can't find switch jump table
With clang 9, code can be generated where a function contains two
indirect jump instructions which use the same switch table.
The fix is the same for both issues: split the jump table parsing into
two passes.
In the first pass, locate the heads of all switch tables for the
function and mark their locations.
In the second pass, parse the switch tables and add them.
Fixes: e55a73251da3 ("bpf: Fix ORC unwinding in non-JIT BPF code")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Jann Horn <jannh@google.com>
Co-developed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 53 +++++++++++++++++++++++--------------------
tools/objtool/check.h | 1 +
tools/objtool/elf.h | 1 +
3 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a904f98236b4..a64bb54abd29 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -900,7 +900,7 @@ static int add_special_section_alts(struct objtool_file *file)
}
static int add_jump_table(struct objtool_file *file, struct instruction *insn,
- struct rela *table, struct rela *next_table)
+ struct rela *table)
{
struct rela *rela = table;
struct instruction *dest_insn;
@@ -913,7 +913,9 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
* instruction.
*/
list_for_each_entry_from(rela, &table->sec->rela_list, list) {
- if (rela == next_table)
+
+ /* Check for the end of the table: */
+ if (rela != table && rela->jump_table_start)
break;
/* Make sure the table entries are consecutive: */
@@ -1072,13 +1074,15 @@ static struct rela *find_jump_table(struct objtool_file *file,
return NULL;
}
-
-static int add_func_jump_tables(struct objtool_file *file,
- struct symbol *func)
+/*
+ * First pass: Mark the head of each jump table so that in the next pass,
+ * we know when a given jump table ends and the next one starts.
+ */
+static void mark_func_jump_tables(struct objtool_file *file,
+ struct symbol *func)
{
- struct instruction *insn, *last = NULL, *prev_jump = NULL;
- struct rela *rela, *prev_rela = NULL;
- int ret;
+ struct instruction *insn, *last = NULL;
+ struct rela *rela;
func_for_each_insn_all(file, func, insn) {
if (!last)
@@ -1102,26 +1106,24 @@ static int add_func_jump_tables(struct objtool_file *file,
continue;
rela = find_jump_table(file, func, insn);
- if (!rela)
- continue;
-
- /*
- * We found a jump table, but we don't know yet how big it
- * is. Don't add it until we reach the end of the function or
- * the beginning of another jump table in the same function.
- */
- if (prev_jump) {
- ret = add_jump_table(file, prev_jump, prev_rela, rela);
- if (ret)
- return ret;
+ if (rela) {
+ rela->jump_table_start = true;
+ insn->jump_table = rela;
}
-
- prev_jump = insn;
- prev_rela = rela;
}
+}
+
+static int add_func_jump_tables(struct objtool_file *file,
+ struct symbol *func)
+{
+ struct instruction *insn;
+ int ret;
+
+ func_for_each_insn_all(file, func, insn) {
+ if (!insn->jump_table)
+ continue;
- if (prev_jump) {
- ret = add_jump_table(file, prev_jump, prev_rela, NULL);
+ ret = add_jump_table(file, insn, insn->jump_table);
if (ret)
return ret;
}
@@ -1148,6 +1150,7 @@ static int add_jump_table_alts(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
+ mark_func_jump_tables(file, func);
ret = add_func_jump_tables(file, func);
if (ret)
return ret;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index cb60b9acf5cf..afa6a79e0715 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -38,6 +38,7 @@ struct instruction {
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
+ struct rela *jump_table;
struct list_head alts;
struct symbol *func;
struct stack_op stack_op;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 1b638de4e7c0..14e7d4c3aff1 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@ struct rela {
unsigned int type;
unsigned long offset;
int addend;
+ bool jump_table_start;
};
struct elf {
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 20/22] objtool: Fix seg fault on bad switch table entry
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (18 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:21 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 21/22] objtool: convert insn type to enum Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 22/22] objtool: Support conditional retpolines Josh Poimboeuf
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
In one rare case, Clang generated the following code:
5ca: 83 e0 21 and $0x21,%eax
5cd: b9 04 00 00 00 mov $0x4,%ecx
5d2: ff 24 c5 00 00 00 00 jmpq *0x0(,%rax,8)
5d5: R_X86_64_32S .rodata+0x38
which uses the corresponding jump table relocations:
000000000038 000200000001 R_X86_64_64 0000000000000000 .text + 834
000000000040 000200000001 R_X86_64_64 0000000000000000 .text + 5d9
000000000048 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000050 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000058 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000060 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000068 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000070 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000078 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000080 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000088 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000090 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000098 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000a0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000a8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000b0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000b8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000c0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000c8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000d0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000d8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000e0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000e8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000f0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000f8 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000100 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000108 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000110 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000118 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000120 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000128 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000130 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000138 000200000001 R_X86_64_64 0000000000000000 .text + 82f
000000000140 000200000001 R_X86_64_64 0000000000000000 .text + 828
Since %eax was masked with 0x21, only the first two and the last two
entries are possible.
Objtool doesn't actually emulate all the code, so it isn't smart enough
to know that all the middle entries aren't reachable. They point to the
NOP padding area after the end of the function, so objtool seg faulted
when it tried to dereference a NULL insn->func.
After this fix, objtool still gives an "unreachable" error because it
stops reading the jump table when it encounters the bad addresses:
/home/jpoimboe/objtool-tests/adm1275.o: warning: objtool: adm1275_probe()+0x828: unreachable instruction
While the above code is technically correct, it's very wasteful of
memory -- it uses 34 jump table entries when only 4 are needed. It's
also not possible for objtool to validate this type of switch table
because the unused entries point outside the function and objtool has no
way of determining if that's intentional. Hopefully the Clang folks can
fix it.
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/check.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a64bb54abd29..082ede40c6c0 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -932,7 +932,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
break;
/* Make sure the destination is in the same function: */
- if (dest_insn->func->pfunc != pfunc)
+ if (!dest_insn->func || dest_insn->func->pfunc != pfunc)
break;
alt = malloc(sizeof(*alt));
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 21/22] objtool: convert insn type to enum
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (19 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:22 ` [tip:core/urgent] objtool: Convert " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 22/22] objtool: Support conditional retpolines Josh Poimboeuf
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
This makes it easier to add new instruction types. Also it's hopefully
more robust since the compiler should warn about out-of-range enums.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/arch.h | 35 +++++++++++++++++----------------
tools/objtool/arch/x86/decode.c | 2 +-
tools/objtool/check.c | 7 -------
tools/objtool/check.h | 2 +-
4 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 580e344db3dd..50448c0c4bca 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -11,22 +11,23 @@
#include "elf.h"
#include "cfi.h"
-#define INSN_JUMP_CONDITIONAL 1
-#define INSN_JUMP_UNCONDITIONAL 2
-#define INSN_JUMP_DYNAMIC 3
-#define INSN_CALL 4
-#define INSN_CALL_DYNAMIC 5
-#define INSN_RETURN 6
-#define INSN_CONTEXT_SWITCH 7
-#define INSN_STACK 8
-#define INSN_BUG 9
-#define INSN_NOP 10
-#define INSN_STAC 11
-#define INSN_CLAC 12
-#define INSN_STD 13
-#define INSN_CLD 14
-#define INSN_OTHER 15
-#define INSN_LAST INSN_OTHER
+enum insn_type {
+ INSN_JUMP_CONDITIONAL,
+ INSN_JUMP_UNCONDITIONAL,
+ INSN_JUMP_DYNAMIC,
+ INSN_CALL,
+ INSN_CALL_DYNAMIC,
+ INSN_RETURN,
+ INSN_CONTEXT_SWITCH,
+ INSN_STACK,
+ INSN_BUG,
+ INSN_NOP,
+ INSN_STAC,
+ INSN_CLAC,
+ INSN_STD,
+ INSN_CLD,
+ INSN_OTHER,
+};
enum op_dest_type {
OP_DEST_REG,
@@ -68,7 +69,7 @@ void arch_initial_func_cfi_state(struct cfi_state *state);
int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int maxlen,
- unsigned int *len, unsigned char *type,
+ unsigned int *len, enum insn_type *type,
unsigned long *immediate, struct stack_op *op);
bool arch_callee_saved_reg(unsigned char reg);
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 584568f27a83..0567c47a91b1 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -68,7 +68,7 @@ bool arch_callee_saved_reg(unsigned char reg)
int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int maxlen,
- unsigned int *len, unsigned char *type,
+ unsigned int *len, enum insn_type *type,
unsigned long *immediate, struct stack_op *op)
{
struct insn insn;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 082ede40c6c0..381e36dc587c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,13 +267,6 @@ static int decode_instructions(struct objtool_file *file)
if (ret)
goto err;
- if (!insn->type || insn->type > INSN_LAST) {
- WARN_FUNC("invalid instruction type %d",
- insn->sec, insn->offset, insn->type);
- ret = -1;
- goto err;
- }
-
hash_add(file->insn_hash, &insn->hash, insn->offset);
list_add_tail(&insn->list, &file->insn_list);
}
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index afa6a79e0715..b881fafcf55d 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct instruction {
struct section *sec;
unsigned long offset;
unsigned int len;
- unsigned char type;
+ enum insn_type type;
unsigned long immediate;
bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
bool retpoline_safe;
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 22/22] objtool: Support conditional retpolines
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (20 preceding siblings ...)
2019-07-18 1:36 ` [PATCH v2 21/22] objtool: convert insn type to enum Josh Poimboeuf
@ 2019-07-18 1:36 ` Josh Poimboeuf
2019-07-18 19:23 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
21 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 1:36 UTC (permalink / raw
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
A Clang-built kernel is showing the following warning:
arch/x86/kernel/platform-quirks.o: warning: objtool: x86_early_init_platform_quirks()+0x84: unreachable instruction
That corresponds to this code:
7e: 0f 85 00 00 00 00 jne 84 <x86_early_init_platform_quirks+0x84>
80: R_X86_64_PC32 __x86_indirect_thunk_r11-0x4
84: c3 retq
This is a conditional retpoline sibling call, which is now possible
thanks to retpolines. Objtool hasn't seen that before. It's
incorrectly interpreting the conditional jump as an unconditional
dynamic jump.
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
tools/objtool/arch.h | 1 +
tools/objtool/check.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 50448c0c4bca..ced3765c4f44 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -15,6 +15,7 @@ enum insn_type {
INSN_JUMP_CONDITIONAL,
INSN_JUMP_UNCONDITIONAL,
INSN_JUMP_DYNAMIC,
+ INSN_JUMP_DYNAMIC_CONDITIONAL,
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 381e36dc587c..a93ecce40641 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -575,7 +575,11 @@ static int add_jump_destinations(struct objtool_file *file)
* Retpoline jumps are really dynamic jumps in
* disguise, so convert them accordingly.
*/
- insn->type = INSN_JUMP_DYNAMIC;
+ if (insn->type == INSN_JUMP_UNCONDITIONAL)
+ insn->type = INSN_JUMP_DYNAMIC;
+ else
+ insn->type = INSN_JUMP_DYNAMIC_CONDITIONAL;
+
insn->retpoline_safe = true;
continue;
} else {
@@ -2114,13 +2118,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
}
- return 0;
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return 0;
+
+ break;
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
--
2.20.1
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2
2019-07-18 1:36 ` [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2 Josh Poimboeuf
@ 2019-07-18 8:17 ` Paolo Bonzini
2019-07-18 19:08 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
1 sibling, 0 replies; 60+ messages in thread
From: Paolo Bonzini @ 2019-07-18 8:17 UTC (permalink / raw
To: Josh Poimboeuf, x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 18/07/19 03:36, Josh Poimboeuf wrote:
> Objtool reports the following:
>
> arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
>
> But frame pointers are necessarily broken anyway, because
> __vmx_vcpu_run() clobbers RBP with the guest's value before calling
> vmx_vmenter(). So calling without a frame pointer doesn't make things
> any worse.
>
> Make objtool happy by changing the call to a UD2.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> v2: ud2 instead of kvm_spurious_fault() [Paolo]
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> ---
> arch/x86/kvm/vmx/vmenter.S | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index d4cb1945b2e3..4010d519eb8c 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -54,9 +54,9 @@ ENTRY(vmx_vmenter)
> ret
>
> 3: cmpb $0, kvm_rebooting
> - jne 4f
> - call kvm_spurious_fault
> -4: ret
> + je 4f
> + ret
> +4: ud2
>
> .pushsection .fixup, "ax"
> 5: jmp 3b
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 1:36 ` [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
@ 2019-07-18 8:22 ` Paolo Bonzini
2019-07-18 13:16 ` Sean Christopherson
2019-07-18 19:09 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
1 sibling, 1 reply; 60+ messages in thread
From: Paolo Bonzini @ 2019-07-18 8:22 UTC (permalink / raw
To: Josh Poimboeuf, x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 18/07/19 03:36, Josh Poimboeuf wrote:
> After making a change to improve objtool's sibling call detection, it
> started showing the following warning:
>
> arch/x86/kvm/vmx/nested.o: warning: objtool: .fixup+0x15: sibling call from callable instruction with modified stack frame
>
> The problem is the ____kvm_handle_fault_on_reboot() macro. It does a
> fake call by pushing a fake RIP and doing a jump. That tricks the
> unwinder into printing the function which triggered the exception,
> rather than the .fixup code.
>
> Instead of the hack to make it look like the original function made the
> call, just change the macro so that the original function actually does
> make the call. This allows removal of the hack, and also makes objtool
> happy.
>
> I triggered a vmx instruction exception and verified that the stack
> trace is still sane:
>
> kernel BUG at arch/x86/kvm/x86.c:358!
> invalid opcode: 0000 [#1] SMP PTI
> CPU: 28 PID: 4096 Comm: qemu-kvm Not tainted 5.2.0+ #16
> Hardware name: Lenovo THINKSYSTEM SD530 -[7X2106Z000]-/-[7X2106Z000]-, BIOS -[TEE113Z-1.00]- 07/17/2017
> RIP: 0010:kvm_spurious_fault+0x5/0x10
> Code: 00 00 00 00 00 8b 44 24 10 89 d2 45 89 c9 48 89 44 24 10 8b 44 24 08 48 89 44 24 08 e9 d4 40 22 00 0f 1f 40 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 49 89 fd 41
> RSP: 0018:ffffbf91c683bd00 EFLAGS: 00010246
> RAX: 000061f040000000 RBX: ffff9e159c77bba0 RCX: ffff9e15a5c87000
> RDX: 0000000665c87000 RSI: ffff9e15a5c87000 RDI: ffff9e159c77bba0
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffff9e15a5c87000
> R10: 0000000000000000 R11: fffff8f2d99721c0 R12: ffff9e159c77bba0
> R13: ffffbf91c671d960 R14: ffff9e159c778000 R15: 0000000000000000
> FS: 00007fa341cbe700(0000) GS:ffff9e15b7400000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fdd38356804 CR3: 00000006759de003 CR4: 00000000007606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> loaded_vmcs_init+0x4f/0xe0
> alloc_loaded_vmcs+0x38/0xd0
> vmx_create_vcpu+0xf7/0x600
> kvm_vm_ioctl+0x5e9/0x980
> ? __switch_to_asm+0x40/0x70
> ? __switch_to_asm+0x34/0x70
> ? __switch_to_asm+0x40/0x70
> ? __switch_to_asm+0x34/0x70
> ? free_one_page+0x13f/0x4e0
> do_vfs_ioctl+0xa4/0x630
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x55/0x1c0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7fa349b1ee5b
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> v2: Fix ____kvm_handle_fault_on_reboot() comment [Paolo]
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> ---
> arch/x86/include/asm/kvm_host.h | 34 ++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 0cc5b611a113..8282b8d41209 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1496,25 +1496,29 @@ enum {
> #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
> #define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
>
> +asmlinkage void __noreturn kvm_spurious_fault(void);
> +
> /*
> * Hardware virtualization extension instructions may fault if a
> * reboot turns off virtualization while processes are running.
> - * Trap the fault and ignore the instruction if that happens.
> + * Usually after catching the fault we just panic; during reboot
> + * instead the instruction is ignored.
> */
> -asmlinkage void kvm_spurious_fault(void);
> -
> -#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
> - "666: " insn "\n\t" \
> - "668: \n\t" \
> - ".pushsection .fixup, \"ax\" \n" \
> - "667: \n\t" \
> - cleanup_insn "\n\t" \
> - "cmpb $0, kvm_rebooting \n\t" \
> - "jne 668b \n\t" \
> - __ASM_SIZE(push) " $666b \n\t" \
> - "jmp kvm_spurious_fault \n\t" \
> - ".popsection \n\t" \
> - _ASM_EXTABLE(666b, 667b)
> +#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
> + "666: \n\t" \
> + insn "\n\t" \
> + "jmp 668f \n\t" \
> + "667: \n\t" \
> + "call kvm_spurious_fault \n\t" \
> + "668: \n\t" \
> + ".pushsection .fixup, \"ax\" \n\t" \
> + "700: \n\t" \
> + cleanup_insn "\n\t" \
> + "cmpb $0, kvm_rebooting\n\t" \
> + "je 667b \n\t" \
> + "jmp 668b \n\t" \
> + ".popsection \n\t" \
> + _ASM_EXTABLE(666b, 700b)
>
> #define __kvm_handle_fault_on_reboot(insn) \
> ____kvm_handle_fault_on_reboot(insn, "")
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
This has a side effect of adding a jump in a generally hot path, but
let's hope that the speculation gods for once help us.
Paolo
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 8:22 ` Paolo Bonzini
@ 2019-07-18 13:16 ` Sean Christopherson
2019-07-18 13:18 ` Paolo Bonzini
2019-07-18 14:03 ` Josh Poimboeuf
0 siblings, 2 replies; 60+ messages in thread
From: Sean Christopherson @ 2019-07-18 13:16 UTC (permalink / raw
To: Paolo Bonzini
Cc: Josh Poimboeuf, x86, linux-kernel, Peter Zijlstra,
Thomas Gleixner, Nick Desaulniers, Arnd Bergmann, Jann Horn,
Randy Dunlap, Radim Krčmář
On Thu, Jul 18, 2019 at 10:22:50AM +0200, Paolo Bonzini wrote:
> On 18/07/19 03:36, Josh Poimboeuf wrote:
> > After making a change to improve objtool's sibling call detection, it
> > started showing the following warning:
> >
> > arch/x86/kvm/vmx/nested.o: warning: objtool: .fixup+0x15: sibling call from callable instruction with modified stack frame
> >
> > The problem is the ____kvm_handle_fault_on_reboot() macro. It does a
> > fake call by pushing a fake RIP and doing a jump. That tricks the
> > unwinder into printing the function which triggered the exception,
> > rather than the .fixup code.
> >
> > Instead of the hack to make it look like the original function made the
> > call, just change the macro so that the original function actually does
> > make the call. This allows removal of the hack, and also makes objtool
> > happy.
> >
> > I triggered a vmx instruction exception and verified that the stack
> > trace is still sane:
> >
> > kernel BUG at arch/x86/kvm/x86.c:358!
> > invalid opcode: 0000 [#1] SMP PTI
> > CPU: 28 PID: 4096 Comm: qemu-kvm Not tainted 5.2.0+ #16
> > Hardware name: Lenovo THINKSYSTEM SD530 -[7X2106Z000]-/-[7X2106Z000]-, BIOS -[TEE113Z-1.00]- 07/17/2017
> > RIP: 0010:kvm_spurious_fault+0x5/0x10
> > Code: 00 00 00 00 00 8b 44 24 10 89 d2 45 89 c9 48 89 44 24 10 8b 44 24 08 48 89 44 24 08 e9 d4 40 22 00 0f 1f 40 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 49 89 fd 41
> > RSP: 0018:ffffbf91c683bd00 EFLAGS: 00010246
> > RAX: 000061f040000000 RBX: ffff9e159c77bba0 RCX: ffff9e15a5c87000
> > RDX: 0000000665c87000 RSI: ffff9e15a5c87000 RDI: ffff9e159c77bba0
> > RBP: 0000000000000000 R08: 0000000000000000 R09: ffff9e15a5c87000
> > R10: 0000000000000000 R11: fffff8f2d99721c0 R12: ffff9e159c77bba0
> > R13: ffffbf91c671d960 R14: ffff9e159c778000 R15: 0000000000000000
> > FS: 00007fa341cbe700(0000) GS:ffff9e15b7400000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007fdd38356804 CR3: 00000006759de003 CR4: 00000000007606e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > PKRU: 55555554
> > Call Trace:
> > loaded_vmcs_init+0x4f/0xe0
> > alloc_loaded_vmcs+0x38/0xd0
> > vmx_create_vcpu+0xf7/0x600
> > kvm_vm_ioctl+0x5e9/0x980
> > ? __switch_to_asm+0x40/0x70
> > ? __switch_to_asm+0x34/0x70
> > ? __switch_to_asm+0x40/0x70
> > ? __switch_to_asm+0x34/0x70
> > ? free_one_page+0x13f/0x4e0
> > do_vfs_ioctl+0xa4/0x630
> > ksys_ioctl+0x60/0x90
> > __x64_sys_ioctl+0x16/0x20
> > do_syscall_64+0x55/0x1c0
> > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > RIP: 0033:0x7fa349b1ee5b
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > v2: Fix ____kvm_handle_fault_on_reboot() comment [Paolo]
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > arch/x86/include/asm/kvm_host.h | 34 ++++++++++++++++++---------------
> > 1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 0cc5b611a113..8282b8d41209 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1496,25 +1496,29 @@ enum {
> > #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
> > #define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
> >
> > +asmlinkage void __noreturn kvm_spurious_fault(void);
With __noreturn added, can the entry in __dead_end_function() in
tools/objtool/check.c be removed?
> > +
> > /*
> > * Hardware virtualization extension instructions may fault if a
> > * reboot turns off virtualization while processes are running.
> > - * Trap the fault and ignore the instruction if that happens.
> > + * Usually after catching the fault we just panic; during reboot
> > + * instead the instruction is ignored.
> > */
> > -asmlinkage void kvm_spurious_fault(void);
> > -
> > -#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
> > - "666: " insn "\n\t" \
> > - "668: \n\t" \
> > - ".pushsection .fixup, \"ax\" \n" \
> > - "667: \n\t" \
> > - cleanup_insn "\n\t" \
> > - "cmpb $0, kvm_rebooting \n\t" \
> > - "jne 668b \n\t" \
> > - __ASM_SIZE(push) " $666b \n\t" \
> > - "jmp kvm_spurious_fault \n\t" \
> > - ".popsection \n\t" \
> > - _ASM_EXTABLE(666b, 667b)
> > +#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
> > + "666: \n\t" \
> > + insn "\n\t" \
> > + "jmp 668f \n\t" \
> > + "667: \n\t" \
> > + "call kvm_spurious_fault \n\t" \
> > + "668: \n\t" \
> > + ".pushsection .fixup, \"ax\" \n\t" \
> > + "700: \n\t" \
> > + cleanup_insn "\n\t" \
> > + "cmpb $0, kvm_rebooting\n\t" \
> > + "je 667b \n\t" \
> > + "jmp 668b \n\t" \
> > + ".popsection \n\t" \
> > + _ASM_EXTABLE(666b, 700b)
> >
> > #define __kvm_handle_fault_on_reboot(insn) \
> > ____kvm_handle_fault_on_reboot(insn, "")
> >
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> This has a side effect of adding a jump in a generally hot path, but
> let's hope that the speculation gods for once help us.
Any reason not to take the same approach as vmx_vmenter() and ud2 directly
from fixup? I've never found kvm_spurious_fault() to be all that helpful,
IMO it's a win win. :-)
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 13:16 ` Sean Christopherson
@ 2019-07-18 13:18 ` Paolo Bonzini
2019-07-18 14:12 ` Josh Poimboeuf
2019-07-18 14:03 ` Josh Poimboeuf
1 sibling, 1 reply; 60+ messages in thread
From: Paolo Bonzini @ 2019-07-18 13:18 UTC (permalink / raw
To: Sean Christopherson
Cc: Josh Poimboeuf, x86, linux-kernel, Peter Zijlstra,
Thomas Gleixner, Nick Desaulniers, Arnd Bergmann, Jann Horn,
Randy Dunlap, Radim Krčmář
On 18/07/19 15:16, Sean Christopherson wrote:
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> This has a side effect of adding a jump in a generally hot path, but
>> let's hope that the speculation gods for once help us.
> Any reason not to take the same approach as vmx_vmenter() and ud2 directly
> from fixup? I've never found kvm_spurious_fault() to be all that helpful,
> IMO it's a win win. :-)
Honestly I've never seen a backtrace from here but I would rather not
regret this when a customer encounters it...
Paolo
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 13:16 ` Sean Christopherson
2019-07-18 13:18 ` Paolo Bonzini
@ 2019-07-18 14:03 ` Josh Poimboeuf
1 sibling, 0 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 14:03 UTC (permalink / raw
To: Sean Christopherson
Cc: Paolo Bonzini, x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On Thu, Jul 18, 2019 at 06:16:54AM -0700, Sean Christopherson wrote:
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index 0cc5b611a113..8282b8d41209 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1496,25 +1496,29 @@ enum {
> > > #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
> > > #define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
> > >
> > > +asmlinkage void __noreturn kvm_spurious_fault(void);
>
> With __noreturn added, can the entry in __dead_end_function() in
> tools/objtool/check.c be removed?
No, that's actually still needed because objtool can't see the
__noreturn annotation. So it still needs to know that the "call
kvm_spurious_fault" doesn't return.
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 13:18 ` Paolo Bonzini
@ 2019-07-18 14:12 ` Josh Poimboeuf
2019-07-18 14:13 ` Paolo Bonzini
0 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2019-07-18 14:12 UTC (permalink / raw
To: Paolo Bonzini
Cc: Sean Christopherson, x86, linux-kernel, Peter Zijlstra,
Thomas Gleixner, Nick Desaulniers, Arnd Bergmann, Jann Horn,
Randy Dunlap, Radim Krčmář
On Thu, Jul 18, 2019 at 03:18:50PM +0200, Paolo Bonzini wrote:
> On 18/07/19 15:16, Sean Christopherson wrote:
> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >>
> >> This has a side effect of adding a jump in a generally hot path, but
> >> let's hope that the speculation gods for once help us.
> > Any reason not to take the same approach as vmx_vmenter() and ud2 directly
> > from fixup? I've never found kvm_spurious_fault() to be all that helpful,
> > IMO it's a win win. :-)
>
> Honestly I've never seen a backtrace from here but I would rather not
> regret this when a customer encounters it...
In theory, changing the "call kvm_spurious_fault" to ud2 should be fine.
It should be tested, of course.
I would defer to Sean to make the patch on top of mine :-)
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 14:12 ` Josh Poimboeuf
@ 2019-07-18 14:13 ` Paolo Bonzini
0 siblings, 0 replies; 60+ messages in thread
From: Paolo Bonzini @ 2019-07-18 14:13 UTC (permalink / raw
To: Josh Poimboeuf
Cc: Sean Christopherson, x86, linux-kernel, Peter Zijlstra,
Thomas Gleixner, Nick Desaulniers, Arnd Bergmann, Jann Horn,
Randy Dunlap, Radim Krčmář
On 18/07/19 16:12, Josh Poimboeuf wrote:
> On Thu, Jul 18, 2019 at 03:18:50PM +0200, Paolo Bonzini wrote:
>> On 18/07/19 15:16, Sean Christopherson wrote:
>>>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>
>>>> This has a side effect of adding a jump in a generally hot path, but
>>>> let's hope that the speculation gods for once help us.
>>> Any reason not to take the same approach as vmx_vmenter() and ud2 directly
>>> from fixup? I've never found kvm_spurious_fault() to be all that helpful,
>>> IMO it's a win win. :-)
>>
>> Honestly I've never seen a backtrace from here but I would rather not
>> regret this when a customer encounters it...
>
> In theory, changing the "call kvm_spurious_fault" to ud2 should be fine.
> It should be tested, of course.
>
> I would defer to Sean to make the patch on top of mine :-)
>
Yes, this can be done easily on top.
Paolo
^ permalink raw reply [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/paravirt: Fix callee-saved function ELF sizes
2019-07-18 1:36 ` [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
@ 2019-07-18 19:07 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:07 UTC (permalink / raw
To: linux-tip-commits
Cc: linux-kernel, mingo, jgross, jpoimboe, tglx, hpa, peterz
Commit-ID: 083db6764821996526970e42d09c1ab2f4155dd4
Gitweb: https://git.kernel.org/tip/083db6764821996526970e42d09c1ab2f4155dd4
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:36 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:03 +0200
x86/paravirt: Fix callee-saved function ELF sizes
The __raw_callee_save_*() functions have an ELF symbol size of zero,
which confuses objtool and other tools.
Fixes a bunch of warnings like the following:
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_pte_val() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_pgd_val() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_make_pte() is missing an ELF size annotation
arch/x86/xen/mmu_pv.o: warning: objtool: __raw_callee_save_xen_make_pgd() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Juergen Gross <jgross@suse.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/afa6d49bb07497ca62e4fc3b27a2d0cece545b4e.1563413318.git.jpoimboe@redhat.com
---
arch/x86/include/asm/paravirt.h | 1 +
arch/x86/kernel/kvm.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index c25c38a05c1c..d6f5ae2c79ab 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -746,6 +746,7 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
PV_RESTORE_ALL_CALLER_REGS \
FRAME_END \
"ret;" \
+ ".size " PV_THUNK_NAME(func) ", .-" PV_THUNK_NAME(func) ";" \
".popsection")
/* Get a reference to a callee-save function */
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 82caf01b63dd..6661bd2f08a6 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -838,6 +838,7 @@ asm(
"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
"setne %al;"
"ret;"
+".size __raw_callee_save___kvm_vcpu_is_preempted, .-__raw_callee_save___kvm_vcpu_is_preempted;"
".popsection");
#endif
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/kvm: Fix fastop function ELF metadata
2019-07-18 1:36 ` [PATCH v2 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
@ 2019-07-18 19:08 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:08 UTC (permalink / raw
To: linux-tip-commits
Cc: mingo, linux-kernel, jpoimboe, tglx, peterz, pbonzini, hpa
Commit-ID: d99a6ce70ec6ed990b74bd4e34232fd830d20d27
Gitweb: https://git.kernel.org/tip/d99a6ce70ec6ed990b74bd4e34232fd830d20d27
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:37 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:03 +0200
x86/kvm: Fix fastop function ELF metadata
Some of the fastop functions, e.g. em_setcc(), are actually just used as
global labels which point to blocks of functions. The global labels are
incorrectly annotated as functions. Also the functions themselves don't
have size annotations.
Fixes a bunch of warnings like the following:
arch/x86/kvm/emulate.o: warning: objtool: seto() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: em_setcc() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: setno() is missing an ELF size annotation
arch/x86/kvm/emulate.o: warning: objtool: setc() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/c8cc9be60ebbceb3092aa5dd91916039a1f88275.1563413318.git.jpoimboe@redhat.com
---
arch/x86/kvm/emulate.c | 44 +++++++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8e409ad448f9..718f7d9afedc 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -312,29 +312,42 @@ static void invalidate_registers(struct x86_emulate_ctxt *ctxt)
static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
-#define FOP_FUNC(name) \
+#define __FOP_FUNC(name) \
".align " __stringify(FASTOP_SIZE) " \n\t" \
".type " name ", @function \n\t" \
name ":\n\t"
-#define FOP_RET "ret \n\t"
+#define FOP_FUNC(name) \
+ __FOP_FUNC(#name)
+
+#define __FOP_RET(name) \
+ "ret \n\t" \
+ ".size " name ", .-" name "\n\t"
+
+#define FOP_RET(name) \
+ __FOP_RET(#name)
#define FOP_START(op) \
extern void em_##op(struct fastop *fake); \
asm(".pushsection .text, \"ax\" \n\t" \
".global em_" #op " \n\t" \
- FOP_FUNC("em_" #op)
+ ".align " __stringify(FASTOP_SIZE) " \n\t" \
+ "em_" #op ":\n\t"
#define FOP_END \
".popsection")
+#define __FOPNOP(name) \
+ __FOP_FUNC(name) \
+ __FOP_RET(name)
+
#define FOPNOP() \
- FOP_FUNC(__stringify(__UNIQUE_ID(nop))) \
- FOP_RET
+ __FOPNOP(__stringify(__UNIQUE_ID(nop)))
#define FOP1E(op, dst) \
- FOP_FUNC(#op "_" #dst) \
- "10: " #op " %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst) \
+ "10: " #op " %" #dst " \n\t" \
+ __FOP_RET(#op "_" #dst)
#define FOP1EEX(op, dst) \
FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
@@ -366,8 +379,9 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
FOP_END
#define FOP2E(op, dst, src) \
- FOP_FUNC(#op "_" #dst "_" #src) \
- #op " %" #src ", %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst "_" #src) \
+ #op " %" #src ", %" #dst " \n\t" \
+ __FOP_RET(#op "_" #dst "_" #src)
#define FASTOP2(op) \
FOP_START(op) \
@@ -405,8 +419,9 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
FOP_END
#define FOP3E(op, dst, src, src2) \
- FOP_FUNC(#op "_" #dst "_" #src "_" #src2) \
- #op " %" #src2 ", %" #src ", %" #dst " \n\t" FOP_RET
+ __FOP_FUNC(#op "_" #dst "_" #src "_" #src2) \
+ #op " %" #src2 ", %" #src ", %" #dst " \n\t"\
+ __FOP_RET(#op "_" #dst "_" #src "_" #src2)
/* 3-operand, word-only, src2=cl */
#define FASTOP3WCL(op) \
@@ -423,7 +438,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *));
".type " #op ", @function \n\t" \
#op ": \n\t" \
#op " %al \n\t" \
- FOP_RET
+ __FOP_RET(#op)
asm(".pushsection .fixup, \"ax\"\n"
".global kvm_fastop_exception \n"
@@ -449,7 +464,10 @@ FOP_SETCC(setle)
FOP_SETCC(setnle)
FOP_END;
-FOP_START(salc) "pushf; sbb %al, %al; popf \n\t" FOP_RET
+FOP_START(salc)
+FOP_FUNC(salc)
+"pushf; sbb %al, %al; popf \n\t"
+FOP_RET(salc)
FOP_END;
/*
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2
2019-07-18 1:36 ` [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2 Josh Poimboeuf
2019-07-18 8:17 ` Paolo Bonzini
@ 2019-07-18 19:08 ` tip-bot for Josh Poimboeuf
1 sibling, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:08 UTC (permalink / raw
To: linux-tip-commits
Cc: hpa, tglx, pbonzini, mingo, linux-kernel, jpoimboe, peterz
Commit-ID: 19f2d8fa98644c7b78845b1d66abeae4e3d9dfa8
Gitweb: https://git.kernel.org/tip/19f2d8fa98644c7b78845b1d66abeae4e3d9dfa8
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:38 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:03 +0200
x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2
Objtool reports the following:
arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
But frame pointers are necessarily broken anyway, because
__vmx_vcpu_run() clobbers RBP with the guest's value before calling
vmx_vmenter(). So calling without a frame pointer doesn't make things
any worse.
Make objtool happy by changing the call to a UD2.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lkml.kernel.org/r/9fc2216c9dc972f95bb65ce2966a682c6bda1cb0.1563413318.git.jpoimboe@redhat.com
---
arch/x86/kvm/vmx/vmenter.S | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index d4cb1945b2e3..4010d519eb8c 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -54,9 +54,9 @@ ENTRY(vmx_vmenter)
ret
3: cmpb $0, kvm_rebooting
- jne 4f
- call kvm_spurious_fault
-4: ret
+ je 4f
+ ret
+4: ud2
.pushsection .fixup, "ax"
5: jmp 3b
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-18 1:36 ` [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
2019-07-18 8:22 ` Paolo Bonzini
@ 2019-07-18 19:09 ` tip-bot for Josh Poimboeuf
1 sibling, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:09 UTC (permalink / raw
To: linux-tip-commits
Cc: peterz, mingo, tglx, pbonzini, linux-kernel, jpoimboe, hpa
Commit-ID: 3901336ed9887b075531bffaeef7742ba614058b
Gitweb: https://git.kernel.org/tip/3901336ed9887b075531bffaeef7742ba614058b
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:39 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:04 +0200
x86/kvm: Don't call kvm_spurious_fault() from .fixup
After making a change to improve objtool's sibling call detection, it
started showing the following warning:
arch/x86/kvm/vmx/nested.o: warning: objtool: .fixup+0x15: sibling call from callable instruction with modified stack frame
The problem is the ____kvm_handle_fault_on_reboot() macro. It does a
fake call by pushing a fake RIP and doing a jump. That tricks the
unwinder into printing the function which triggered the exception,
rather than the .fixup code.
Instead of the hack to make it look like the original function made the
call, just change the macro so that the original function actually does
make the call. This allows removal of the hack, and also makes objtool
happy.
I triggered a vmx instruction exception and verified that the stack
trace is still sane:
kernel BUG at arch/x86/kvm/x86.c:358!
invalid opcode: 0000 [#1] SMP PTI
CPU: 28 PID: 4096 Comm: qemu-kvm Not tainted 5.2.0+ #16
Hardware name: Lenovo THINKSYSTEM SD530 -[7X2106Z000]-/-[7X2106Z000]-, BIOS -[TEE113Z-1.00]- 07/17/2017
RIP: 0010:kvm_spurious_fault+0x5/0x10
Code: 00 00 00 00 00 8b 44 24 10 89 d2 45 89 c9 48 89 44 24 10 8b 44 24 08 48 89 44 24 08 e9 d4 40 22 00 0f 1f 40 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 49 89 fd 41
RSP: 0018:ffffbf91c683bd00 EFLAGS: 00010246
RAX: 000061f040000000 RBX: ffff9e159c77bba0 RCX: ffff9e15a5c87000
RDX: 0000000665c87000 RSI: ffff9e15a5c87000 RDI: ffff9e159c77bba0
RBP: 0000000000000000 R08: 0000000000000000 R09: ffff9e15a5c87000
R10: 0000000000000000 R11: fffff8f2d99721c0 R12: ffff9e159c77bba0
R13: ffffbf91c671d960 R14: ffff9e159c778000 R15: 0000000000000000
FS: 00007fa341cbe700(0000) GS:ffff9e15b7400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fdd38356804 CR3: 00000006759de003 CR4: 00000000007606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
loaded_vmcs_init+0x4f/0xe0
alloc_loaded_vmcs+0x38/0xd0
vmx_create_vcpu+0xf7/0x600
kvm_vm_ioctl+0x5e9/0x980
? __switch_to_asm+0x40/0x70
? __switch_to_asm+0x34/0x70
? __switch_to_asm+0x40/0x70
? __switch_to_asm+0x34/0x70
? free_one_page+0x13f/0x4e0
do_vfs_ioctl+0xa4/0x630
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x55/0x1c0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fa349b1ee5b
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/64a9b64d127e87b6920a97afde8e96ea76f6524e.1563413318.git.jpoimboe@redhat.com
---
arch/x86/include/asm/kvm_host.h | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0cc5b611a113..8282b8d41209 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1496,25 +1496,29 @@ enum {
#define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
#define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
+asmlinkage void __noreturn kvm_spurious_fault(void);
+
/*
* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
- * Trap the fault and ignore the instruction if that happens.
+ * Usually after catching the fault we just panic; during reboot
+ * instead the instruction is ignored.
*/
-asmlinkage void kvm_spurious_fault(void);
-
-#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
- "666: " insn "\n\t" \
- "668: \n\t" \
- ".pushsection .fixup, \"ax\" \n" \
- "667: \n\t" \
- cleanup_insn "\n\t" \
- "cmpb $0, kvm_rebooting \n\t" \
- "jne 668b \n\t" \
- __ASM_SIZE(push) " $666b \n\t" \
- "jmp kvm_spurious_fault \n\t" \
- ".popsection \n\t" \
- _ASM_EXTABLE(666b, 667b)
+#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
+ "666: \n\t" \
+ insn "\n\t" \
+ "jmp 668f \n\t" \
+ "667: \n\t" \
+ "call kvm_spurious_fault \n\t" \
+ "668: \n\t" \
+ ".pushsection .fixup, \"ax\" \n\t" \
+ "700: \n\t" \
+ cleanup_insn "\n\t" \
+ "cmpb $0, kvm_rebooting\n\t" \
+ "je 667b \n\t" \
+ "jmp 668b \n\t" \
+ ".popsection \n\t" \
+ _ASM_EXTABLE(666b, 700b)
#define __kvm_handle_fault_on_reboot(insn) \
____kvm_handle_fault_on_reboot(insn, "")
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/entry: Fix thunk function ELF sizes
2019-07-18 1:36 ` [PATCH v2 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
@ 2019-07-18 19:10 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:10 UTC (permalink / raw
To: linux-tip-commits; +Cc: hpa, jpoimboe, mingo, tglx, linux-kernel, peterz
Commit-ID: e6dd47394493061c605285a868fc72eae2e9c866
Gitweb: https://git.kernel.org/tip/e6dd47394493061c605285a868fc72eae2e9c866
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:40 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:04 +0200
x86/entry: Fix thunk function ELF sizes
Fix the following warnings:
arch/x86/entry/thunk_64.o: warning: objtool: trace_hardirqs_on_thunk() is missing an ELF size annotation
arch/x86/entry/thunk_64.o: warning: objtool: trace_hardirqs_off_thunk() is missing an ELF size annotation
arch/x86/entry/thunk_64.o: warning: objtool: lockdep_sys_exit_thunk() is missing an ELF size annotation
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/89c97adc9f6cc44a0f5d03cde6d0357662938909.1563413318.git.jpoimboe@redhat.com
---
arch/x86/entry/thunk_64.S | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index cfdca8b42c70..cc20465b2867 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -12,9 +12,7 @@
/* rdi: arg1 ... normal C conventions. rax is saved/restored. */
.macro THUNK name, func, put_ret_addr_in_rdi=0
- .globl \name
- .type \name, @function
-\name:
+ ENTRY(\name)
pushq %rbp
movq %rsp, %rbp
@@ -35,6 +33,7 @@
call \func
jmp .L_restore
+ ENDPROC(\name)
_ASM_NOKPROBE(\name)
.endm
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/head/64: Annotate start_cpu0() as non-callable
2019-07-18 1:36 ` [PATCH v2 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
@ 2019-07-18 19:11 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:11 UTC (permalink / raw
To: linux-tip-commits; +Cc: hpa, jpoimboe, tglx, mingo, peterz, linux-kernel
Commit-ID: 61a73f5cd1a5794626d216cc56e20a1b195c5d0c
Gitweb: https://git.kernel.org/tip/61a73f5cd1a5794626d216cc56e20a1b195c5d0c
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:41 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:04 +0200
x86/head/64: Annotate start_cpu0() as non-callable
After an objtool improvement, it complains about the fact that
start_cpu0() jumps to code which has an LRET instruction.
arch/x86/kernel/head_64.o: warning: objtool: .head.text+0xe4: unsupported instruction in callable function
Technically, start_cpu0() is callable, but it acts nothing like a
callable function. Prevent objtool from treating it like one by
removing its ELF function annotation.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/6b1b4505fcb90571a55fa1b52d71fb458ca24454.1563413318.git.jpoimboe@redhat.com
---
arch/x86/kernel/head_64.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index bcd206c8ac90..66b4a7757397 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -253,10 +253,10 @@ END(secondary_startup_64)
* start_secondary() via .Ljump_to_C_code.
*/
ENTRY(start_cpu0)
- movq initial_stack(%rip), %rsp
UNWIND_HINT_EMPTY
+ movq initial_stack(%rip), %rsp
jmp .Ljump_to_C_code
-ENDPROC(start_cpu0)
+END(start_cpu0)
#endif
/* Both SMP bootup and ACPI suspend change these variables */
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
2019-07-18 1:36 ` [PATCH v2 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
@ 2019-07-18 19:11 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:11 UTC (permalink / raw
To: linux-tip-commits; +Cc: peterz, jpoimboe, linux-kernel, tglx, mingo, hpa
Commit-ID: 3a6ab4bcc52263dd5b1d2fd2e4ce95a38c798b4d
Gitweb: https://git.kernel.org/tip/3a6ab4bcc52263dd5b1d2fd2e4ce95a38c798b4d
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:42 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:05 +0200
x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
After an objtool improvement, it's complaining about the CLAC in
copy_user_handle_tail():
arch/x86/lib/copy_user_64.o: warning: objtool: .altinstr_replacement+0x12: redundant UACCESS disable
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x6: (alt)
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x2: (alt)
arch/x86/lib/copy_user_64.o: warning: objtool: copy_user_handle_tail()+0x0: <=== (func)
copy_user_handle_tail() is incorrectly marked as a callable function, so
objtool is rightfully concerned about the CLAC with no corresponding
STAC.
Remove the ELF function annotation. The copy_user_handle_tail() code
path is already verified by objtool because it's jumped to by other
callable asm code (which does the corresponding STAC).
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/6b6e436774678b4b9873811ff023bd29935bee5b.1563413318.git.jpoimboe@redhat.com
---
arch/x86/lib/copy_user_64.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index 378a1f70ae7d..4fe1601dbc5d 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -239,7 +239,7 @@ copy_user_handle_tail:
ret
_ASM_EXTABLE_UA(1b, 2b)
-ENDPROC(copy_user_handle_tail)
+END(copy_user_handle_tail)
/*
* copy_user_nocache - Uncached memory copy with exception handling
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()
2019-07-18 1:36 ` [PATCH v2 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
@ 2019-07-18 19:12 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:12 UTC (permalink / raw
To: linux-tip-commits; +Cc: linux-kernel, mingo, peterz, jpoimboe, tglx, hpa
Commit-ID: 5e307a6bc7b600999742675dd182bcb8a6fe308e
Gitweb: https://git.kernel.org/tip/5e307a6bc7b600999742675dd182bcb8a6fe308e
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:43 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:05 +0200
x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()
After adding mcsafe_handle_tail() to the objtool uaccess safe list,
objtool reports:
arch/x86/lib/usercopy_64.o: warning: objtool: mcsafe_handle_tail()+0x0: call to __fentry__() with UACCESS enabled
With SMAP, this function is called with AC=1, so it needs to be careful
about which functions it calls. Disable the ftrace entry hook, which
can potentially pull in a lot of extra code.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/8e13d6f0da1c8a3f7603903da6cbf6d582bbfe10.1563413318.git.jpoimboe@redhat.com
---
arch/x86/lib/usercopy_64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index e0e006f1624e..fff28c6f73a2 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -60,7 +60,7 @@ EXPORT_SYMBOL(clear_user);
* but reuse __memcpy_mcsafe in case a new read error is encountered.
* clac() is handled in _copy_to_iter_mcsafe().
*/
-__visible unsigned long
+__visible notrace unsigned long
mcsafe_handle_tail(char *to, char *from, unsigned len)
{
for (; len; --len, to++, from++) {
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths
2019-07-18 1:36 ` [PATCH v2 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
@ 2019-07-18 19:13 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:13 UTC (permalink / raw
To: linux-tip-commits; +Cc: tglx, linux-kernel, hpa, mingo, peterz, jpoimboe
Commit-ID: 82e844a6536d1a3c12a73e44712f4021d90a4b53
Gitweb: https://git.kernel.org/tip/82e844a6536d1a3c12a73e44712f4021d90a4b53
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:44 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
x86/uaccess: Remove redundant CLACs in getuser/putuser error paths
The same getuser/putuser error paths are used regardless of whether AC
is set. In non-exception failure cases, this results in an unnecessary
CLAC.
Fixes the following warnings:
arch/x86/lib/getuser.o: warning: objtool: .altinstr_replacement+0x18: redundant UACCESS disable
arch/x86/lib/putuser.o: warning: objtool: .altinstr_replacement+0x18: redundant UACCESS disable
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/bc14ded2755ae75bd9010c446079e113dbddb74b.1563413318.git.jpoimboe@redhat.com
---
arch/x86/lib/getuser.S | 20 ++++++++++----------
arch/x86/lib/putuser.S | 29 ++++++++++++++++-------------
2 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index 74fdff968ea3..304f958c27b2 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -115,29 +115,29 @@ ENDPROC(__get_user_8)
EXPORT_SYMBOL(__get_user_8)
+bad_get_user_clac:
+ ASM_CLAC
bad_get_user:
xor %edx,%edx
mov $(-EFAULT),%_ASM_AX
- ASM_CLAC
ret
-END(bad_get_user)
#ifdef CONFIG_X86_32
+bad_get_user_8_clac:
+ ASM_CLAC
bad_get_user_8:
xor %edx,%edx
xor %ecx,%ecx
mov $(-EFAULT),%_ASM_AX
- ASM_CLAC
ret
-END(bad_get_user_8)
#endif
- _ASM_EXTABLE_UA(1b, bad_get_user)
- _ASM_EXTABLE_UA(2b, bad_get_user)
- _ASM_EXTABLE_UA(3b, bad_get_user)
+ _ASM_EXTABLE_UA(1b, bad_get_user_clac)
+ _ASM_EXTABLE_UA(2b, bad_get_user_clac)
+ _ASM_EXTABLE_UA(3b, bad_get_user_clac)
#ifdef CONFIG_X86_64
- _ASM_EXTABLE_UA(4b, bad_get_user)
+ _ASM_EXTABLE_UA(4b, bad_get_user_clac)
#else
- _ASM_EXTABLE_UA(4b, bad_get_user_8)
- _ASM_EXTABLE_UA(5b, bad_get_user_8)
+ _ASM_EXTABLE_UA(4b, bad_get_user_8_clac)
+ _ASM_EXTABLE_UA(5b, bad_get_user_8_clac)
#endif
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index d2e5c9c39601..14bf78341d3c 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -32,8 +32,6 @@
*/
#define ENTER mov PER_CPU_VAR(current_task), %_ASM_BX
-#define EXIT ASM_CLAC ; \
- ret
.text
ENTRY(__put_user_1)
@@ -43,7 +41,8 @@ ENTRY(__put_user_1)
ASM_STAC
1: movb %al,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_1)
EXPORT_SYMBOL(__put_user_1)
@@ -56,7 +55,8 @@ ENTRY(__put_user_2)
ASM_STAC
2: movw %ax,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_2)
EXPORT_SYMBOL(__put_user_2)
@@ -69,7 +69,8 @@ ENTRY(__put_user_4)
ASM_STAC
3: movl %eax,(%_ASM_CX)
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ ret
ENDPROC(__put_user_4)
EXPORT_SYMBOL(__put_user_4)
@@ -85,19 +86,21 @@ ENTRY(__put_user_8)
5: movl %edx,4(%_ASM_CX)
#endif
xor %eax,%eax
- EXIT
+ ASM_CLAC
+ RET
ENDPROC(__put_user_8)
EXPORT_SYMBOL(__put_user_8)
+bad_put_user_clac:
+ ASM_CLAC
bad_put_user:
movl $-EFAULT,%eax
- EXIT
-END(bad_put_user)
+ RET
- _ASM_EXTABLE_UA(1b, bad_put_user)
- _ASM_EXTABLE_UA(2b, bad_put_user)
- _ASM_EXTABLE_UA(3b, bad_put_user)
- _ASM_EXTABLE_UA(4b, bad_put_user)
+ _ASM_EXTABLE_UA(1b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(2b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(3b, bad_put_user_clac)
+ _ASM_EXTABLE_UA(4b, bad_put_user_clac)
#ifdef CONFIG_X86_32
- _ASM_EXTABLE_UA(5b, bad_put_user)
+ _ASM_EXTABLE_UA(5b, bad_put_user_clac)
#endif
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
2019-07-18 1:36 ` [PATCH v2 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
@ 2019-07-18 19:14 ` tip-bot for Josh Poimboeuf
2020-04-29 21:51 ` BPF vs objtool again Josh Poimboeuf
0 siblings, 1 reply; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:14 UTC (permalink / raw
To: linux-tip-commits
Cc: tglx, linux-kernel, mingo, hpa, ast, peterz, jpoimboe, rdunlap
Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
On x86-64, with CONFIG_RETPOLINE=n, GCC's "global common subexpression
elimination" optimization results in ___bpf_prog_run()'s jumptable code
changing from this:
select_insn:
jmp *jumptable(, %rax, 8)
...
ALU64_ADD_X:
...
jmp *jumptable(, %rax, 8)
ALU_ADD_X:
...
jmp *jumptable(, %rax, 8)
to this:
select_insn:
mov jumptable, %r12
jmp *(%r12, %rax, 8)
...
ALU64_ADD_X:
...
jmp *(%r12, %rax, 8)
ALU_ADD_X:
...
jmp *(%r12, %rax, 8)
The jumptable address is placed in a register once, at the beginning of
the function. The function execution can then go through multiple
indirect jumps which rely on that same register value. This has a few
issues:
1) Objtool isn't smart enough to be able to track such a register value
across multiple recursive indirect jumps through the jump table.
2) With CONFIG_RETPOLINE enabled, this optimization actually results in
a small slowdown. I measured a ~4.7% slowdown in the test_bpf
"tcpdump port 22" selftest.
This slowdown is actually predicted by the GCC manual:
Note: When compiling a program using computed gotos, a GCC
extension, you may get better run-time performance if you
disable the global common subexpression elimination pass by
adding -fno-gcse to the command line.
So just disable the optimization for this function.
Fixes: e55a73251da3 ("bpf: Fix ORC unwinding in non-JIT BPF code")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/30c3ca29ba037afcbd860a8672eef0021addf9fe.1563413318.git.jpoimboe@redhat.com
---
include/linux/compiler-gcc.h | 2 ++
include/linux/compiler_types.h | 4 ++++
kernel/bpf/core.c | 2 +-
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e8579412ad21..d7ee4c6bad48 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -170,3 +170,5 @@
#else
#define __diag_GCC_8(s)
#endif
+
+#define __no_fgcse __attribute__((optimize("-fno-gcse")))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 095d55c3834d..599c27b56c29 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -189,6 +189,10 @@ struct ftrace_likely_data {
#define asm_volatile_goto(x...) asm goto(x)
#endif
+#ifndef __no_fgcse
+# define __no_fgcse
+#endif
+
/* Are two types/vars the same type (ignoring qualifiers)? */
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 7e98f36a14e2..8191a7db2777 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1295,7 +1295,7 @@ bool bpf_opcode_in_insntable(u8 code)
*
* Decode and execute eBPF instructions.
*/
-static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
+static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
{
#define BPF_INSN_2_LBL(x, y) [BPF_##x | BPF_##y] = &&x##_##y
#define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Add mcsafe_handle_tail() to the uaccess safe list
2019-07-18 1:36 ` [PATCH v2 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
@ 2019-07-18 19:14 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:14 UTC (permalink / raw
To: linux-tip-commits
Cc: linux-kernel, ndesaulniers, jpoimboe, tglx, hpa, peterz, mingo
Commit-ID: a7e47f26039c26312a4144c3001b4e9fa886bd45
Gitweb: https://git.kernel.org/tip/a7e47f26039c26312a4144c3001b4e9fa886bd45
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:46 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
objtool: Add mcsafe_handle_tail() to the uaccess safe list
After an objtool improvement, it's reporting that __memcpy_mcsafe() is
calling mcsafe_handle_tail() with AC=1:
arch/x86/lib/memcpy_64.o: warning: objtool: .fixup+0x13: call to mcsafe_handle_tail() with UACCESS enabled
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0x34: (alt)
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0xb: (branch)
arch/x86/lib/memcpy_64.o: warning: objtool: __memcpy_mcsafe()+0x0: <=== (func)
mcsafe_handle_tail() is basically an extension of __memcpy_mcsafe(), so
AC=1 is supposed to be set. Add mcsafe_handle_tail() to the uaccess
safe list.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/035c38f7eac845281d3c3d36749144982e06e58c.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2f8ba0368231..f9494ff8c286 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -490,6 +490,7 @@ static const char *uaccess_safe_builtin[] = {
/* misc */
"csum_partial_copy_generic",
"__memcpy_mcsafe",
+ "mcsafe_handle_tail",
"ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
NULL
};
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Track original function across branches
2019-07-18 1:36 ` [PATCH v2 12/22] objtool: Track original function across branches Josh Poimboeuf
@ 2019-07-18 19:15 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:15 UTC (permalink / raw
To: linux-tip-commits
Cc: jpoimboe, peterz, linux-kernel, mingo, ndesaulniers, hpa, tglx
Commit-ID: c705cecc8431951b4f34178e6b1db51b4a504c43
Gitweb: https://git.kernel.org/tip/c705cecc8431951b4f34178e6b1db51b4a504c43
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:47 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:07 +0200
objtool: Track original function across branches
If 'insn->func' is NULL, objtool skips some important checks, including
sibling call validation. So if some .fixup code does an invalid sibling
call, objtool ignores it.
Treat all code branches (including alts) as part of the original
function by keeping track of the original func value from
validate_functions().
This improves the usefulness of some clang function fallthrough
warnings, and exposes some additional kernel bugs in the process.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/505df630f33c9717e1ccde6e4b64c5303135c25f.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f9494ff8c286..d8a1ce80fded 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1934,13 +1934,12 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
* each instruction and validate all the rules described in
* tools/objtool/Documentation/stack-validation.txt.
*/
-static int validate_branch(struct objtool_file *file, struct instruction *first,
- struct insn_state state)
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+ struct instruction *first, struct insn_state state)
{
struct alternative *alt;
struct instruction *insn, *next_insn;
struct section *sec;
- struct symbol *func = NULL;
int ret;
insn = first;
@@ -1961,9 +1960,6 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
return 1;
}
- if (insn->func)
- func = insn->func->pfunc;
-
if (func && insn->ignore) {
WARN_FUNC("BUG: why am I validating an ignored function?",
sec, insn->offset);
@@ -1985,7 +1981,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
i = insn;
save_insn = NULL;
- func_for_each_insn_continue_reverse(file, insn->func, i) {
+ func_for_each_insn_continue_reverse(file, func, i) {
if (i->save) {
save_insn = i;
break;
@@ -2031,7 +2027,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
if (alt->skip_orig)
skip_orig = true;
- ret = validate_branch(file, alt->insn, state);
+ ret = validate_branch(file, func, alt->insn, state);
if (ret) {
if (backtrace)
BT_FUNC("(alt)", insn);
@@ -2069,7 +2065,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
if (state.bp_scratch) {
WARN("%s uses BP as a scratch register",
- insn->func->name);
+ func->name);
return 1;
}
@@ -2109,8 +2105,8 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
} else if (insn->jump_dest &&
(!func || !insn->jump_dest->func ||
insn->jump_dest->func->pfunc == func)) {
- ret = validate_branch(file, insn->jump_dest,
- state);
+ ret = validate_branch(file, func,
+ insn->jump_dest, state);
if (ret) {
if (backtrace)
BT_FUNC("(branch)", insn);
@@ -2176,7 +2172,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
break;
case INSN_CLAC:
- if (!state.uaccess && insn->func) {
+ if (!state.uaccess && func) {
WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
return 1;
}
@@ -2197,7 +2193,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
break;
case INSN_CLD:
- if (!state.df && insn->func)
+ if (!state.df && func)
WARN_FUNC("redundant CLD", sec, insn->offset);
state.df = false;
@@ -2236,7 +2232,7 @@ static int validate_unwind_hints(struct objtool_file *file)
for_each_insn(file, insn) {
if (insn->hint && !insn->visited) {
- ret = validate_branch(file, insn, state);
+ ret = validate_branch(file, insn->func, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (hint)", insn);
warnings += ret;
@@ -2363,12 +2359,12 @@ static int validate_functions(struct objtool_file *file)
continue;
insn = find_insn(file, sec, func->offset);
- if (!insn || insn->ignore)
+ if (!insn || insn->ignore || insn->visited)
continue;
state.uaccess = func->alias->uaccess_safe;
- ret = validate_branch(file, insn, state);
+ ret = validate_branch(file, func, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (func)", insn);
warnings += ret;
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Refactor function alias logic
2019-07-18 1:36 ` [PATCH v2 13/22] objtool: Refactor function alias logic Josh Poimboeuf
@ 2019-07-18 19:16 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:16 UTC (permalink / raw
To: linux-tip-commits
Cc: tglx, jpoimboe, hpa, ndesaulniers, peterz, mingo, linux-kernel
Commit-ID: e10cd8fe8ddfd28a172d2be57ae0e90c7f752e6a
Gitweb: https://git.kernel.org/tip/e10cd8fe8ddfd28a172d2be57ae0e90c7f752e6a
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:48 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:07 +0200
objtool: Refactor function alias logic
- Add an alias check in validate_functions(). With this change, aliases
no longer need uaccess_safe set.
- Add an alias check in decode_instructions(). With this change, the
"if (!insn->func)" check is no longer needed.
- Don't create aliases for zero-length functions, as it can have
unexpected results. The next patch will spit out a warning for
zero-length functions anyway.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/26a99c31426540f19c9a58b9e10727c385a147bc.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 16 +++++++++-------
tools/objtool/elf.c | 2 +-
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index d8a1ce80fded..3f8664b0e3f9 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -276,7 +276,7 @@ static int decode_instructions(struct objtool_file *file)
}
list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC)
+ if (func->type != STT_FUNC || func->alias != func)
continue;
if (!find_insn(file, sec, func->offset)) {
@@ -286,8 +286,7 @@ static int decode_instructions(struct objtool_file *file)
}
func_for_each_insn(file, func, insn)
- if (!insn->func)
- insn->func = func;
+ insn->func = func;
}
}
@@ -508,7 +507,7 @@ static void add_uaccess_safe(struct objtool_file *file)
if (!func)
continue;
- func->alias->uaccess_safe = true;
+ func->uaccess_safe = true;
}
}
@@ -1887,7 +1886,7 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state)
static inline bool func_uaccess_safe(struct symbol *func)
{
if (func)
- return func->alias->uaccess_safe;
+ return func->uaccess_safe;
return false;
}
@@ -2355,14 +2354,17 @@ static int validate_functions(struct objtool_file *file)
for_each_sec(file, sec) {
list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC || func->pfunc != func)
+ if (func->type != STT_FUNC)
+ continue;
+
+ if (func->pfunc != func || func->alias != func)
continue;
insn = find_insn(file, sec, func->offset);
if (!insn || insn->ignore || insn->visited)
continue;
- state.uaccess = func->alias->uaccess_safe;
+ state.uaccess = func->uaccess_safe;
ret = validate_branch(file, func, insn, state);
if (ret && backtrace)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index e18698262837..9194732a673d 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -278,7 +278,7 @@ static int read_symbols(struct elf *elf)
}
if (sym->offset == s->offset) {
- if (sym->len == s->len && alias == sym)
+ if (sym->len && sym->len == s->len && alias == sym)
alias = s;
if (sym->len >= s->len) {
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Warn on zero-length functions
2019-07-18 1:36 ` [PATCH v2 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
@ 2019-07-18 19:17 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:17 UTC (permalink / raw
To: linux-tip-commits
Cc: peterz, linux-kernel, ndesaulniers, hpa, mingo, tglx, jpoimboe
Commit-ID: 61e9b75a0ccf1fecacc28a2d77ea4a19aa404e39
Gitweb: https://git.kernel.org/tip/61e9b75a0ccf1fecacc28a2d77ea4a19aa404e39
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:49 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:07 +0200
objtool: Warn on zero-length functions
All callable functions should have an ELF size.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/03d429c4fa87829c61c5dc0e89652f4d9efb62f1.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3f8664b0e3f9..dece3253ff6a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2357,6 +2357,12 @@ static int validate_functions(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
+ if (!func->len) {
+ WARN("%s() is missing an ELF size annotation",
+ func->name);
+ warnings++;
+ }
+
if (func->pfunc != func || func->alias != func)
continue;
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Change dead_end_function() to return boolean
2019-07-18 1:36 ` [PATCH v2 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
@ 2019-07-18 19:17 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:17 UTC (permalink / raw
To: linux-tip-commits
Cc: peterz, jpoimboe, ndesaulniers, hpa, tglx, mingo, linux-kernel
Commit-ID: 8e25c9f8b482ea8d8b6fb4f6f5c09bcc5ee18663
Gitweb: https://git.kernel.org/tip/8e25c9f8b482ea8d8b6fb4f6f5c09bcc5ee18663
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:50 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:08 +0200
objtool: Change dead_end_function() to return boolean
dead_end_function() can no longer return an error. Simplify its
interface by making it return boolean.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/9e6679610768fb6e6c51dca23f7d4d0c03b0c910.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 36 ++++++++++++++----------------------
1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index dece3253ff6a..d9d1c9b54947 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -105,14 +105,9 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
*
* For local functions, we have to detect them manually by simply looking for
* the lack of a return instruction.
- *
- * Returns:
- * -1: error
- * 0: no dead end
- * 1: dead end
*/
-static int __dead_end_function(struct objtool_file *file, struct symbol *func,
- int recursion)
+static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
+ int recursion)
{
int i;
struct instruction *insn;
@@ -139,29 +134,29 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
};
if (func->bind == STB_WEAK)
- return 0;
+ return false;
if (func->bind == STB_GLOBAL)
for (i = 0; i < ARRAY_SIZE(global_noreturns); i++)
if (!strcmp(func->name, global_noreturns[i]))
- return 1;
+ return true;
if (!func->len)
- return 0;
+ return false;
insn = find_insn(file, func->sec, func->offset);
if (!insn->func)
- return 0;
+ return false;
func_for_each_insn_all(file, func, insn) {
empty = false;
if (insn->type == INSN_RETURN)
- return 0;
+ return false;
}
if (empty)
- return 0;
+ return false;
/*
* A function can have a sibling call instead of a return. In that
@@ -174,7 +169,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
if (!dest)
/* sibling call to another file */
- return 0;
+ return false;
if (dest->func && dest->func->pfunc != insn->func->pfunc) {
@@ -186,7 +181,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
* This is a very rare case. It means
* they aren't dead ends.
*/
- return 0;
+ return false;
}
return __dead_end_function(file, dest->func,
@@ -196,13 +191,13 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
if (insn->type == INSN_JUMP_DYNAMIC && list_empty(&insn->alts))
/* sibling call */
- return 0;
+ return false;
}
- return 1;
+ return true;
}
-static int dead_end_function(struct objtool_file *file, struct symbol *func)
+static bool dead_end_function(struct objtool_file *file, struct symbol *func)
{
return __dead_end_function(file, func, 0);
}
@@ -2080,11 +2075,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (is_fentry_call(insn))
break;
- ret = dead_end_function(file, insn->call_dest);
- if (ret == 1)
+ if (dead_end_function(file, insn->call_dest))
return 0;
- if (ret == -1)
- return 1;
}
if (!no_fp && func && !has_valid_stack_frame(&state)) {
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Do frame pointer check before dead end check
2019-07-18 1:36 ` [PATCH v2 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
@ 2019-07-18 19:18 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:18 UTC (permalink / raw
To: linux-tip-commits
Cc: hpa, linux-kernel, peterz, ndesaulniers, tglx, jpoimboe, mingo
Commit-ID: c9bab22bc449ad2496a6bbbf68acc711d9c5301c
Gitweb: https://git.kernel.org/tip/c9bab22bc449ad2496a6bbbf68acc711d9c5301c
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:51 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:08 +0200
objtool: Do frame pointer check before dead end check
Even calls to __noreturn functions need the frame pointer setup first.
Such functions often dump the stack.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/aed62fbd60e239280218be623f751a433658e896.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index d9d1c9b54947..0d2a8e54a82e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -133,6 +133,9 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
"rewind_stack_do_exit",
};
+ if (!func)
+ return false;
+
if (func->bind == STB_WEAK)
return false;
@@ -2071,19 +2074,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (ret)
return ret;
- if (insn->type == INSN_CALL) {
- if (is_fentry_call(insn))
- break;
-
- if (dead_end_function(file, insn->call_dest))
- return 0;
- }
-
- if (!no_fp && func && !has_valid_stack_frame(&state)) {
+ if (!no_fp && func && !is_fentry_call(insn) &&
+ !has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
return 1;
}
+
+ if (dead_end_function(file, insn->call_dest))
+ return 0;
+
break;
case INSN_JUMP_CONDITIONAL:
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Refactor sibling call detection logic
2019-07-18 1:36 ` [PATCH v2 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
@ 2019-07-18 19:19 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:19 UTC (permalink / raw
To: linux-tip-commits
Cc: tglx, linux-kernel, ndesaulniers, mingo, jpoimboe, hpa, peterz
Commit-ID: 0c1ddd33177530feb3685a800bba1ac4cc58cc4b
Gitweb: https://git.kernel.org/tip/0c1ddd33177530feb3685a800bba1ac4cc58cc4b
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:52 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:08 +0200
objtool: Refactor sibling call detection logic
Simplify the sibling call detection logic a bit.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/8357dbef9e7f5512e76bf83a76c81722fc09eb5e.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 65 ++++++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 32 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0d2a8e54a82e..7fe31e0f8afe 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -97,6 +97,20 @@ static struct instruction *next_insn_same_func(struct objtool_file *file,
for (insn = next_insn_same_sec(file, insn); insn; \
insn = next_insn_same_sec(file, insn))
+static bool is_sibling_call(struct instruction *insn)
+{
+ /* An indirect jump is either a sibling call or a jump to a table. */
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return list_empty(&insn->alts);
+
+ if (insn->type != INSN_JUMP_CONDITIONAL &&
+ insn->type != INSN_JUMP_UNCONDITIONAL)
+ return false;
+
+ /* add_jump_destinations() sets insn->call_dest for sibling calls. */
+ return !!insn->call_dest;
+}
+
/*
* This checks to see if the given function is a "noreturn" function.
*
@@ -167,34 +181,25 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
* of the sibling call returns.
*/
func_for_each_insn_all(file, func, insn) {
- if (insn->type == INSN_JUMP_UNCONDITIONAL) {
+ if (is_sibling_call(insn)) {
struct instruction *dest = insn->jump_dest;
if (!dest)
/* sibling call to another file */
return false;
- if (dest->func && dest->func->pfunc != insn->func->pfunc) {
-
- /* local sibling call */
- if (recursion == 5) {
- /*
- * Infinite recursion: two functions
- * have sibling calls to each other.
- * This is a very rare case. It means
- * they aren't dead ends.
- */
- return false;
- }
-
- return __dead_end_function(file, dest->func,
- recursion + 1);
+ /* local sibling call */
+ if (recursion == 5) {
+ /*
+ * Infinite recursion: two functions have
+ * sibling calls to each other. This is a very
+ * rare case. It means they aren't dead ends.
+ */
+ return false;
}
- }
- if (insn->type == INSN_JUMP_DYNAMIC && list_empty(&insn->alts))
- /* sibling call */
- return false;
+ return __dead_end_function(file, dest->func, recursion+1);
+ }
}
return true;
@@ -581,9 +586,8 @@ static int add_jump_destinations(struct objtool_file *file)
insn->retpoline_safe = true;
continue;
} else {
- /* sibling call */
+ /* external sibling call */
insn->call_dest = rela->sym;
- insn->jump_dest = NULL;
continue;
}
@@ -633,9 +637,8 @@ static int add_jump_destinations(struct objtool_file *file)
} else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
insn->jump_dest->offset == insn->jump_dest->func->offset) {
- /* sibling class */
+ /* internal sibling call */
insn->call_dest = insn->jump_dest->func;
- insn->jump_dest = NULL;
}
}
}
@@ -1889,7 +1892,7 @@ static inline bool func_uaccess_safe(struct symbol *func)
return false;
}
-static inline const char *insn_dest_name(struct instruction *insn)
+static inline const char *call_dest_name(struct instruction *insn)
{
if (insn->call_dest)
return insn->call_dest->name;
@@ -1901,13 +1904,13 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
{
if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
WARN_FUNC("call to %s() with UACCESS enabled",
- insn->sec, insn->offset, insn_dest_name(insn));
+ insn->sec, insn->offset, call_dest_name(insn));
return 1;
}
if (state->df) {
WARN_FUNC("call to %s() with DF set",
- insn->sec, insn->offset, insn_dest_name(insn));
+ insn->sec, insn->offset, call_dest_name(insn));
return 1;
}
@@ -2088,14 +2091,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
case INSN_JUMP_CONDITIONAL:
case INSN_JUMP_UNCONDITIONAL:
- if (func && !insn->jump_dest) {
+ if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
- } else if (insn->jump_dest &&
- (!func || !insn->jump_dest->func ||
- insn->jump_dest->func->pfunc == func)) {
+ } else if (insn->jump_dest) {
ret = validate_branch(file, func,
insn->jump_dest, state);
if (ret) {
@@ -2111,7 +2112,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_JUMP_DYNAMIC:
- if (func && list_empty(&insn->alts)) {
+ if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Refactor jump table code
2019-07-18 1:36 ` [PATCH v2 18/22] objtool: Refactor jump table code Josh Poimboeuf
@ 2019-07-18 19:20 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:20 UTC (permalink / raw
To: linux-tip-commits
Cc: mingo, peterz, jpoimboe, tglx, linux-kernel, hpa, ndesaulniers
Commit-ID: e7c2bc37bfae120bce3e7cc8c8abf9d110af0757
Gitweb: https://git.kernel.org/tip/e7c2bc37bfae120bce3e7cc8c8abf9d110af0757
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:53 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:09 +0200
objtool: Refactor jump table code
Now that C jump tables are supported, call them "jump tables" instead of
"switch tables". Also rename some other variables, add comments, and
simplify the code flow a bit.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/cf951b0c0641628e0b9b81f7ceccd9bcabcb4bd8.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 82 +++++++++++++++++++++++++++------------------------
tools/objtool/elf.c | 2 +-
tools/objtool/elf.h | 2 +-
3 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7fe31e0f8afe..4525cf677a1b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -627,7 +627,7 @@ static int add_jump_destinations(struct objtool_file *file)
* 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 switch table.
+ * subfunction is through a jump table.
*/
if (!strstr(insn->func->name, ".cold.") &&
strstr(insn->jump_dest->func->name, ".cold.")) {
@@ -899,20 +899,24 @@ out:
return ret;
}
-static int add_switch_table(struct objtool_file *file, struct instruction *insn,
+static int add_jump_table(struct objtool_file *file, struct instruction *insn,
struct rela *table, struct rela *next_table)
{
struct rela *rela = table;
- struct instruction *alt_insn;
+ struct instruction *dest_insn;
struct alternative *alt;
struct symbol *pfunc = insn->func->pfunc;
unsigned int prev_offset = 0;
- list_for_each_entry_from(rela, &table->rela_sec->rela_list, list) {
+ /*
+ * Each @rela is a switch table relocation which points to the target
+ * instruction.
+ */
+ list_for_each_entry_from(rela, &table->sec->rela_list, list) {
if (rela == next_table)
break;
- /* Make sure the switch table entries are consecutive: */
+ /* Make sure the table entries are consecutive: */
if (prev_offset && rela->offset != prev_offset + 8)
break;
@@ -921,12 +925,12 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
rela->addend == pfunc->offset)
break;
- alt_insn = find_insn(file, rela->sym->sec, rela->addend);
- if (!alt_insn)
+ dest_insn = find_insn(file, rela->sym->sec, rela->addend);
+ if (!dest_insn)
break;
- /* Make sure the jmp dest is in the function or subfunction: */
- if (alt_insn->func->pfunc != pfunc)
+ /* Make sure the destination is in the same function: */
+ if (dest_insn->func->pfunc != pfunc)
break;
alt = malloc(sizeof(*alt));
@@ -935,7 +939,7 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
return -1;
}
- alt->insn = alt_insn;
+ alt->insn = dest_insn;
list_add_tail(&alt->list, &insn->alts);
prev_offset = rela->offset;
}
@@ -950,7 +954,7 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
}
/*
- * find_switch_table() - Given a dynamic jump, find the switch jump table in
+ * find_jump_table() - Given a dynamic jump, find the switch jump table in
* .rodata associated with it.
*
* There are 3 basic patterns:
@@ -992,13 +996,13 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
*
* NOTE: RETPOLINE made it harder still to decode dynamic jumps.
*/
-static struct rela *find_switch_table(struct objtool_file *file,
+static struct rela *find_jump_table(struct objtool_file *file,
struct symbol *func,
struct instruction *insn)
{
- struct rela *text_rela, *rodata_rela;
+ struct rela *text_rela, *table_rela;
struct instruction *orig_insn = insn;
- struct section *rodata_sec;
+ struct section *table_sec;
unsigned long table_offset;
/*
@@ -1031,7 +1035,7 @@ static struct rela *find_switch_table(struct objtool_file *file,
continue;
table_offset = text_rela->addend;
- rodata_sec = text_rela->sym->sec;
+ table_sec = text_rela->sym->sec;
if (text_rela->type == R_X86_64_PC32)
table_offset += 4;
@@ -1045,29 +1049,31 @@ static struct rela *find_switch_table(struct objtool_file *file,
* need to be placed in the C_JUMP_TABLE_SECTION section. They
* have symbols associated with them.
*/
- if (find_symbol_containing(rodata_sec, table_offset) &&
- strcmp(rodata_sec->name, C_JUMP_TABLE_SECTION))
+ if (find_symbol_containing(table_sec, table_offset) &&
+ strcmp(table_sec->name, C_JUMP_TABLE_SECTION))
continue;
- rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
- if (rodata_rela) {
- /*
- * Use of RIP-relative switch jumps is quite rare, and
- * indicates a rare GCC quirk/bug which can leave dead
- * code behind.
- */
- if (text_rela->type == R_X86_64_PC32)
- file->ignore_unreachables = true;
+ /* Each table entry has a rela associated with it. */
+ table_rela = find_rela_by_dest(table_sec, table_offset);
+ if (!table_rela)
+ continue;
- return rodata_rela;
- }
+ /*
+ * Use of RIP-relative switch jumps is quite rare, and
+ * indicates a rare GCC quirk/bug which can leave dead code
+ * behind.
+ */
+ if (text_rela->type == R_X86_64_PC32)
+ file->ignore_unreachables = true;
+
+ return table_rela;
}
return NULL;
}
-static int add_func_switch_tables(struct objtool_file *file,
+static int add_func_jump_tables(struct objtool_file *file,
struct symbol *func)
{
struct instruction *insn, *last = NULL, *prev_jump = NULL;
@@ -1080,7 +1086,7 @@ static int add_func_switch_tables(struct objtool_file *file,
/*
* Store back-pointers for unconditional forward jumps such
- * that find_switch_table() can back-track using those and
+ * that find_jump_table() can back-track using those and
* avoid some potentially confusing code.
*/
if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->jump_dest &&
@@ -1095,17 +1101,17 @@ static int add_func_switch_tables(struct objtool_file *file,
if (insn->type != INSN_JUMP_DYNAMIC)
continue;
- rela = find_switch_table(file, func, insn);
+ rela = find_jump_table(file, func, insn);
if (!rela)
continue;
/*
- * We found a switch table, but we don't know yet how big it
+ * We found a jump table, but we don't know yet how big it
* is. Don't add it until we reach the end of the function or
- * the beginning of another switch table in the same function.
+ * the beginning of another jump table in the same function.
*/
if (prev_jump) {
- ret = add_switch_table(file, prev_jump, prev_rela, rela);
+ ret = add_jump_table(file, prev_jump, prev_rela, rela);
if (ret)
return ret;
}
@@ -1115,7 +1121,7 @@ static int add_func_switch_tables(struct objtool_file *file,
}
if (prev_jump) {
- ret = add_switch_table(file, prev_jump, prev_rela, NULL);
+ ret = add_jump_table(file, prev_jump, prev_rela, NULL);
if (ret)
return ret;
}
@@ -1128,7 +1134,7 @@ static int add_func_switch_tables(struct objtool_file *file,
* section which contains a list of addresses within the function to jump to.
* This finds these jump tables and adds them to the insn->alts lists.
*/
-static int add_switch_table_alts(struct objtool_file *file)
+static int add_jump_table_alts(struct objtool_file *file)
{
struct section *sec;
struct symbol *func;
@@ -1142,7 +1148,7 @@ static int add_switch_table_alts(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
- ret = add_func_switch_tables(file, func);
+ ret = add_func_jump_tables(file, func);
if (ret)
return ret;
}
@@ -1339,7 +1345,7 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
- ret = add_switch_table_alts(file);
+ ret = add_jump_table_alts(file);
if (ret)
return ret;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 9194732a673d..edba4745f25a 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -385,7 +385,7 @@ static int read_relas(struct elf *elf)
rela->offset = rela->rela.r_offset;
symndx = GELF_R_SYM(rela->rela.r_info);
rela->sym = find_symbol_by_index(elf, symndx);
- rela->rela_sec = sec;
+ rela->sec = sec;
if (!rela->sym) {
WARN("can't find rela entry symbol %d for %s",
symndx, sec->name);
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 2fe0b0aa741d..d4d3e0528d4a 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -57,7 +57,7 @@ struct rela {
struct list_head list;
struct hlist_node hash;
GElf_Rela rela;
- struct section *rela_sec;
+ struct section *sec;
struct symbol *sym;
unsigned int type;
unsigned long offset;
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Support repeated uses of the same C jump table
2019-07-18 1:36 ` [PATCH v2 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
@ 2019-07-18 19:20 ` tip-bot for Jann Horn
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Jann Horn @ 2019-07-18 19:20 UTC (permalink / raw
To: linux-tip-commits
Cc: ndesaulniers, linux-kernel, mingo, tglx, arnd, peterz, rdunlap,
hpa, jpoimboe, jannh
Commit-ID: bd98c81346468fc2f86aeeb44d4d0d6f763a62b7
Gitweb: https://git.kernel.org/tip/bd98c81346468fc2f86aeeb44d4d0d6f763a62b7
Author: Jann Horn <jannh@google.com>
AuthorDate: Wed, 17 Jul 2019 20:36:54 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:09 +0200
objtool: Support repeated uses of the same C jump table
This fixes objtool for both a GCC issue and a Clang issue:
1) GCC issue:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x8d5: sibling call from callable instruction with modified stack frame
With CONFIG_RETPOLINE=n, GCC is doing the following optimization in
___bpf_prog_run().
Before:
select_insn:
jmp *jumptable(,%rax,8)
...
ALU64_ADD_X:
...
jmp select_insn
ALU_ADD_X:
...
jmp select_insn
After:
select_insn:
jmp *jumptable(, %rax, 8)
...
ALU64_ADD_X:
...
jmp *jumptable(, %rax, 8)
ALU_ADD_X:
...
jmp *jumptable(, %rax, 8)
This confuses objtool. It has never seen multiple indirect jump
sites which use the same jump table.
For GCC switch tables, the only way of detecting the size of a table
is by continuing to scan for more tables. The size of the previous
table can only be determined after another switch table is found, or
when the scan reaches the end of the function.
That logic was reused for C jump tables, and was based on the
assumption that each jump table only has a single jump site. The
above optimization breaks that assumption.
2) Clang issue:
drivers/usb/misc/sisusbvga/sisusb.o: warning: objtool: sisusb_write_mem_bulk()+0x588: can't find switch jump table
With clang 9, code can be generated where a function contains two
indirect jump instructions which use the same switch table.
The fix is the same for both issues: split the jump table parsing into
two passes.
In the first pass, locate the heads of all switch tables for the
function and mark their locations.
In the second pass, parse the switch tables and add them.
Fixes: e55a73251da3 ("bpf: Fix ORC unwinding in non-JIT BPF code")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/e995befaada9d4d8b2cf788ff3f566ba900d2b4d.1563413318.git.jpoimboe@redhat.com
Co-developed-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/check.c | 53 +++++++++++++++++++++++++++------------------------
tools/objtool/check.h | 1 +
tools/objtool/elf.h | 1 +
3 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4525cf677a1b..66f7c01385a4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -900,7 +900,7 @@ out:
}
static int add_jump_table(struct objtool_file *file, struct instruction *insn,
- struct rela *table, struct rela *next_table)
+ struct rela *table)
{
struct rela *rela = table;
struct instruction *dest_insn;
@@ -913,7 +913,9 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
* instruction.
*/
list_for_each_entry_from(rela, &table->sec->rela_list, list) {
- if (rela == next_table)
+
+ /* Check for the end of the table: */
+ if (rela != table && rela->jump_table_start)
break;
/* Make sure the table entries are consecutive: */
@@ -1072,13 +1074,15 @@ static struct rela *find_jump_table(struct objtool_file *file,
return NULL;
}
-
-static int add_func_jump_tables(struct objtool_file *file,
- struct symbol *func)
+/*
+ * First pass: Mark the head of each jump table so that in the next pass,
+ * we know when a given jump table ends and the next one starts.
+ */
+static void mark_func_jump_tables(struct objtool_file *file,
+ struct symbol *func)
{
- struct instruction *insn, *last = NULL, *prev_jump = NULL;
- struct rela *rela, *prev_rela = NULL;
- int ret;
+ struct instruction *insn, *last = NULL;
+ struct rela *rela;
func_for_each_insn_all(file, func, insn) {
if (!last)
@@ -1102,26 +1106,24 @@ static int add_func_jump_tables(struct objtool_file *file,
continue;
rela = find_jump_table(file, func, insn);
- if (!rela)
- continue;
-
- /*
- * We found a jump table, but we don't know yet how big it
- * is. Don't add it until we reach the end of the function or
- * the beginning of another jump table in the same function.
- */
- if (prev_jump) {
- ret = add_jump_table(file, prev_jump, prev_rela, rela);
- if (ret)
- return ret;
+ if (rela) {
+ rela->jump_table_start = true;
+ insn->jump_table = rela;
}
-
- prev_jump = insn;
- prev_rela = rela;
}
+}
+
+static int add_func_jump_tables(struct objtool_file *file,
+ struct symbol *func)
+{
+ struct instruction *insn;
+ int ret;
+
+ func_for_each_insn_all(file, func, insn) {
+ if (!insn->jump_table)
+ continue;
- if (prev_jump) {
- ret = add_jump_table(file, prev_jump, prev_rela, NULL);
+ ret = add_jump_table(file, insn, insn->jump_table);
if (ret)
return ret;
}
@@ -1148,6 +1150,7 @@ static int add_jump_table_alts(struct objtool_file *file)
if (func->type != STT_FUNC)
continue;
+ mark_func_jump_tables(file, func);
ret = add_func_jump_tables(file, func);
if (ret)
return ret;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index cb60b9acf5cf..afa6a79e0715 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -38,6 +38,7 @@ struct instruction {
struct symbol *call_dest;
struct instruction *jump_dest;
struct instruction *first_jump_src;
+ struct rela *jump_table;
struct list_head alts;
struct symbol *func;
struct stack_op stack_op;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index d4d3e0528d4a..44150204db4d 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@ struct rela {
unsigned int type;
unsigned long offset;
int addend;
+ bool jump_table_start;
};
struct elf {
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Fix seg fault on bad switch table entry
2019-07-18 1:36 ` [PATCH v2 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
@ 2019-07-18 19:21 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:21 UTC (permalink / raw
To: linux-tip-commits
Cc: hpa, linux-kernel, jpoimboe, mingo, ndesaulniers, arnd, tglx,
peterz
Commit-ID: e65050b94d8c518fdbee572ea4ca6d352e1fda37
Gitweb: https://git.kernel.org/tip/e65050b94d8c518fdbee572ea4ca6d352e1fda37
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:55 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:10 +0200
objtool: Fix seg fault on bad switch table entry
In one rare case, Clang generated the following code:
5ca: 83 e0 21 and $0x21,%eax
5cd: b9 04 00 00 00 mov $0x4,%ecx
5d2: ff 24 c5 00 00 00 00 jmpq *0x0(,%rax,8)
5d5: R_X86_64_32S .rodata+0x38
which uses the corresponding jump table relocations:
000000000038 000200000001 R_X86_64_64 0000000000000000 .text + 834
000000000040 000200000001 R_X86_64_64 0000000000000000 .text + 5d9
000000000048 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000050 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000058 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000060 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000068 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000070 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000078 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000080 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000088 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000090 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000098 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000a0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000a8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000b0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000b8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000c0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000c8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000d0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000d8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000e0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000e8 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000f0 000200000001 R_X86_64_64 0000000000000000 .text + b96
0000000000f8 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000100 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000108 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000110 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000118 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000120 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000128 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000130 000200000001 R_X86_64_64 0000000000000000 .text + b96
000000000138 000200000001 R_X86_64_64 0000000000000000 .text + 82f
000000000140 000200000001 R_X86_64_64 0000000000000000 .text + 828
Since %eax was masked with 0x21, only the first two and the last two
entries are possible.
Objtool doesn't actually emulate all the code, so it isn't smart enough
to know that all the middle entries aren't reachable. They point to the
NOP padding area after the end of the function, so objtool seg faulted
when it tried to dereference a NULL insn->func.
After this fix, objtool still gives an "unreachable" error because it
stops reading the jump table when it encounters the bad addresses:
/home/jpoimboe/objtool-tests/adm1275.o: warning: objtool: adm1275_probe()+0x828: unreachable instruction
While the above code is technically correct, it's very wasteful of
memory -- it uses 34 jump table entries when only 4 are needed. It's
also not possible for objtool to validate this type of switch table
because the unused entries point outside the function and objtool has no
way of determining if that's intentional. Hopefully the Clang folks can
fix it.
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/a9db88eec4f1ca089e040989846961748238b6d8.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 66f7c01385a4..a966b22a32ef 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -932,7 +932,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
break;
/* Make sure the destination is in the same function: */
- if (dest_insn->func->pfunc != pfunc)
+ if (!dest_insn->func || dest_insn->func->pfunc != pfunc)
break;
alt = malloc(sizeof(*alt));
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Convert insn type to enum
2019-07-18 1:36 ` [PATCH v2 21/22] objtool: convert insn type to enum Josh Poimboeuf
@ 2019-07-18 19:22 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:22 UTC (permalink / raw
To: linux-tip-commits
Cc: peterz, linux-kernel, jpoimboe, tglx, ndesaulniers, mingo, hpa
Commit-ID: 9fe7b7642fe2c5158904d06fe31b740ca0695a01
Gitweb: https://git.kernel.org/tip/9fe7b7642fe2c5158904d06fe31b740ca0695a01
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:56 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:10 +0200
objtool: Convert insn type to enum
This makes it easier to add new instruction types. Also it's hopefully
more robust since the compiler should warn about out-of-range enums.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/0740e96af0d40e54cfd6a07bf09db0fbd10793cd.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/arch.h | 35 ++++++++++++++++++-----------------
tools/objtool/arch/x86/decode.c | 2 +-
tools/objtool/check.c | 7 -------
tools/objtool/check.h | 2 +-
4 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 580e344db3dd..50448c0c4bca 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -11,22 +11,23 @@
#include "elf.h"
#include "cfi.h"
-#define INSN_JUMP_CONDITIONAL 1
-#define INSN_JUMP_UNCONDITIONAL 2
-#define INSN_JUMP_DYNAMIC 3
-#define INSN_CALL 4
-#define INSN_CALL_DYNAMIC 5
-#define INSN_RETURN 6
-#define INSN_CONTEXT_SWITCH 7
-#define INSN_STACK 8
-#define INSN_BUG 9
-#define INSN_NOP 10
-#define INSN_STAC 11
-#define INSN_CLAC 12
-#define INSN_STD 13
-#define INSN_CLD 14
-#define INSN_OTHER 15
-#define INSN_LAST INSN_OTHER
+enum insn_type {
+ INSN_JUMP_CONDITIONAL,
+ INSN_JUMP_UNCONDITIONAL,
+ INSN_JUMP_DYNAMIC,
+ INSN_CALL,
+ INSN_CALL_DYNAMIC,
+ INSN_RETURN,
+ INSN_CONTEXT_SWITCH,
+ INSN_STACK,
+ INSN_BUG,
+ INSN_NOP,
+ INSN_STAC,
+ INSN_CLAC,
+ INSN_STD,
+ INSN_CLD,
+ INSN_OTHER,
+};
enum op_dest_type {
OP_DEST_REG,
@@ -68,7 +69,7 @@ void arch_initial_func_cfi_state(struct cfi_state *state);
int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int maxlen,
- unsigned int *len, unsigned char *type,
+ unsigned int *len, enum insn_type *type,
unsigned long *immediate, struct stack_op *op);
bool arch_callee_saved_reg(unsigned char reg);
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 584568f27a83..0567c47a91b1 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -68,7 +68,7 @@ bool arch_callee_saved_reg(unsigned char reg)
int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int maxlen,
- unsigned int *len, unsigned char *type,
+ unsigned int *len, enum insn_type *type,
unsigned long *immediate, struct stack_op *op)
{
struct insn insn;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a966b22a32ef..04572a049cfc 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,13 +267,6 @@ static int decode_instructions(struct objtool_file *file)
if (ret)
goto err;
- if (!insn->type || insn->type > INSN_LAST) {
- WARN_FUNC("invalid instruction type %d",
- insn->sec, insn->offset, insn->type);
- ret = -1;
- goto err;
- }
-
hash_add(file->insn_hash, &insn->hash, insn->offset);
list_add_tail(&insn->list, &file->insn_list);
}
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index afa6a79e0715..b881fafcf55d 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct instruction {
struct section *sec;
unsigned long offset;
unsigned int len;
- unsigned char type;
+ enum insn_type type;
unsigned long immediate;
bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
bool retpoline_safe;
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [tip:core/urgent] objtool: Support conditional retpolines
2019-07-18 1:36 ` [PATCH v2 22/22] objtool: Support conditional retpolines Josh Poimboeuf
@ 2019-07-18 19:23 ` tip-bot for Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-07-18 19:23 UTC (permalink / raw
To: linux-tip-commits
Cc: linux-kernel, jpoimboe, ndesaulniers, tglx, mingo, hpa, peterz
Commit-ID: b68b9907069a8d3a65bc16a35360bf8f8603c8fa
Gitweb: https://git.kernel.org/tip/b68b9907069a8d3a65bc16a35360bf8f8603c8fa
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 17 Jul 2019 20:36:57 -0500
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 18 Jul 2019 21:01:10 +0200
objtool: Support conditional retpolines
A Clang-built kernel is showing the following warning:
arch/x86/kernel/platform-quirks.o: warning: objtool: x86_early_init_platform_quirks()+0x84: unreachable instruction
That corresponds to this code:
7e: 0f 85 00 00 00 00 jne 84 <x86_early_init_platform_quirks+0x84>
80: R_X86_64_PC32 __x86_indirect_thunk_r11-0x4
84: c3 retq
This is a conditional retpoline sibling call, which is now possible
thanks to retpolines. Objtool hasn't seen that before. It's
incorrectly interpreting the conditional jump as an unconditional
dynamic jump.
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/30d4c758b267ef487fb97e6ecb2f148ad007b554.1563413318.git.jpoimboe@redhat.com
---
tools/objtool/arch.h | 1 +
tools/objtool/check.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 50448c0c4bca..ced3765c4f44 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -15,6 +15,7 @@ enum insn_type {
INSN_JUMP_CONDITIONAL,
INSN_JUMP_UNCONDITIONAL,
INSN_JUMP_DYNAMIC,
+ INSN_JUMP_DYNAMIC_CONDITIONAL,
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 04572a049cfc..5f26620f13f5 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -575,7 +575,11 @@ static int add_jump_destinations(struct objtool_file *file)
* Retpoline jumps are really dynamic jumps in
* disguise, so convert them accordingly.
*/
- insn->type = INSN_JUMP_DYNAMIC;
+ if (insn->type == INSN_JUMP_UNCONDITIONAL)
+ insn->type = INSN_JUMP_DYNAMIC;
+ else
+ insn->type = INSN_JUMP_DYNAMIC_CONDITIONAL;
+
insn->retpoline_safe = true;
continue;
} else {
@@ -2114,13 +2118,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
if (func && is_sibling_call(insn)) {
ret = validate_sibling_call(insn, &state);
if (ret)
return ret;
}
- return 0;
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return 0;
+
+ break;
case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
^ permalink raw reply related [flat|nested] 60+ messages in thread
* BPF vs objtool again
2019-07-18 19:14 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
@ 2020-04-29 21:51 ` Josh Poimboeuf
2020-04-29 22:01 ` Arvind Sankar
2020-04-29 23:41 ` Alexei Starovoitov
0 siblings, 2 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2020-04-29 21:51 UTC (permalink / raw
To: x86; +Cc: tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann
On Thu, Jul 18, 2019 at 12:14:08PM -0700, tip-bot for Josh Poimboeuf wrote:
> Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
> Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
> Author: Josh Poimboeuf <jpoimboe@redhat.com>
> AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
> Committer: Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
>
> bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
For some reason, this
__attribute__((optimize("-fno-gcse")))
is disabling frame pointers in ___bpf_prog_run(). If you compile with
CONFIG_FRAME_POINTER it'll show something like:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run.cold()+0x7: call without frame pointer save/setup
Also, since GCC 9.1, the GCC docs say "The optimize attribute should be
used for debugging purposes only. It is not suitable in production
code." That doesn't sound too promising.
So it seems like this commit should be reverted. But then we're back to
objtool being broken again in the RETPOLINE=n case, which means no ORC
coverage in this function. (See above commit for the details)
Some ideas:
- Skip objtool checking of that func/file (at least for RETPOLINE=n) --
but then it won't have ORC coverage.
- Get rid of the "double goto" in ___bpf_prog_run(), which simplifies it
enough for objtool to understand -- but then the text explodes for
RETPOLINE=y.
- Add -fno-gfcse to the Makefile for kernel/bpf/core.c -- but then that
affects the optimization of other functions in the file. However I
don't think the impact is significant.
- Move ___bpf_prog_run() to its own file with the -fno-gfcse flag. I'm
thinking this could be the least bad option. Alexei?
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-29 21:51 ` BPF vs objtool again Josh Poimboeuf
@ 2020-04-29 22:01 ` Arvind Sankar
2020-04-29 23:41 ` Alexei Starovoitov
1 sibling, 0 replies; 60+ messages in thread
From: Arvind Sankar @ 2020-04-29 22:01 UTC (permalink / raw
To: Josh Poimboeuf
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann
On Wed, Apr 29, 2020 at 04:51:59PM -0500, Josh Poimboeuf wrote:
> On Thu, Jul 18, 2019 at 12:14:08PM -0700, tip-bot for Josh Poimboeuf wrote:
> > Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
> > Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
> > Author: Josh Poimboeuf <jpoimboe@redhat.com>
> > AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
> > Committer: Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
> >
> > bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
>
> For some reason, this
>
> __attribute__((optimize("-fno-gcse")))
>
> is disabling frame pointers in ___bpf_prog_run(). If you compile with
> CONFIG_FRAME_POINTER it'll show something like:
>
> kernel/bpf/core.o: warning: objtool: ___bpf_prog_run.cold()+0x7: call without frame pointer save/setup
>
> Also, since GCC 9.1, the GCC docs say "The optimize attribute should be
> used for debugging purposes only. It is not suitable in production
> code." That doesn't sound too promising.
>
It turns out that the optimize attribute doesn't append options to the
command-line arguments, it starts from the defaults and only adds
whatever you specify in the attribute. So it's not very useful for
production code.
See this for eg where the same thing came up in a different context.
https://lore.kernel.org/lkml/alpine.LSU.2.21.2004151445520.11688@wotan.suse.de/
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-29 21:51 ` BPF vs objtool again Josh Poimboeuf
2020-04-29 22:01 ` Arvind Sankar
@ 2020-04-29 23:41 ` Alexei Starovoitov
2020-04-30 0:13 ` Josh Poimboeuf
1 sibling, 1 reply; 60+ messages in thread
From: Alexei Starovoitov @ 2020-04-29 23:41 UTC (permalink / raw
To: Josh Poimboeuf
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 04:51:59PM -0500, Josh Poimboeuf wrote:
> On Thu, Jul 18, 2019 at 12:14:08PM -0700, tip-bot for Josh Poimboeuf wrote:
> > Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
> > Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
> > Author: Josh Poimboeuf <jpoimboe@redhat.com>
> > AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
> > Committer: Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
> >
> > bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
>
> For some reason, this
>
> __attribute__((optimize("-fno-gcse")))
>
> is disabling frame pointers in ___bpf_prog_run(). If you compile with
> CONFIG_FRAME_POINTER it'll show something like:
>
> kernel/bpf/core.o: warning: objtool: ___bpf_prog_run.cold()+0x7: call without frame pointer save/setup
you mean it started to disable frame pointers from some version of gcc?
It wasn't doing this before, since objtool wasn't complaining, right?
Sounds like gcc bug?
> Also, since GCC 9.1, the GCC docs say "The optimize attribute should be
> used for debugging purposes only. It is not suitable in production
> code." That doesn't sound too promising.
>
> So it seems like this commit should be reverted. But then we're back to
> objtool being broken again in the RETPOLINE=n case, which means no ORC
> coverage in this function. (See above commit for the details)
>
> Some ideas:
>
> - Skip objtool checking of that func/file (at least for RETPOLINE=n) --
> but then it won't have ORC coverage.
>
> - Get rid of the "double goto" in ___bpf_prog_run(), which simplifies it
> enough for objtool to understand -- but then the text explodes for
> RETPOLINE=y.
How that will look like?
That could be the best option.
> - Add -fno-gfcse to the Makefile for kernel/bpf/core.c -- but then that
> affects the optimization of other functions in the file. However I
> don't think the impact is significant.
>
> - Move ___bpf_prog_run() to its own file with the -fno-gfcse flag. I'm
> thinking this could be the least bad option. Alexei?
I think it would be easier to move some of the hot path
functions out of core.c instead.
Like *ksym*, BPF_CALL*, bpf_jit*, bpf_prog*.
I think resulting churn will be less.
imo it's more important to keep git blame history for interpreter
than for the other funcs.
Sounds like it's a fix that needs to be sent for the next RC ?
Please send a patch for bpf tree then.
Daniel, thoughts?
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-29 23:41 ` Alexei Starovoitov
@ 2020-04-30 0:13 ` Josh Poimboeuf
2020-04-30 2:10 ` Alexei Starovoitov
0 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2020-04-30 0:13 UTC (permalink / raw
To: Alexei Starovoitov
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 04:41:59PM -0700, Alexei Starovoitov wrote:
> On Wed, Apr 29, 2020 at 04:51:59PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jul 18, 2019 at 12:14:08PM -0700, tip-bot for Josh Poimboeuf wrote:
> > > Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
> > > Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
> > > Author: Josh Poimboeuf <jpoimboe@redhat.com>
> > > AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
> > > Committer: Thomas Gleixner <tglx@linutronix.de>
> > > CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
> > >
> > > bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
> >
> > For some reason, this
> >
> > __attribute__((optimize("-fno-gcse")))
> >
> > is disabling frame pointers in ___bpf_prog_run(). If you compile with
> > CONFIG_FRAME_POINTER it'll show something like:
> >
> > kernel/bpf/core.o: warning: objtool: ___bpf_prog_run.cold()+0x7: call without frame pointer save/setup
>
> you mean it started to disable frame pointers from some version of gcc?
> It wasn't doing this before, since objtool wasn't complaining, right?
> Sounds like gcc bug?
I actually think this warning has been around for a while. I just only
recently looked at it. I don't think anything changed in GCC, it's just
that almost nobody uses CONFIG_FRAME_POINTER, so it wasn't really
noticed.
> > Also, since GCC 9.1, the GCC docs say "The optimize attribute should be
> > used for debugging purposes only. It is not suitable in production
> > code." That doesn't sound too promising.
> >
> > So it seems like this commit should be reverted. But then we're back to
> > objtool being broken again in the RETPOLINE=n case, which means no ORC
> > coverage in this function. (See above commit for the details)
> >
> > Some ideas:
> >
> > - Skip objtool checking of that func/file (at least for RETPOLINE=n) --
> > but then it won't have ORC coverage.
> >
> > - Get rid of the "double goto" in ___bpf_prog_run(), which simplifies it
> > enough for objtool to understand -- but then the text explodes for
> > RETPOLINE=y.
>
> How that will look like?
> That could be the best option.
For example:
#define GOTO ({ goto *jumptable[insn->code]; })
and then replace all 'goto select_insn' with 'GOTO;'
The problem is that with RETPOLINE=y, the function text size grows from
5k to 7k, because for each of the 160+ retpoline JMPs, GCC (stupidly)
reloads the jump table register into a scratch register.
> > - Add -fno-gfcse to the Makefile for kernel/bpf/core.c -- but then that
> > affects the optimization of other functions in the file. However I
> > don't think the impact is significant.
> >
> > - Move ___bpf_prog_run() to its own file with the -fno-gfcse flag. I'm
> > thinking this could be the least bad option. Alexei?
>
> I think it would be easier to move some of the hot path
> functions out of core.c instead.
> Like *ksym*, BPF_CALL*, bpf_jit*, bpf_prog*.
> I think resulting churn will be less.
> imo it's more important to keep git blame history for interpreter
> than for the other funcs.
> Sounds like it's a fix that needs to be sent for the next RC ?
> Please send a patch for bpf tree then.
I can make a patch, what file would you recommend moving those hot path
functions to?
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-30 0:13 ` Josh Poimboeuf
@ 2020-04-30 2:10 ` Alexei Starovoitov
2020-04-30 3:53 ` Josh Poimboeuf
0 siblings, 1 reply; 60+ messages in thread
From: Alexei Starovoitov @ 2020-04-30 2:10 UTC (permalink / raw
To: Josh Poimboeuf
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 07:13:00PM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 29, 2020 at 04:41:59PM -0700, Alexei Starovoitov wrote:
> > On Wed, Apr 29, 2020 at 04:51:59PM -0500, Josh Poimboeuf wrote:
> > > On Thu, Jul 18, 2019 at 12:14:08PM -0700, tip-bot for Josh Poimboeuf wrote:
> > > > Commit-ID: 3193c0836f203a91bef96d88c64cccf0be090d9c
> > > > Gitweb: https://git.kernel.org/tip/3193c0836f203a91bef96d88c64cccf0be090d9c
> > > > Author: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > AuthorDate: Wed, 17 Jul 2019 20:36:45 -0500
> > > > Committer: Thomas Gleixner <tglx@linutronix.de>
> > > > CommitDate: Thu, 18 Jul 2019 21:01:06 +0200
> > > >
> > > > bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
> > >
> > > For some reason, this
> > >
> > > __attribute__((optimize("-fno-gcse")))
> > >
> > > is disabling frame pointers in ___bpf_prog_run(). If you compile with
> > > CONFIG_FRAME_POINTER it'll show something like:
> > >
> > > kernel/bpf/core.o: warning: objtool: ___bpf_prog_run.cold()+0x7: call without frame pointer save/setup
> >
> > you mean it started to disable frame pointers from some version of gcc?
> > It wasn't doing this before, since objtool wasn't complaining, right?
> > Sounds like gcc bug?
>
> I actually think this warning has been around for a while. I just only
> recently looked at it. I don't think anything changed in GCC, it's just
> that almost nobody uses CONFIG_FRAME_POINTER, so it wasn't really
> noticed.
>
> > > Also, since GCC 9.1, the GCC docs say "The optimize attribute should be
> > > used for debugging purposes only. It is not suitable in production
> > > code." That doesn't sound too promising.
> > >
> > > So it seems like this commit should be reverted. But then we're back to
> > > objtool being broken again in the RETPOLINE=n case, which means no ORC
> > > coverage in this function. (See above commit for the details)
> > >
> > > Some ideas:
> > >
> > > - Skip objtool checking of that func/file (at least for RETPOLINE=n) --
> > > but then it won't have ORC coverage.
> > >
> > > - Get rid of the "double goto" in ___bpf_prog_run(), which simplifies it
> > > enough for objtool to understand -- but then the text explodes for
> > > RETPOLINE=y.
> >
> > How that will look like?
> > That could be the best option.
>
> For example:
>
> #define GOTO ({ goto *jumptable[insn->code]; })
>
> and then replace all 'goto select_insn' with 'GOTO;'
>
> The problem is that with RETPOLINE=y, the function text size grows from
> 5k to 7k, because for each of the 160+ retpoline JMPs, GCC (stupidly)
> reloads the jump table register into a scratch register.
that would be a tiny change, right?
I'd rather go with that and gate it with 'ifdef CONFIG_FRAME_POINTER'
Like:
#ifndef CONFIG_FRAME_POINTER
#define CONT ({ insn++; goto select_insn; })
#define CONT_JMP ({ insn++; goto select_insn; })
#else
#define CONT ({ insn++; goto *jumptable[insn->code]; })
#define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
#endif
The reason this CONT and CONT_JMP macros are there because a combination
of different gcc versions together with different cpus make branch predictor
behave 'unpredictably'.
I've played with CONT and CONT_JMP either both doing direct goto or
indirect goto and observed quite different performance characteristics
from the interpreter.
What you see right now was the best tune I could get from a set of cpus
I had to play with and compilers. If I did the same tuning today the outcome
could have been different.
So I think it's totally fine to use above code. I think some cpus may actually
see performance gains with certain versions of gcc.
The retpoline text increase is unfortunate but when retpoline is on
other security knobs should be on too. In particular CONFIG_BPF_JIT_ALWAYS_ON
should be on as well. Which will remove interpreter from .text completely.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-30 2:10 ` Alexei Starovoitov
@ 2020-04-30 3:53 ` Josh Poimboeuf
2020-04-30 4:24 ` Alexei Starovoitov
0 siblings, 1 reply; 60+ messages in thread
From: Josh Poimboeuf @ 2020-04-30 3:53 UTC (permalink / raw
To: Alexei Starovoitov
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 07:10:52PM -0700, Alexei Starovoitov wrote:
> > For example:
> >
> > #define GOTO ({ goto *jumptable[insn->code]; })
> >
> > and then replace all 'goto select_insn' with 'GOTO;'
> >
> > The problem is that with RETPOLINE=y, the function text size grows from
> > 5k to 7k, because for each of the 160+ retpoline JMPs, GCC (stupidly)
> > reloads the jump table register into a scratch register.
>
> that would be a tiny change, right?
> I'd rather go with that and gate it with 'ifdef CONFIG_FRAME_POINTER'
> Like:
> #ifndef CONFIG_FRAME_POINTER
> #define CONT ({ insn++; goto select_insn; })
> #define CONT_JMP ({ insn++; goto select_insn; })
> #else
> #define CONT ({ insn++; goto *jumptable[insn->code]; })
> #define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
> #endif
>
> The reason this CONT and CONT_JMP macros are there because a combination
> of different gcc versions together with different cpus make branch predictor
> behave 'unpredictably'.
>
> I've played with CONT and CONT_JMP either both doing direct goto or
> indirect goto and observed quite different performance characteristics
> from the interpreter.
> What you see right now was the best tune I could get from a set of cpus
> I had to play with and compilers. If I did the same tuning today the outcome
> could have been different.
> So I think it's totally fine to use above code. I think some cpus may actually
> see performance gains with certain versions of gcc.
> The retpoline text increase is unfortunate but when retpoline is on
> other security knobs should be on too. In particular CONFIG_BPF_JIT_ALWAYS_ON
> should be on as well. Which will remove interpreter from .text completely.
This would actually be contingent on RETPOLINE, not FRAME_POINTER.
(FRAME_POINTER was the other issue with the "optimize" attribute, which
we're reverting so it'll no longer be a problem.)
So if you're not concerned about the retpoline text growth, it could be
as simple as:
#define CONT ({ insn++; goto *jumptable[insn->code]; })
#define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
Or, if you wanted to avoid the text growth, it could be:
#ifdef CONFIG_RETPOLINE
/*
* Avoid a 40% increase in function text size by getting GCC to generate a
* single retpoline jump instead of 160+.
*/
#define CONT ({ insn++; goto select_insn; })
#define CONT_JMP ({ insn++; goto select_insn; })
select_insn:
goto *jumptable[insn->code];
#else /* !CONFIG_RETPOLINE */
#define CONT ({ insn++; goto *jumptable[insn->code]; })
#define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
#endif /* CONFIG_RETPOLINE */
But since this is legacy path, I think the first one is much nicer.
Also, JMP_TAIL_CALL has a "goto select_insn", is it ok to convert that
to CONT?
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-30 3:53 ` Josh Poimboeuf
@ 2020-04-30 4:24 ` Alexei Starovoitov
2020-04-30 4:43 ` Josh Poimboeuf
0 siblings, 1 reply; 60+ messages in thread
From: Alexei Starovoitov @ 2020-04-30 4:24 UTC (permalink / raw
To: Josh Poimboeuf
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 10:53:15PM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 29, 2020 at 07:10:52PM -0700, Alexei Starovoitov wrote:
> > > For example:
> > >
> > > #define GOTO ({ goto *jumptable[insn->code]; })
> > >
> > > and then replace all 'goto select_insn' with 'GOTO;'
> > >
> > > The problem is that with RETPOLINE=y, the function text size grows from
> > > 5k to 7k, because for each of the 160+ retpoline JMPs, GCC (stupidly)
> > > reloads the jump table register into a scratch register.
> >
> > that would be a tiny change, right?
> > I'd rather go with that and gate it with 'ifdef CONFIG_FRAME_POINTER'
> > Like:
> > #ifndef CONFIG_FRAME_POINTER
> > #define CONT ({ insn++; goto select_insn; })
> > #define CONT_JMP ({ insn++; goto select_insn; })
> > #else
> > #define CONT ({ insn++; goto *jumptable[insn->code]; })
> > #define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
> > #endif
> >
> > The reason this CONT and CONT_JMP macros are there because a combination
> > of different gcc versions together with different cpus make branch predictor
> > behave 'unpredictably'.
> >
> > I've played with CONT and CONT_JMP either both doing direct goto or
> > indirect goto and observed quite different performance characteristics
> > from the interpreter.
> > What you see right now was the best tune I could get from a set of cpus
> > I had to play with and compilers. If I did the same tuning today the outcome
> > could have been different.
> > So I think it's totally fine to use above code. I think some cpus may actually
> > see performance gains with certain versions of gcc.
> > The retpoline text increase is unfortunate but when retpoline is on
> > other security knobs should be on too. In particular CONFIG_BPF_JIT_ALWAYS_ON
> > should be on as well. Which will remove interpreter from .text completely.
>
> This would actually be contingent on RETPOLINE, not FRAME_POINTER.
>
> (FRAME_POINTER was the other issue with the "optimize" attribute, which
> we're reverting so it'll no longer be a problem.)
>
> So if you're not concerned about the retpoline text growth, it could be
> as simple as:
>
> #define CONT ({ insn++; goto *jumptable[insn->code]; })
> #define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
>
>
> Or, if you wanted to avoid the text growth, it could be:
>
> #ifdef CONFIG_RETPOLINE
I'm a bit lost. So objtool is fine with the asm when retpoline is on?
Then pls do:
#if defined(CONFIG_RETPOLINE) || !defined(CONFIG_X86)
since there is no need to mess with other archs.
> /*
> * Avoid a 40% increase in function text size by getting GCC to generate a
> * single retpoline jump instead of 160+.
> */
> #define CONT ({ insn++; goto select_insn; })
> #define CONT_JMP ({ insn++; goto select_insn; })
>
> select_insn:
> goto *jumptable[insn->code];
>
> #else /* !CONFIG_RETPOLINE */
> #define CONT ({ insn++; goto *jumptable[insn->code]; })
> #define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
> #endif /* CONFIG_RETPOLINE */
>
>
> But since this is legacy path, I think the first one is much nicer.
>
>
> Also, JMP_TAIL_CALL has a "goto select_insn", is it ok to convert that
> to CONT?
yep
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: BPF vs objtool again
2020-04-30 4:24 ` Alexei Starovoitov
@ 2020-04-30 4:43 ` Josh Poimboeuf
0 siblings, 0 replies; 60+ messages in thread
From: Josh Poimboeuf @ 2020-04-30 4:43 UTC (permalink / raw
To: Alexei Starovoitov
Cc: x86, tglx, linux-kernel, mingo, hpa, ast, peterz, rdunlap,
Arnd Bergmann, bpf, daniel
On Wed, Apr 29, 2020 at 09:24:00PM -0700, Alexei Starovoitov wrote:
> > This would actually be contingent on RETPOLINE, not FRAME_POINTER.
> >
> > (FRAME_POINTER was the other issue with the "optimize" attribute, which
> > we're reverting so it'll no longer be a problem.)
> >
> > So if you're not concerned about the retpoline text growth, it could be
> > as simple as:
> >
> > #define CONT ({ insn++; goto *jumptable[insn->code]; })
> > #define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
> >
> >
> > Or, if you wanted to avoid the text growth, it could be:
> >
> > #ifdef CONFIG_RETPOLINE
>
> I'm a bit lost. So objtool is fine with the asm when retpoline is on?
Yeah, it's confusing... this has been quite an adventure with GCC.
Objtool is fine with the RETPOLINE double goto. It's only the
!RETPOLINE double goto which is the problem, because that triggers
more GCC weirdness (see 3193c0836f20).
> Then pls do:
> #if defined(CONFIG_RETPOLINE) || !defined(CONFIG_X86)
>
> since there is no need to mess with other archs.
Getting rid of select_insn altogether would make the code a lot simpler,
but it's your call. I'll make a patch soon.
--
Josh
^ permalink raw reply [flat|nested] 60+ messages in thread
end of thread, other threads:[~2020-04-30 4:43 UTC | newest]
Thread overview: 60+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-18 1:36 [PATCH v2 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
2019-07-18 19:07 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
2019-07-18 19:08 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 03/22] x86/kvm: Replace vmx_vmenter()'s call to kvm_spurious_fault() with UD2 Josh Poimboeuf
2019-07-18 8:17 ` Paolo Bonzini
2019-07-18 19:08 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
2019-07-18 8:22 ` Paolo Bonzini
2019-07-18 13:16 ` Sean Christopherson
2019-07-18 13:18 ` Paolo Bonzini
2019-07-18 14:12 ` Josh Poimboeuf
2019-07-18 14:13 ` Paolo Bonzini
2019-07-18 14:03 ` Josh Poimboeuf
2019-07-18 19:09 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
2019-07-18 19:10 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
2019-07-18 19:11 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
2019-07-18 19:11 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
2019-07-18 19:12 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
2019-07-18 19:13 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
2019-07-18 19:14 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2020-04-29 21:51 ` BPF vs objtool again Josh Poimboeuf
2020-04-29 22:01 ` Arvind Sankar
2020-04-29 23:41 ` Alexei Starovoitov
2020-04-30 0:13 ` Josh Poimboeuf
2020-04-30 2:10 ` Alexei Starovoitov
2020-04-30 3:53 ` Josh Poimboeuf
2020-04-30 4:24 ` Alexei Starovoitov
2020-04-30 4:43 ` Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
2019-07-18 19:14 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 12/22] objtool: Track original function across branches Josh Poimboeuf
2019-07-18 19:15 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 13/22] objtool: Refactor function alias logic Josh Poimboeuf
2019-07-18 19:16 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
2019-07-18 19:17 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
2019-07-18 19:17 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
2019-07-18 19:18 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
2019-07-18 19:19 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 18/22] objtool: Refactor jump table code Josh Poimboeuf
2019-07-18 19:20 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
2019-07-18 19:20 ` [tip:core/urgent] " tip-bot for Jann Horn
2019-07-18 1:36 ` [PATCH v2 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
2019-07-18 19:21 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 21/22] objtool: convert insn type to enum Josh Poimboeuf
2019-07-18 19:22 ` [tip:core/urgent] objtool: Convert " tip-bot for Josh Poimboeuf
2019-07-18 1:36 ` [PATCH v2 22/22] objtool: Support conditional retpolines Josh Poimboeuf
2019-07-18 19:23 ` [tip:core/urgent] " tip-bot for Josh Poimboeuf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.