Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Tue, Jan 15, 2019 at 08:13:22AM +0100, Paolo Bonzini wrote:
> On 15/01/19 08:04, Qian Cai wrote:
> > 
> > 
> > On 1/15/19 1:44 AM, Qian Cai wrote:
> >> compilation warning since v5.0-rc1,
> >>
> >> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
> >> call without frame pointer save/setup

The warning is complaining about vmx_vcpu_run() in vmx.c, not vmenter.S.
The rule being "broken" is that a call is made without creating a stack
frame, and vmx_vmenter() obviously makes no calls.

E.g., manually running objtool check:

    $ tools/objtool/objtool check arch/x86/kvm/vmx/vmenter.o
    $ tools/objtool/objtool check arch/x86/kvm/vmx/vmx.o
    arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.19()+0x83e: call without frame pointer save/setup

I put "broken" in quotes because AFAICT we're not actually violating the
rule.  From tools/objtool/Documentation/stack-validation.txt:

   If it's a GCC-compiled .c file, the error may be because the function
   uses an inline asm() statement which has a "call" instruction.  An
   asm() statement with a call instruction must declare the use of the
   stack pointer in its output operand.  On x86_64, this means adding
   the ASM_CALL_CONSTRAINT as an output constraint:

     asm volatile("call func" : ASM_CALL_CONSTRAINT);

   Otherwise the stack frame may not get created before the call.


The asm() blob that calls vmx_vmenter() uses ASM_CALL_CONSTRAINT, and
the resulting asm output generates a frame pointer, e.g. this is from
the vmx.o that objtool warns on:

Dump of assembler code for function vmx_vcpu_run:
   0x0000000000007440 <+0>:     e8 00 00 00 00  callq  0x7445 <vmx_vcpu_run+5>
   0x0000000000007445 <+5>:     55      push   %rbp
   0x0000000000007446 <+6>:     48 89 e5        mov    %rsp,%rbp


The warning only shows up in certain configs, e.g. I was only able to
reproduce this using the .config provided by lkp.  Even explicitly
enabling CONFIG_FRAME_POINTERS and CONFIG_STACK_VALIDATION didn't
trigger the warning using my usual config.

And all that being said, I'm pretty sure this isn't related to the call
to vmx_vmenter() at all, but rather is something that was exposed by
removing __noclone from vmx_vcpu_run().

E.g. I still get the warning if I comment out the call to vmx_vmenter,
it just shifts to something else (and continues to shift I comment out
more calls).  The warning goes away if I re-add __noclone, regardless
of whether or not commit 2bcbd406715d ("Revert "compiler-gcc: disable
-ftracer for __noclone functions"") is applied.

0x6659 is in vmx_vcpu_run (./arch/x86/include/asm/spec-ctrl.h:43).
38       * Avoids writing to the MSR if the content/bits are the same
39       */
40      static inline
41      void x86_spec_ctrl_restore_host(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl)
42      {
43              x86_virt_spec_ctrl(guest_spec_ctrl, guest_virt_spec_ctrl, false);
44      }
45
46      /* AMD specific Speculative Store Bypass MSR data */
47      extern u64 x86_amd_ls_cfg_base;

What's really baffling is that we explicitly tell objtool to not do stack
metadata validation on vmx_vcpu_run():

	STACK_FRAME_NON_STANDARD(vmx_vcpu_run);

As an aside, we might be able to remove STACK_FRAME_NON_STANDARD, assuming
we get this warning sorted out.

> >> Fixes: 453eafbe65f (KVM: VMX: Move VM-Enter + VM-Exit handling to
> >> non-inline sub-routines)
> > 
> > Oops, wrong fix. Back to square one.
> > 
> 
> Hmm, maybe like this:
> 
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index bcef2c7e9bc4..33122fa9d4bd 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
>  	ret
> 
>  2:	vmlaunch
> +3:
>  	ret
> 
> -3:	cmpb $0, kvm_rebooting
> -	jne 4f
> -	call kvm_spurious_fault
> -4:	ret
> -
>  	.pushsection .fixup, "ax"
> -5:	jmp 3b
> +4:	cmpb $0, kvm_rebooting
> +	jne 3b
> +	jmp kvm_spurious_fault
>  	.popsection
> 
> -	_ASM_EXTABLE(1b, 5b)
> -	_ASM_EXTABLE(2b, 5b)
> +	_ASM_EXTABLE(1b, 4b)
> +	_ASM_EXTABLE(2b, 4b)
> 
>  ENDPROC(vmx_vmenter)
> 
> 
> Paolo