Re: [PATCH v2] KVM/PPC Patch for KVM issue in real mode

[Balbir Singh <bsingharora@gmail.com>]

On 30/11/16 19:35, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> Some KVM functions for book3s_hv are called in real mode.
>> In real mode the top 4 bits of the address space are ignored,
>> hence an address beginning with 0xc0000000+offset is the
>> same as 0xd0000000+offset. The issue was observed when
>> a kvm memslot resolution lead to random values when
>> access from kvmppc_h_enter(). The issue is hit if the
>> KVM host is running with a page size of 4K, since
>> kvzalloc() looks at size < PAGE_SIZE. On systems with
>> 64K the issue is not observed easily, it largely depends
>> on the size of the structure being allocated.
>>
>> The proposed fix moves all KVM allocations for book3s_hv
>> to kzalloc() until all structures used in real mode are
>> audited. For safety allocations are moved to kmalloc
>> space. The impact is a large allocation on systems with
>> 4K page size.
> 
> We did such access using *real_vmalloc_addr(void *x). So you are
> suggesting here is we don't do that for all code path ?
> 

Yep.. that is true

> Do you have a stack dump for which you identified the issue ?
> 

I found it with kvm_memslots, don't have a stack dump, but
IIRC, I saw it with search_memslots <-- __gfn_to_memslot()

>>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>> ---
>>  Changelog v2:
>>    Fix build failures reported by the kbuild test robot
>>    http://www.spinics.net/lists/kvm/msg141727.html
>>
>>  arch/powerpc/include/asm/kvm_host.h | 19 +++++++++++++++++++
>>  include/linux/kvm_host.h            | 11 +++++++++++
>>  virt/kvm/kvm_main.c                 |  2 +-
>>  3 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>> index f15713a..53f5172 100644
>> --- a/arch/powerpc/include/asm/kvm_host.h
>> +++ b/arch/powerpc/include/asm/kvm_host.h
>> @@ -734,6 +734,25 @@ struct kvm_vcpu_arch {
>>  #define __KVM_HAVE_ARCH_WQP
>>  #define __KVM_HAVE_CREATE_DEVICE
>>  
>> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>> +#define __KVM_HAVE_ARCH_VZALLOC_OVERRIDE
> 
> do we need that OVERRIDE ? We usually have HAVE_ARCH_KVM_VZALLOC
> or just say #ifndef kvm_arch_vzalloc ?
> 

I can move __KVM_HAVE_ARCH_VZALLOC_OVERRIDE to
HAVE_ARCH_KVM_VZALLOC_OVERRIDE if it helps with clarity
and convention

Thanks for the review,
Balbir