Re: KVM overflows the stack

[Avi Kivity <avi@qumranet.com>]

Dave Hansen wrote:
> On Wed, 2008-07-16 at 14:44 -0700, Dave Hansen wrote:
>   
>> On a suggestion of Anthony's, I tried a defconfig kernel.
>>
>> It is now bombing out on an assertion in the lapic code:
>>
>> 	http://sr71.net/~dave/linux/2.6.26-oops1.txt
>>     
>
> I think I found it!!!
>
> $ (objdump -d kvm.ko ; objdump -d kvm-intel.ko ) | egrep 'sub.*0x...,.*esp|>:'  | egrep sub -B1
> 00001a90 <kvm_vcpu_ioctl>:
>     1a9a:	81 ec 60 06 00 00    	sub    $0x660,%esp
> --
> 00004e90 <kvm_arch_vcpu_ioctl>:
>     4e9d:	81 ec 6c 08 00 00    	sub    $0x86c,%esp
> --
> 00005900 <kvm_arch_vm_ioctl>:
>     5903:	81 ec 34 05 00 00    	sub    $0x534,%esp
> --
> 0000d4f0 <paging64_prefetch_page>:
>     d4f8:	81 ec 1c 01 00 00    	sub    $0x11c,%esp
> --
> 0000dfd0 <paging32_prefetch_page>:
>     dfd8:	81 ec 1c 01 00 00    	sub    $0x11c,%esp
> --
> 0000f390 <kvm_pv_mmu_op>:
>     f3a1:	81 ec 28 02 00 00    	sub    $0x228,%esp
>
> We're simply overflowing the stack.  I changed all of the large on-stack
> allocations to 'static', and it actually boots now.  I know 'static'
> isn't safe, but it was good for a quick test.
>
>   

Yes!   It's obvious, once you know it...

> A 'make stackcheck' confirms this:
>
> dave@nimitz:~/kernels/linux-2.6.git$ make checkstack
> objdump -d vmlinux $(find . -name '*.ko') | \
> 	perl /home/dave/kernels/linux-2.6.git-t61/scripts/checkstack.pl i386
> 0x000042d3 kvm_arch_vcpu_ioctl [kvm]:			2148
> 0x000012e3 kvm_vcpu_ioctl [kvm]:			1620
> 0x00004a83 kvm_arch_vm_ioctl [kvm]:			1332
> 0x00009a26 airo_get_aplist [airo]:			1140
> 0x00009b76 airo_get_aplist [airo]:			1140
> 0x00009c82 airo_get_aplist [airo]:			1140
> ...
>
> In other words, kvm has the top 3 stack users in my kernel.  As you can
> see from my trace above, these things also get called with super-long
> stacks already.  Man.  That sucked to find.
>
> Avi, how would you like this fixed?  I'd be happy to prepare some
> patches.  Do you have a particular approach that you think we should
> use?  Just make the big objects dynamically allocated?
>   

Yes, things like kvm_lapic_state are way too big to be on the stack.  
There's an additional problem here, that apparently your gcc (which 
version?) doesn't fold objects in a switch statement into the same stack 
slot:

switch (...) {
    case x: {
         struct medium a;
         ...
    }
    case y:
          struct medium b;
          ...
    }
};

These could be solved either by stack allocation, or by moving into 
functions marked noinline.  Whichever is easier.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.