On Mon, Apr 08, 2019 at 11:07:30AM -0700, Ben Gardon wrote:
> KVM bases its memory usage limits on the total number of guest pages
> across all memslots. However, those limits, and the calculations to
> produce, them use 32 bit unsigned integers. This can result in overflow
Nit: I think you wanted the comma after "them".
> if a VM has more guest pages that can be represented by a u32. As a
> result of this overflow, KVM can use a low limit on the number of MMU
> pages it will allocate. This makes KVM unable to map all of guest memory
> at once, prompting spurious faults.
>
> Tested: Ran all kvm-unit-tests on an Intel Haswell machine. This patch
> introduced no new failures.
>
> Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 12 ++++++------
> arch/x86/kvm/mmu.c | 13 ++++++-------
> arch/x86/kvm/mmu.h | 2 +-
> arch/x86/kvm/x86.c | 4 ++--
> 4 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 159b5988292f3..9b7b731a00321 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -126,7 +126,7 @@ static inline gfn_t gfn_to_index(gfn_t gfn, gfn_t base_gfn, int level)
> }
>
> #define KVM_PERMILLE_MMU_PAGES 20
> -#define KVM_MIN_ALLOC_MMU_PAGES 64
> +#define KVM_MIN_ALLOC_MMU_PAGES 64UL
> #define KVM_MMU_HASH_SHIFT 12
> #define KVM_NUM_MMU_PAGES (1 << KVM_MMU_HASH_SHIFT)
> #define KVM_MIN_FREE_MMU_PAGES 5
> @@ -844,9 +844,9 @@ enum kvm_irqchip_mode {
> };
>
> struct kvm_arch {
> - unsigned int n_used_mmu_pages;
> - unsigned int n_requested_mmu_pages;
> - unsigned int n_max_mmu_pages;
> + unsigned long n_used_mmu_pages;
> + unsigned long n_requested_mmu_pages;
> + unsigned long n_max_mmu_pages;
> unsigned int indirect_shadow_pages;
It probably makes sense to change 'indirect_shadow_pages' as well. I
haven't done the math to know whether or not it can actually overflow,
but 4 bytes per VM seems cheap compared to a bug that causes KVM to think
it doesn't have any shadow pages. Note, there's also a local variable
in reexecute_instruction() that snapshots 'indirect_shadow_pages'.
Another case that should be changed to an unsigned long is 'lpages' in
kvm_arch_create_memslot(). Overflow there seems inevitable, and it's a
local so there's no memory overhead.
> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> /*
> @@ -1256,8 +1256,8 @@ void kvm_mmu_clear_dirty_pt_masked(struct kvm *kvm,
> gfn_t gfn_offset, unsigned long mask);
> void kvm_mmu_zap_all(struct kvm *kvm);
> void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, u64 gen);
> -unsigned int kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm);
> -void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
> +unsigned long kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm);
> +void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned long kvm_nr_mmu_pages);
>
> int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3);
> bool pdptrs_changed(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index eee455a8a612d..bd07270196744 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2007,7 +2007,7 @@ static int is_empty_shadow_page(u64 *spt)
> * aggregate version in order to make the slab shrinker
> * faster
> */
> -static inline void kvm_mod_used_mmu_pages(struct kvm *kvm, int nr)
> +static inline void kvm_mod_used_mmu_pages(struct kvm *kvm, unsigned long nr)
> {
> kvm->arch.n_used_mmu_pages += nr;
> percpu_counter_add(&kvm_total_used_mmu_pages, nr);
> @@ -2763,7 +2763,7 @@ static bool prepare_zap_oldest_mmu_page(struct kvm *kvm,
> * Changing the number of mmu pages allocated to the vm
> * Note: if goal_nr_mmu_pages is too small, you will get dead lock
> */
> -void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int goal_nr_mmu_pages)
> +void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned long goal_nr_mmu_pages)
> {
> LIST_HEAD(invalid_list);
>
> @@ -6031,10 +6031,10 @@ int kvm_mmu_module_init(void)
> /*
> * Calculate mmu pages needed for kvm.
> */
> -unsigned int kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm)
> +unsigned long kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm)
> {
> - unsigned int nr_mmu_pages;
> - unsigned int nr_pages = 0;
> + unsigned long nr_mmu_pages;
> + unsigned long nr_pages = 0;
> struct kvm_memslots *slots;
> struct kvm_memory_slot *memslot;
> int i;
> @@ -6047,8 +6047,7 @@ unsigned int kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm)
> }
>
> nr_mmu_pages = nr_pages * KVM_PERMILLE_MMU_PAGES / 1000;
> - nr_mmu_pages = max(nr_mmu_pages,
> - (unsigned int) KVM_MIN_ALLOC_MMU_PAGES);
> + nr_mmu_pages = max(nr_mmu_pages, KVM_MIN_ALLOC_MMU_PAGES);
>
> return nr_mmu_pages;
> }
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index bbdc60f2fae89..54c2a377795be 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -64,7 +64,7 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
> int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
> u64 fault_address, char *insn, int insn_len);
>
> -static inline unsigned int kvm_mmu_available_pages(struct kvm *kvm)
> +static inline unsigned long kvm_mmu_available_pages(struct kvm *kvm)
> {
> if (kvm->arch.n_max_mmu_pages > kvm->arch.n_used_mmu_pages)
> return kvm->arch.n_max_mmu_pages -
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 099b851dabafd..455f156f56ede 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4270,7 +4270,7 @@ static int kvm_vm_ioctl_set_identity_map_addr(struct kvm *kvm,
> }
>
> static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
> - u32 kvm_nr_mmu_pages)
> + unsigned long kvm_nr_mmu_pages)
> {
> if (kvm_nr_mmu_pages < KVM_MIN_ALLOC_MMU_PAGES)
> return -EINVAL;
> @@ -4284,7 +4284,7 @@ static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
> return 0;
> }
>
> -static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> +static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> {
> return kvm->arch.n_max_mmu_pages;
> }
> --
> 2.21.0.392.gf8f6787159e-goog
>
