From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1AA36C282DA for ; Tue, 9 Apr 2019 14:20:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DE5062133D for ; Tue, 9 Apr 2019 14:20:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726464AbfDIOUM (ORCPT ); Tue, 9 Apr 2019 10:20:12 -0400 Received: from mga11.intel.com ([192.55.52.93]:4301 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726412AbfDIOUM (ORCPT ); Tue, 9 Apr 2019 10:20:12 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Apr 2019 07:20:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,329,1549958400"; d="scan'208";a="160089113" Received: from sjchrist-coffee.jf.intel.com (HELO linux.intel.com) ([10.54.74.181]) by fmsmga004.fm.intel.com with ESMTP; 09 Apr 2019 07:20:08 -0700 Date: Tue, 9 Apr 2019 07:20:08 -0700 From: Sean Christopherson To: Ben Gardon Cc: Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Peter Feiner , Junaid Shahid , kvm@vger.kernel.org Subject: Re: [PATCH v2] kvm: mmu: Fix overflow on kvm mmu page limit calculation Message-ID: <20190409142002.GA23061@linux.intel.com> References: <20190408180730.226619-1-bgardon@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190408180730.226619-1-bgardon@google.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org 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 > --- > 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 >