[PATCH 00/22] x86, objtool: several fixes/improvements

[PATCH 00/22] x86, objtool: several fixes/improvements

[Josh Poimboeuf]

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

[PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes

[Josh Poimboeuf]

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

Re: [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes

[Juergen Gross]

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

Re: [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes

[Josh Poimboeuf]

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

[PATCH 02/22] x86/kvm: Fix fastop function ELF metadata

[Josh Poimboeuf]

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

Re: [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata

[Paolo Bonzini]

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>

[PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Josh Poimboeuf]

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

Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Paolo Bonzini]

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

Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Josh Poimboeuf]

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

Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Paolo Bonzini]

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.
> 

Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Josh Poimboeuf]

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

Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

[Paolo Bonzini]

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
> 

[PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Josh Poimboeuf]

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

Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Paolo Bonzini]

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, "")
> 

Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Josh Poimboeuf]

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

Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Paolo Bonzini]

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

Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Josh Poimboeuf]

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

Re: [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup

[Paolo Bonzini]

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)		\
> 
> 

[PATCH 05/22] x86/entry: Fix thunk function ELF sizes

[Josh Poimboeuf]

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

[PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable

[Josh Poimboeuf]

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

[PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()

[Josh Poimboeuf]

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

Re: [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()

[Nick Desaulniers]

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

Re: [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail()

[Peter Zijlstra]

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")

[PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail()

[Josh Poimboeuf]

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

[PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths

[Josh Poimboeuf]

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

[PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()

[Josh Poimboeuf]

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

Re: [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()

[Nick Desaulniers]

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

Re: [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()

[Josh Poimboeuf]

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

[PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list

[Josh Poimboeuf]

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

[PATCH 12/22] objtool: Track original function across branches

[Josh Poimboeuf]

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

[PATCH 13/22] objtool: Refactor function alias logic

[Josh Poimboeuf]

- 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

[PATCH 14/22] objtool: Warn on zero-length functions

[Josh Poimboeuf]

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

[PATCH 15/22] objtool: Change dead_end_function() to return boolean

[Josh Poimboeuf]

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

[PATCH 16/22] objtool: Do frame pointer check before dead end check

[Josh Poimboeuf]

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

[PATCH 17/22] objtool: Refactor sibling call detection logic

[Josh Poimboeuf]

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

[PATCH 18/22] objtool: Refactor jump table code

[Josh Poimboeuf]

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

Re: [PATCH 18/22] objtool: Refactor jump table code

[Peter Zijlstra]

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// ?

[PATCH 19/22] objtool: Support repeated uses of the same C jump table

[Josh Poimboeuf]

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

[PATCH 20/22] objtool: Fix seg fault on bad switch table entry

[Josh Poimboeuf]

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

Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry

[Nick Desaulniers]

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

Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry

[Josh Poimboeuf]

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

Re: [PATCH 20/22] objtool: Fix seg fault on bad switch table entry

[Nick Desaulniers]

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

[PATCH 21/22] objtool: convert insn type to enum

[Josh Poimboeuf]

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

[PATCH 22/22] objtool: Support conditional retpolines

[Josh Poimboeuf]

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

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Peter Zijlstra]

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>

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Josh Poimboeuf]

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

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Nick Desaulniers]

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

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Josh Poimboeuf]

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

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Nick Desaulniers]

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

Re: [PATCH 00/22] x86, objtool: several fixes/improvements

[Nick Desaulniers]

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