* [PATCH 00/22] x86, objtool: several fixes/improvements
@ 2019-07-15 0:36 Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
` (23 more replies)
0 siblings, 24 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:36 UTC (permalink / raw)
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
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: Fix frame pointer usage in vmx_vmenter()
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 | 33 ++--
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 | 10 +-
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, 281 insertions(+), 234 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
@ 2019-07-15 0:36 ` Josh Poimboeuf
2019-07-15 4:58 ` Juergen Gross
2019-07-15 0:36 ` [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
` (22 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:36 UTC (permalink / raw)
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap, Juergen Gross,
Alok Kataria
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>
---
Cc: Juergen Gross <jgross@suse.com>
Cc: Alok Kataria <akataria@vmware.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] 50+ messages in thread
* [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
@ 2019-07-15 0:36 ` Josh Poimboeuf
2019-07-15 9:05 ` Paolo Bonzini
2019-07-15 0:36 ` [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter() Josh Poimboeuf
` (21 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0: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>
---
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] 50+ messages in thread
* [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
@ 2019-07-15 0:36 ` Josh Poimboeuf
2019-07-15 9:04 ` Paolo Bonzini
2019-07-15 0:36 ` [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
` (20 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0: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ář
With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
before calling kvm_spurious_fault().
Fixes the following warning:
arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
---
arch/x86/kvm/vmx/vmenter.S | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index d4cb1945b2e3..a2fabe3aaf9c 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -4,6 +4,7 @@
#include <asm/bitsperlong.h>
#include <asm/kvm_vcpu_regs.h>
#include <asm/nospec-branch.h>
+#include <asm/frame.h>
#define WORD_SIZE (BITS_PER_LONG / 8)
@@ -44,19 +45,22 @@
* to vmx_vmexit.
*/
ENTRY(vmx_vmenter)
+ FRAME_BEGIN
/* EFLAGS.ZF is set if VMCS.LAUNCHED == 0 */
je 2f
1: vmresume
- ret
+ jmp 4f
2: vmlaunch
- ret
+ jmp 4f
3: cmpb $0, kvm_rebooting
jne 4f
call kvm_spurious_fault
-4: ret
+
+4: FRAME_END
+ ret
.pushsection .fixup, "ax"
5: jmp 3b
--
2.20.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (2 preceding siblings ...)
2019-07-15 0:36 ` [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter() Josh Poimboeuf
@ 2019-07-15 0:36 ` Josh Poimboeuf
2019-07-15 9:07 ` Paolo Bonzini
2019-07-15 0:37 ` [PATCH 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
` (19 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0: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>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
---
arch/x86/include/asm/kvm_host.h | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0cc5b611a113..af7e18c05f98 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1499,22 +1499,25 @@ enum {
/*
* 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.
+ * If that happens, trap the fault and panic (unless we're rebooting).
*/
-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)
+asmlinkage void __noreturn kvm_spurious_fault(void);
+
+#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] 50+ messages in thread
* [PATCH 05/22] x86/entry: Fix thunk function ELF sizes
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (3 preceding siblings ...)
2019-07-15 0:36 ` [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
` (18 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (4 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
` (17 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (5 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-16 18:16 ` Nick Desaulniers
2019-07-15 0:37 ` [PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
` (16 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (6 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
` (15 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (7 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
` (14 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (8 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-16 18:15 ` Nick Desaulniers
2019-07-15 0:37 ` [PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
` (13 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (9 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 12/22] objtool: Track original function across branches Josh Poimboeuf
` (12 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 12/22] objtool: Track original function across branches
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (10 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 13/22] objtool: Refactor function alias logic Josh Poimboeuf
` (11 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 13/22] objtool: Refactor function alias logic
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (11 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 12/22] objtool: Track original function across branches Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
` (10 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 14/22] objtool: Warn on zero-length functions
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (12 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 13/22] objtool: Refactor function alias logic Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
` (9 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 15/22] objtool: Change dead_end_function() to return boolean
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (13 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
` (8 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 16/22] objtool: Do frame pointer check before dead end check
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (14 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
` (7 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 17/22] objtool: Refactor sibling call detection logic
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (15 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 18/22] objtool: Refactor jump table code Josh Poimboeuf
` (6 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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] 50+ messages in thread
* [PATCH 18/22] objtool: Refactor jump table code
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (16 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 9:38 ` Peter Zijlstra
2019-07-15 0:37 ` [PATCH 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
` (5 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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..b21e9f7768d0 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 switch 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] 50+ messages in thread
* [PATCH 19/22] objtool: Support repeated uses of the same C jump table
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (17 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 18/22] objtool: Refactor jump table code Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
` (4 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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 b21e9f7768d0..4ed7cb71a1d9 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] 50+ messages in thread
* [PATCH 20/22] objtool: Fix seg fault on bad switch table entry
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (18 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 17:24 ` Nick Desaulniers
2019-07-15 0:37 ` [PATCH 21/22] objtool: convert insn type to enum Josh Poimboeuf
` (3 subsequent siblings)
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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 4ed7cb71a1d9..716fe87e2c7d 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] 50+ messages in thread
* [PATCH 21/22] objtool: convert insn type to enum
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (19 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 22/22] objtool: Support conditional retpolines Josh Poimboeuf
` (2 subsequent siblings)
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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 716fe87e2c7d..5a237fc05cdc 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] 50+ messages in thread
* [PATCH 22/22] objtool: Support conditional retpolines
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (20 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 21/22] objtool: convert insn type to enum Josh Poimboeuf
@ 2019-07-15 0:37 ` Josh Poimboeuf
2019-07-15 9:52 ` [PATCH 00/22] x86, objtool: several fixes/improvements Peter Zijlstra
2019-07-15 19:38 ` Josh Poimboeuf
23 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 0:37 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>
---
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 5a237fc05cdc..a8411355a80d 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] 50+ messages in thread
* Re: [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes
2019-07-15 0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
@ 2019-07-15 4:58 ` Juergen Gross
2019-07-15 12:43 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Juergen Gross @ 2019-07-15 4:58 UTC (permalink / raw)
To: Josh Poimboeuf, x86
Cc: Arnd Bergmann, Jann Horn, Nick Desaulniers, Peter Zijlstra,
Randy Dunlap, Thomas Gleixner, linux-kernel, Alok Kataria
On 15.07.19 02:36, Josh Poimboeuf wrote:
> 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>
Juergen
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 0:36 ` [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter() Josh Poimboeuf
@ 2019-07-15 9:04 ` Paolo Bonzini
2019-07-15 12:37 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 9:04 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 15/07/19 02:36, Josh Poimboeuf wrote:
> With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
> before calling kvm_spurious_fault().
>
> Fixes the following warning:
>
> arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
This is not enough, because the RSP value must match what is computed at
this place:
/* Adjust RSP to account for the CALL to vmx_vmenter(). */
lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
call vmx_update_host_rsp
Is this important since kvm_spurious_fault is just BUG()? There is no
macro currently to support CONFIG_DEBUG_BUGVERBOSE in assembly code, but
it's also fine if you just change the call to ud2.
Paolo
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata
2019-07-15 0:36 ` [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
@ 2019-07-15 9:05 ` Paolo Bonzini
0 siblings, 0 replies; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 9:05 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 15/07/19 02:36, Josh Poimboeuf wrote:
> 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>
> ---
> 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;
>
> /*
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 0:36 ` [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
@ 2019-07-15 9:07 ` Paolo Bonzini
2019-07-15 12:40 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 9:07 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 15/07/19 02: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>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> ---
> arch/x86/include/asm/kvm_host.h | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 0cc5b611a113..af7e18c05f98 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1499,22 +1499,25 @@ enum {
> /*
> * 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.
> + * If that happens, trap the fault and panic (unless we're rebooting).
Not sure the comment is better than before, but apar from that
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks for the cleanups!
Paolo
> */
> -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)
> +asmlinkage void __noreturn kvm_spurious_fault(void);
> +
> +#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 [flat|nested] 50+ messages in thread
* Re: [PATCH 18/22] objtool: Refactor jump table code
2019-07-15 0:37 ` [PATCH 18/22] objtool: Refactor jump table code Josh Poimboeuf
@ 2019-07-15 9:38 ` Peter Zijlstra
0 siblings, 0 replies; 50+ messages in thread
From: Peter Zijlstra @ 2019-07-15 9:38 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
On Sun, Jul 14, 2019 at 07:37:13PM -0500, Josh Poimboeuf wrote:
> 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>
> ---
> 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..b21e9f7768d0 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 switch jump table.
s/switch// ?
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (21 preceding siblings ...)
2019-07-15 0:37 ` [PATCH 22/22] objtool: Support conditional retpolines Josh Poimboeuf
@ 2019-07-15 9:52 ` Peter Zijlstra
2019-07-15 19:38 ` Josh Poimboeuf
23 siblings, 0 replies; 50+ messages in thread
From: Peter Zijlstra @ 2019-07-15 9:52 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
On Sun, Jul 14, 2019 at 07:36:55PM -0500, Josh Poimboeuf wrote:
> 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: Fix frame pointer usage in vmx_vmenter()
> 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
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 9:04 ` Paolo Bonzini
@ 2019-07-15 12:37 ` Josh Poimboeuf
2019-07-15 13:03 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 12:37 UTC (permalink / raw)
To: Paolo Bonzini
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On Mon, Jul 15, 2019 at 11:04:03AM +0200, Paolo Bonzini wrote:
> On 15/07/19 02:36, Josh Poimboeuf wrote:
> > With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
> > before calling kvm_spurious_fault().
> >
> > Fixes the following warning:
> >
> > arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
> This is not enough, because the RSP value must match what is computed at
> this place:
>
> /* Adjust RSP to account for the CALL to vmx_vmenter(). */
> lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
> call vmx_update_host_rsp
Ah, that is surprising :-)
And then there's this, which overwrites the frame pointer anyway:
mov VCPU_RBP(%_ASM_AX), %_ASM_BP
Would it make sense to remove the call to vmx_vmenter() altogether, and
just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
from __vmx_vcpu_run()?
Then you could get rid of the RSP adjustment hack, and the
vmx_update_host_rsp() function altogether.
> Is this important since kvm_spurious_fault is just BUG()?
It's probably only important if you care about the stack trace for the
BUG() case. But BP is clobbered anyway so I guess it doesn't matter.
> There is no macro currently to support CONFIG_DEBUG_BUGVERBOSE in
> assembly code, but it's also fine if you just change the call to ud2.
That would be one way to make objtool happy.
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 9:07 ` Paolo Bonzini
@ 2019-07-15 12:40 ` Josh Poimboeuf
2019-07-15 13:05 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 12:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On Mon, Jul 15, 2019 at 11:07:28AM +0200, Paolo Bonzini wrote:
> On 15/07/19 02: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>
> > ---
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > arch/x86/include/asm/kvm_host.h | 33 ++++++++++++++++++---------------
> > 1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 0cc5b611a113..af7e18c05f98 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1499,22 +1499,25 @@ enum {
> > /*
> > * 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.
> > + * If that happens, trap the fault and panic (unless we're rebooting).
>
> Not sure the comment is better than before, but apar from that
The previous comment didn't seem to match the code, since we only ignore
the instruction if we're rebooting.
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks!
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes
2019-07-15 4:58 ` Juergen Gross
@ 2019-07-15 12:43 ` Josh Poimboeuf
0 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 12:43 UTC (permalink / raw)
To: Juergen Gross
Cc: x86, Arnd Bergmann, Jann Horn, Nick Desaulniers, Peter Zijlstra,
Randy Dunlap, Thomas Gleixner, linux-kernel
On Mon, Jul 15, 2019 at 06:58:36AM +0200, Juergen Gross wrote:
> On 15.07.19 02:36, Josh Poimboeuf wrote:
> > 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>
Thanks!
BTW, Alok's email address from the MAINTAINERS file seems to be bouncing.
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 12:37 ` Josh Poimboeuf
@ 2019-07-15 13:03 ` Paolo Bonzini
2019-07-15 13:35 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 13:03 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 15/07/19 14:37, Josh Poimboeuf wrote:
> On Mon, Jul 15, 2019 at 11:04:03AM +0200, Paolo Bonzini wrote:
>> On 15/07/19 02:36, Josh Poimboeuf wrote:
>>> With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
>>> before calling kvm_spurious_fault().
>>>
>>> Fixes the following warning:
>>>
>>> arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>
>> This is not enough, because the RSP value must match what is computed at
>> this place:
>>
>> /* Adjust RSP to account for the CALL to vmx_vmenter(). */
>> lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
>> call vmx_update_host_rsp
>
> Ah, that is surprising :-)
>
> And then there's this, which overwrites the frame pointer anyway:
>
> mov VCPU_RBP(%_ASM_AX), %_ASM_BP
>
> Would it make sense to remove the call to vmx_vmenter() altogether, and
> just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
> from __vmx_vcpu_run()?
Unfortunately there's another use of it in nested_vmx_check_vmentry_hw.
The problem is that storing RSP (no matter if adjusted or not) needs a
scratch register. And vmx_vmenter is exactly the part of the vmentry
that doesn't have any scratch registers available.
Therefore, you must arrange for the caller to store RSP. You cannot for
example remove it from __vmx_vcpu_run and nested_vmx_check_vmentry_hw in
favor of vmx_vmenter. :(
>> Is this important since kvm_spurious_fault is just BUG()?
>
> It's probably only important if you care about the stack trace for the
> BUG() case. But BP is clobbered anyway so I guess it doesn't matter.
Yes, BP is the guest RBP at this point of the code.
Paolo
>> There is no macro currently to support CONFIG_DEBUG_BUGVERBOSE in
>> assembly code, but it's also fine if you just change the call to ud2.
>
> That would be one way to make objtool happy.
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 12:40 ` Josh Poimboeuf
@ 2019-07-15 13:05 ` Paolo Bonzini
2019-07-15 13:25 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 13:05 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 15/07/19 14:40, Josh Poimboeuf wrote:
>>> * 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.
>>> + * If that happens, trap the fault and panic (unless we're rebooting).
>> Not sure the comment is better than before, but apar from that
> The previous comment didn't seem to match the code, since we only ignore
> the instruction if we're rebooting.
>
"If that happens" refers to "a reboot turns off virtualization while
processes are running". Perhaps
* Usually after catching the fault we just panic; during reboot
* instead the instruction is ignored.
Paolo
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 13:05 ` Paolo Bonzini
@ 2019-07-15 13:25 ` Josh Poimboeuf
2019-07-15 18:16 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 13:25 UTC (permalink / raw)
To: Paolo Bonzini
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On Mon, Jul 15, 2019 at 03:05:33PM +0200, Paolo Bonzini wrote:
> On 15/07/19 14:40, Josh Poimboeuf wrote:
> >>> * 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.
> >>> + * If that happens, trap the fault and panic (unless we're rebooting).
> >> Not sure the comment is better than before, but apar from that
> > The previous comment didn't seem to match the code, since we only ignore
> > the instruction if we're rebooting.
> >
>
> "If that happens" refers to "a reboot turns off virtualization while
> processes are running".
Ah, makes sense now. I was reading "if that happens" to mean the fault.
> * Usually after catching the fault we just panic; during reboot
> * instead the instruction is ignored.
Yes, that's much clearer. Assuming you meant to replace the entire
comment. I also moved it to directly above the macro it's describing:
asmlinkage void __noreturn kvm_spurious_fault(void);
/*
* Usually after catching the fault we just panic; during reboot
* instead the instruction is ignored.
*/
#define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 13:03 ` Paolo Bonzini
@ 2019-07-15 13:35 ` Josh Poimboeuf
2019-07-15 18:17 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 13:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On Mon, Jul 15, 2019 at 03:03:23PM +0200, Paolo Bonzini wrote:
> On 15/07/19 14:37, Josh Poimboeuf wrote:
> > On Mon, Jul 15, 2019 at 11:04:03AM +0200, Paolo Bonzini wrote:
> >> On 15/07/19 02:36, Josh Poimboeuf wrote:
> >>> With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
> >>> before calling kvm_spurious_fault().
> >>>
> >>> Fixes the following warning:
> >>>
> >>> arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
> >>>
> >>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >>
> >> This is not enough, because the RSP value must match what is computed at
> >> this place:
> >>
> >> /* Adjust RSP to account for the CALL to vmx_vmenter(). */
> >> lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
> >> call vmx_update_host_rsp
> >
> > Ah, that is surprising :-)
> >
> > And then there's this, which overwrites the frame pointer anyway:
> >
> > mov VCPU_RBP(%_ASM_AX), %_ASM_BP
> >
> > Would it make sense to remove the call to vmx_vmenter() altogether, and
> > just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
> > from __vmx_vcpu_run()?
>
> Unfortunately there's another use of it in nested_vmx_check_vmentry_hw.
Ah, I missed that too (failed by cscope). Is this ok?
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] 50+ messages in thread
* Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry
2019-07-15 0:37 ` [PATCH 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
@ 2019-07-15 17:24 ` Nick Desaulniers
2019-07-15 17:29 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-15 17:24 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> 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.
So this came from
drivers/hwmon/pmbus/adm1275.c ?
Any special configuration?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry
2019-07-15 17:24 ` Nick Desaulniers
@ 2019-07-15 17:29 ` Josh Poimboeuf
2019-07-18 23:02 ` Nick Desaulniers
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 17:29 UTC (permalink / raw)
To: Nick Desaulniers
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Mon, Jul 15, 2019 at 10:24:24AM -0700, Nick Desaulniers wrote:
> On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > 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.
>
> So this came from
> drivers/hwmon/pmbus/adm1275.c ?
> Any special configuration?
Arnd shared the config and the .o file here:
https://lkml.kernel.org/r/CAK8P3a2beBPP+KX4zTfSfFPwo+7ksWZLqZzpP9BJ80iPecg3zA@mail.gmail.com
I used Arnd's .o file for testing.
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup
2019-07-15 13:25 ` Josh Poimboeuf
@ 2019-07-15 18:16 ` Paolo Bonzini
0 siblings, 0 replies; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 18:16 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 15/07/19 15:25, Josh Poimboeuf wrote:
> On Mon, Jul 15, 2019 at 03:05:33PM +0200, Paolo Bonzini wrote:
>> On 15/07/19 14:40, Josh Poimboeuf wrote:
>>>>> * 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.
>>>>> + * If that happens, trap the fault and panic (unless we're rebooting).
>>>> Not sure the comment is better than before, but apar from that
>>> The previous comment didn't seem to match the code, since we only ignore
>>> the instruction if we're rebooting.
>>>
>>
>> "If that happens" refers to "a reboot turns off virtualization while
>> processes are running".
>
> Ah, makes sense now. I was reading "if that happens" to mean the fault.
>
>> * Usually after catching the fault we just panic; during reboot
>> * instead the instruction is ignored.
>
> Yes, that's much clearer. Assuming you meant to replace the entire
> comment.
No, I didn't. :) I meant only the last line (otherwise it removes
information on why the fault may happen and the simplest choice is to
ignore). Thanks for taking care of this!
Paolo
I also moved it to directly above the macro it's describing:
>
>
> asmlinkage void __noreturn kvm_spurious_fault(void);
>
> /*
> * Usually after catching the fault we just panic; during reboot
> * instead the instruction is ignored.
> */
> #define ____kvm_handle_fault_on_reboot(insn, cleanup_insn) \
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
2019-07-15 13:35 ` Josh Poimboeuf
@ 2019-07-15 18:17 ` Paolo Bonzini
0 siblings, 0 replies; 50+ messages in thread
From: Paolo Bonzini @ 2019-07-15 18:17 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner,
Nick Desaulniers, Arnd Bergmann, Jann Horn, Randy Dunlap,
Radim Krčmář
On 15/07/19 15:35, Josh Poimboeuf wrote:
> On Mon, Jul 15, 2019 at 03:03:23PM +0200, Paolo Bonzini wrote:
>> On 15/07/19 14:37, Josh Poimboeuf wrote:
>>> Would it make sense to remove the call to vmx_vmenter() altogether, and
>>> just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
>>> from __vmx_vcpu_run()?
>>
>> Unfortunately there's another use of it in nested_vmx_check_vmentry_hw.
>
> Ah, I missed that too (failed by cscope). Is this ok?
Yes, it is. Thanks!
Paolo
> 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 [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
` (22 preceding siblings ...)
2019-07-15 9:52 ` [PATCH 00/22] x86, objtool: several fixes/improvements Peter Zijlstra
@ 2019-07-15 19:38 ` Josh Poimboeuf
2019-07-15 21:45 ` Nick Desaulniers
23 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-15 19:38 UTC (permalink / raw)
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Nick Desaulniers,
Arnd Bergmann, Jann Horn, Randy Dunlap
On Sun, Jul 14, 2019 at 07:36:55PM -0500, Josh Poimboeuf wrote:
> 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.
These patches are also at:
git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git objtool-many-fixes
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-15 19:38 ` Josh Poimboeuf
@ 2019-07-15 21:45 ` Nick Desaulniers
2019-07-16 23:17 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-15 21:45 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Mon, Jul 15, 2019 at 12:38 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Sun, Jul 14, 2019 at 07:36:55PM -0500, Josh Poimboeuf wrote:
> > 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
For a defconfig, that's the only issue I see.
(Note that I just landed https://reviews.llvm.org/rL366130 for fixing
up bugs from loop unrolling loops containing asm goto with Clang, so
anyone else testing w/ clang will see fewer objtool warnings with that
patch applied. A follow up is being worked on in
https://reviews.llvm.org/D64101).
For allmodconfig:
arch/x86/ia32/ia32_signal.o: warning: objtool:
ia32_setup_rt_frame()+0x247: call to memset() with UACCESS enabled
mm/kasan/common.o: warning: objtool: kasan_report()+0x52: call to
__stack_chk_fail() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool:
x32_setup_rt_frame()+0x255: call to memset() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: __setup_rt_frame()+0x254:
call to memset() with UACCESS enabled
drivers/ata/sata_dwc_460ex.o: warning: objtool:
sata_dwc_bmdma_start_by_tag()+0x3a0: can't find switch jump table
lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x88:
call to memset() with UACCESS enabled
lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x610:
call to __stack_chk_fail() with UACCESS enabled
lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x88:
call to memset() with UACCESS enabled
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool:
.altinstr_replacement+0x56: redundant UACCESS disable
Without your series, I see them anyways, so I don't consider them
regressions added by this series. Let's follow up on these maybe in a
new thread? (Shall I send you these object files?)
So for the series:
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > 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.
Thanks for the report, let's follow up on steps for me to reproduce.
> > These patches are also at:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git objtool-many-fixes
Are these the same patches? Some of the commit messages look different, like:
https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/commit/?h=objtool-many-fixes&id=3e39561c52c4f0062207d604c972148b7b60c341
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
2019-07-15 0:37 ` [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
@ 2019-07-16 18:15 ` Nick Desaulniers
2019-07-16 23:02 ` Josh Poimboeuf
0 siblings, 1 reply; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-16 18:15 UTC (permalink / raw)
To: Josh Poimboeuf, Miguel Ojeda
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap, Alexei Starovoitov, Daniel Borkmann
On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> 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>
> ---
> 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")))
+ Miguel, maintainer of compiler_attributes.h
I wonder if the optimize attributes can be feature detected?
Is -fno-gcse supported all the way back to GCC 4.6?
> 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
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
2019-07-15 0:37 ` [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
@ 2019-07-16 18:16 ` Nick Desaulniers
2019-07-16 18:51 ` Peter Zijlstra
0 siblings, 1 reply; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-16 18:16 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> 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).
What is CLAC and STAC?
>
> Signed-off-by: Josh Poimboeuf <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
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()
2019-07-16 18:16 ` Nick Desaulniers
@ 2019-07-16 18:51 ` Peter Zijlstra
0 siblings, 0 replies; 50+ messages in thread
From: Peter Zijlstra @ 2019-07-16 18:51 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Josh Poimboeuf, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Thomas Gleixner, Arnd Bergmann, Jann Horn, Randy Dunlap
On Tue, Jul 16, 2019 at 11:16:48AM -0700, Nick Desaulniers wrote:
> On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > 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).
>
> What is CLAC and STAC?
CLear AC flag and SeT AC flag, SMAP repurposed the EFLAGS.AC for CPL0.
Also see commit: ea24213d8088 ("objtool: Add UACCESS validation")
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()
2019-07-16 18:15 ` Nick Desaulniers
@ 2019-07-16 23:02 ` Josh Poimboeuf
0 siblings, 0 replies; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-16 23:02 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Miguel Ojeda, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap, Alexei Starovoitov, Daniel Borkmann
On Tue, Jul 16, 2019 at 11:15:54AM -0700, Nick Desaulniers wrote:
> On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > 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>
> > ---
> > 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")))
>
> + Miguel, maintainer of compiler_attributes.h
> I wonder if the optimize attributes can be feature detected?
> Is -fno-gcse supported all the way back to GCC 4.6?
Yeah, from snooping in the GCC tree it looks like it's been around
for 18+ years.
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-15 21:45 ` Nick Desaulniers
@ 2019-07-16 23:17 ` Josh Poimboeuf
2019-07-18 22:26 ` Nick Desaulniers
0 siblings, 1 reply; 50+ messages in thread
From: Josh Poimboeuf @ 2019-07-16 23:17 UTC (permalink / raw)
To: Nick Desaulniers
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Mon, Jul 15, 2019 at 02:45:39PM -0700, Nick Desaulniers wrote:
> For a defconfig, that's the only issue I see.
> (Note that I just landed https://reviews.llvm.org/rL366130 for fixing
> up bugs from loop unrolling loops containing asm goto with Clang, so
> anyone else testing w/ clang will see fewer objtool warnings with that
> patch applied. A follow up is being worked on in
> https://reviews.llvm.org/D64101).
>
> For allmodconfig:
> arch/x86/ia32/ia32_signal.o: warning: objtool:
> ia32_setup_rt_frame()+0x247: call to memset() with UACCESS enabled
> mm/kasan/common.o: warning: objtool: kasan_report()+0x52: call to
> __stack_chk_fail() with UACCESS enabled
> arch/x86/kernel/signal.o: warning: objtool:
> x32_setup_rt_frame()+0x255: call to memset() with UACCESS enabled
> arch/x86/kernel/signal.o: warning: objtool: __setup_rt_frame()+0x254:
> call to memset() with UACCESS enabled
> drivers/ata/sata_dwc_460ex.o: warning: objtool:
> sata_dwc_bmdma_start_by_tag()+0x3a0: can't find switch jump table
> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x88:
> call to memset() with UACCESS enabled
> lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x610:
> call to __stack_chk_fail() with UACCESS enabled
> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x88:
> call to memset() with UACCESS enabled
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool:
> .altinstr_replacement+0x56: redundant UACCESS disable
>
> Without your series, I see them anyways, so I don't consider them
> regressions added by this series. Let's follow up on these maybe in a
> new thread? (Shall I send you these object files?)
Yes, maybe open a new thread and be sure to copy PeterZ. He loves those
warnings ;-) Object files are definitely needed.
> So for the series:
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Thanks!
>
> > >
> > > 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.
>
> Thanks for the report, let's follow up on steps for me to reproduce.
Just to clarify, there are two clang issues. Both of them were reported
originally by Arnd, IIRC.
1) The one described above and in patch 20, where the switch table is
mostly unused entries. Not a real bug, but it's a bit sloppy and
wasteful, and objtool doesn't know how to interpret it.
2) The bug with the noreturn call site having a different stack size
depending on which code path was taken.
> > > These patches are also at:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git objtool-many-fixes
>
> Are these the same patches? Some of the commit messages look different, like:
> https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/commit/?h=objtool-many-fixes&id=3e39561c52c4f0062207d604c972148b7b60c341
Oops, those extra 3 commits weren't supposed to be there. That's future
work. I dropped them from the branch.
--
Josh
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-16 23:17 ` Josh Poimboeuf
@ 2019-07-18 22:26 ` Nick Desaulniers
2019-09-27 20:24 ` Nick Desaulniers
0 siblings, 1 reply; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-18 22:26 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Tue, Jul 16, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Jul 15, 2019 at 02:45:39PM -0700, Nick Desaulniers wrote:
> > For a defconfig, that's the only issue I see.
> > (Note that I just landed https://reviews.llvm.org/rL366130 for fixing
> > up bugs from loop unrolling loops containing asm goto with Clang, so
> > anyone else testing w/ clang will see fewer objtool warnings with that
> > patch applied. A follow up is being worked on in
> > https://reviews.llvm.org/D64101).
> >
> > For allmodconfig:
> > arch/x86/ia32/ia32_signal.o: warning: objtool:
> > ia32_setup_rt_frame()+0x247: call to memset() with UACCESS enabled
> > mm/kasan/common.o: warning: objtool: kasan_report()+0x52: call to
> > __stack_chk_fail() with UACCESS enabled
> > arch/x86/kernel/signal.o: warning: objtool:
> > x32_setup_rt_frame()+0x255: call to memset() with UACCESS enabled
> > arch/x86/kernel/signal.o: warning: objtool: __setup_rt_frame()+0x254:
> > call to memset() with UACCESS enabled
> > drivers/ata/sata_dwc_460ex.o: warning: objtool:
> > sata_dwc_bmdma_start_by_tag()+0x3a0: can't find switch jump table
> > lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x88:
> > call to memset() with UACCESS enabled
> > lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x610:
> > call to __stack_chk_fail() with UACCESS enabled
> > lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x88:
> > call to memset() with UACCESS enabled
> > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool:
> > .altinstr_replacement+0x56: redundant UACCESS disable
> >
> > Without your series, I see them anyways, so I don't consider them
> > regressions added by this series. Let's follow up on these maybe in a
> > new thread? (Shall I send you these object files?)
>
> Yes, maybe open a new thread and be sure to copy PeterZ. He loves those
> warnings ;-) Object files are definitely needed.
>
> > So for the series:
> > Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Thanks!
>
> >
> > > >
> > > > 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.
> >
> > Thanks for the report, let's follow up on steps for me to reproduce.
>
> Just to clarify, there are two clang issues. Both of them were reported
> originally by Arnd, IIRC.
>
> 1) The one described above and in patch 20, where the switch table is
> mostly unused entries. Not a real bug, but it's a bit sloppy and
> wasteful, and objtool doesn't know how to interpret it.
Thanks for the concise reports. Will follow up on these in:
https://github.com/ClangBuiltLinux/linux/issues/611
>
> 2) The bug with the noreturn call site having a different stack size
> depending on which code path was taken.
and:
https://github.com/ClangBuiltLinux/linux/issues/612
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry
2019-07-15 17:29 ` Josh Poimboeuf
@ 2019-07-18 23:02 ` Nick Desaulniers
0 siblings, 0 replies; 50+ messages in thread
From: Nick Desaulniers @ 2019-07-18 23:02 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Mon, Jul 15, 2019 at 10:29 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Jul 15, 2019 at 10:24:24AM -0700, Nick Desaulniers wrote:
> > On Sun, Jul 14, 2019 at 5:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > 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.
> >
> > So this came from
> > drivers/hwmon/pmbus/adm1275.c ?
$ grep switch drivers/hwmon/pmbus/adm1275.c | wc -l
8
$ grep switch drivers/hwmon/pmbus/adm1275.c
switch (reg) {
switch (reg) {
switch (reg) {
switch (data->id) {
switch (config & ADM1075_IRANGE_MASK) {
switch (config & (ADM1275_VRANGE | ADM1272_IRANGE)) {
switch (config & ADM1293_VIN_SEL_MASK) {
switch (config & ADM1293_IRANGE_MASK) {
Looking specifically at the definition of adm1275_probe, I see:
...
switch (data->id) {
...
switch (config & ADM1075_IRANGE_MASK) {
...
switch (config & (ADM1275_VRANGE | ADM1272_IRANGE)) {
...
switch (config & ADM1293_VIN_SEL_MASK) {
...
switch (config & ADM1293_IRANGE_MASK) {
So I assume that the two level switch statement is somehow related.
Maybe the two level switch is transformed into a one level switch with
duplicated case labels? Let me play around in <strikethrough>my
sandbox</strikethrough>godbolt and see if I can reproduce with that
pattern.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 00/22] x86, objtool: several fixes/improvements
2019-07-18 22:26 ` Nick Desaulniers
@ 2019-09-27 20:24 ` Nick Desaulniers
0 siblings, 0 replies; 50+ messages in thread
From: Nick Desaulniers @ 2019-09-27 20:24 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
LKML, Peter Zijlstra, Thomas Gleixner, Arnd Bergmann, Jann Horn,
Randy Dunlap
On Thu, Jul 18, 2019 at 3:26 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Jul 16, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > > > > 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.
> > >
> > > Thanks for the report, let's follow up on steps for me to reproduce.
> >
> > Just to clarify, there are two clang issues. Both of them were reported
> > originally by Arnd, IIRC.
> >
> > 1) The one described above and in patch 20, where the switch table is
> > mostly unused entries. Not a real bug, but it's a bit sloppy and
> > wasteful, and objtool doesn't know how to interpret it.
>
> Thanks for the concise reports. Will follow up on these in:
> https://github.com/ClangBuiltLinux/linux/issues/611
Following up on this one; in one of the test cases we determined that
the default destination of an exhaustive switch wasn't getting cleaned
up properly, and is being fixed in:
https://reviews.llvm.org/D68131
https://bugs.llvm.org/show_bug.cgi?id=43129
I'm not sure that was the precise issue you described, or if there's
more than one bug here, but hopefully it will help.
>
> >
> > 2) The bug with the noreturn call site having a different stack size
> > depending on which code path was taken.
>
> and:
> https://github.com/ClangBuiltLinux/linux/issues/612
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2019-09-27 20:25 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
2019-07-15 4:58 ` Juergen Gross
2019-07-15 12:43 ` Josh Poimboeuf
2019-07-15 0:36 ` [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
2019-07-15 9:05 ` Paolo Bonzini
2019-07-15 0:36 ` [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter() Josh Poimboeuf
2019-07-15 9:04 ` Paolo Bonzini
2019-07-15 12:37 ` Josh Poimboeuf
2019-07-15 13:03 ` Paolo Bonzini
2019-07-15 13:35 ` Josh Poimboeuf
2019-07-15 18:17 ` Paolo Bonzini
2019-07-15 0:36 ` [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
2019-07-15 9:07 ` Paolo Bonzini
2019-07-15 12:40 ` Josh Poimboeuf
2019-07-15 13:05 ` Paolo Bonzini
2019-07-15 13:25 ` Josh Poimboeuf
2019-07-15 18:16 ` Paolo Bonzini
2019-07-15 0:37 ` [PATCH 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
2019-07-16 18:16 ` Nick Desaulniers
2019-07-16 18:51 ` Peter Zijlstra
2019-07-15 0:37 ` [PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
2019-07-16 18:15 ` Nick Desaulniers
2019-07-16 23:02 ` Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 12/22] objtool: Track original function across branches Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 13/22] objtool: Refactor function alias logic Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 18/22] objtool: Refactor jump table code Josh Poimboeuf
2019-07-15 9:38 ` Peter Zijlstra
2019-07-15 0:37 ` [PATCH 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
2019-07-15 17:24 ` Nick Desaulniers
2019-07-15 17:29 ` Josh Poimboeuf
2019-07-18 23:02 ` Nick Desaulniers
2019-07-15 0:37 ` [PATCH 21/22] objtool: convert insn type to enum Josh Poimboeuf
2019-07-15 0:37 ` [PATCH 22/22] objtool: Support conditional retpolines Josh Poimboeuf
2019-07-15 9:52 ` [PATCH 00/22] x86, objtool: several fixes/improvements Peter Zijlstra
2019-07-15 19:38 ` Josh Poimboeuf
2019-07-15 21:45 ` Nick Desaulniers
2019-07-16 23:17 ` Josh Poimboeuf
2019-07-18 22:26 ` Nick Desaulniers
2019-09-27 20:24 ` Nick Desaulniers
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.