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=-14.3 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_IN_DEF_DKIM_WL autolearn=unavailable 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 0A781C433E0 for ; Wed, 17 Jun 2020 16:36:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D758521532 for ; Wed, 17 Jun 2020 16:36:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="eawpKQDd" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726906AbgFQQgk (ORCPT ); Wed, 17 Jun 2020 12:36:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50866 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726540AbgFQQgj (ORCPT ); Wed, 17 Jun 2020 12:36:39 -0400 Received: from mail-vs1-xe43.google.com (mail-vs1-xe43.google.com [IPv6:2607:f8b0:4864:20::e43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A0A60C061755 for ; Wed, 17 Jun 2020 09:36:37 -0700 (PDT) Received: by mail-vs1-xe43.google.com with SMTP id y123so1758722vsb.6 for ; Wed, 17 Jun 2020 09:36:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=L8DsS6xxyGZ4X1I2BsfKE5F+wl5+Bc0v/Cbn7ErCR3k=; b=eawpKQDd4c5bCWEDiwcn6BhRKGQcd5albFMeNaZdFKCB6O3sFwD2XAZx9LvvOYi87t Zd0041rw93VndgUPf5lk4RuZlA8dXcSVTpu7VYahKPZdbqv05KKeJ7LnJttdizTtluT5 OYHgJdvNb8dH72kutypMPcXeLhyWIrRgMaS2iyuw/Lr6YD8+6DI8AvQtasmtlXDE9C7F FLi5DWCcWK7BKjDne7sZg/HbVdCiwosxxGZDthxyWacUQng42CxceziSSSXk8BO1AWeN BJDSiX2/zLffm0wAwppKnLGmoK/ndXvK8/qS5cg9IJUTWjiRKFGR6YFdm/zusqJzzUqC dKQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=L8DsS6xxyGZ4X1I2BsfKE5F+wl5+Bc0v/Cbn7ErCR3k=; b=Nz0FxdTwaZxmk13PNdmHJfHDzBY0Le+xEIfnyX9EFTJXhxYnjQB//dhkKqIHl5IQIb h6T8kNglzRm4xL+tIoU3EcKRvYR6Z6er/qDFHSejWjC9/X2F/Q0fYeJp5ib9YVrmw6Ar 3JXc7Ck+LR6mj1Mn21JS+afu9QuuIXQ6mwBdaL5vmb9UlTypgmwwHFryl4LV5TgVGpGU Z9BaqEXUq52eCSF2s2JpiMWqV7uXw2TD0XOyR9CvRkmE0ntqcDi/MoIm04giPwvl2gWm ZfFGwuswxluXR0Lz8LbEyVLGjTEpze8KRpt061oCxv5h9/+nRAl1ecRys5n0akprGwfN Pmfw== X-Gm-Message-State: AOAM532KdZ3kLU8vBom88pxcBm8i6q2fUj8+4kkUJdI3tCgksyuXppoQ DG86SmeWOPzOS9O7YbBy1ChiW8+4T8Zh5n29Y7I6nQ== X-Google-Smtp-Source: ABdhPJz7oaun07TMv+zayNnAIYVTbbeP778/fuG5Ll7911G7zeLD5BHbO9dbHYGezneDNySTtZXTodF2yXRXfDK1+ZE= X-Received: by 2002:a67:70c3:: with SMTP id l186mr6698034vsc.117.1592411796250; Wed, 17 Jun 2020 09:36:36 -0700 (PDT) MIME-Version: 1.0 References: <20200605213853.14959-1-sean.j.christopherson@intel.com> <20200605213853.14959-6-sean.j.christopherson@intel.com> <20200617005309.GA19540@linux.intel.com> In-Reply-To: <20200617005309.GA19540@linux.intel.com> From: Ben Gardon Date: Wed, 17 Jun 2020 09:36:25 -0700 Message-ID: Subject: Re: [PATCH 05/21] KVM: x86/mmu: Try to avoid crashing KVM if a MMU memory cache is empty To: Sean Christopherson Cc: Marc Zyngier , Paul Mackerras , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Julien Thierry , Suzuki K Poulose , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-kernel@vger.kernel.org, Peter Feiner , Peter Shier , Junaid Shahid , Christoffer Dall Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 16, 2020 at 5:53 PM Sean Christopherson wrote: > > On Wed, Jun 10, 2020 at 03:12:04PM -0700, Ben Gardon wrote: > > On Fri, Jun 5, 2020 at 2:39 PM Sean Christopherson > > wrote: > > > > > > Attempt to allocate a new object instead of crashing KVM (and likely the > > > kernel) if a memory cache is unexpectedly empty. Use GFP_ATOMIC for the > > > allocation as the caches are used while holding mmu_lock. The immediate > > > BUG_ON() makes the code unnecessarily explosive and led to confusing > > > minimums being used in the past, e.g. allocating 4 objects where 1 would > > > suffice. > > > > > > Signed-off-by: Sean Christopherson Reviewed-by: Ben Gardon > > > --- > > > arch/x86/kvm/mmu/mmu.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > > > index ba70de24a5b0..5e773564ab20 100644 > > > --- a/arch/x86/kvm/mmu/mmu.c > > > +++ b/arch/x86/kvm/mmu/mmu.c > > > @@ -1060,6 +1060,15 @@ static void walk_shadow_page_lockless_end(struct kvm_vcpu *vcpu) > > > local_irq_enable(); > > > } > > > > > > +static inline void *mmu_memory_cache_alloc_obj(struct kvm_mmu_memory_cache *mc, > > > + gfp_t gfp_flags) > > > +{ > > > + if (mc->kmem_cache) > > > + return kmem_cache_zalloc(mc->kmem_cache, gfp_flags); > > > + else > > > + return (void *)__get_free_page(gfp_flags); > > > +} > > > + > > > static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *mc, int min) > > > { > > > void *obj; > > > @@ -1067,10 +1076,7 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *mc, int min) > > > if (mc->nobjs >= min) > > > return 0; > > > while (mc->nobjs < ARRAY_SIZE(mc->objects)) { > > > - if (mc->kmem_cache) > > > - obj = kmem_cache_zalloc(mc->kmem_cache, GFP_KERNEL_ACCOUNT); > > > - else > > > - obj = (void *)__get_free_page(GFP_KERNEL_ACCOUNT); > > > + obj = mmu_memory_cache_alloc_obj(mc, GFP_KERNEL_ACCOUNT); > > > if (!obj) > > > return mc->nobjs >= min ? 0 : -ENOMEM; > > > mc->objects[mc->nobjs++] = obj; > > > @@ -1118,8 +1124,11 @@ static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc) > > > { > > > void *p; > > > > > > - BUG_ON(!mc->nobjs); > > > - p = mc->objects[--mc->nobjs]; > > > + if (WARN_ON(!mc->nobjs)) > > > + p = mmu_memory_cache_alloc_obj(mc, GFP_ATOMIC | __GFP_ACCOUNT); > > Is an atomic allocation really necessary here? In most cases, when > > topping up the memory cache we are handing a guest page fault. This > > bug could also be removed by returning null if unable to allocate from > > the cache, and then re-trying the page fault in that case. > > The whole point of these caches is to avoid having to deal with allocation > errors in the low level MMU paths, e.g. propagating an error up from > pte_list_add() would be a mess. > > > I don't know if this is necessary to handle other, non-x86 architectures more > > easily, but I worry this could cause some unpleasantness if combined with > > some other bug or the host was in a low memory situation and then this > > consumed the atomic pool. Perhaps this is a moot point since we log a warning > > and consider the atomic allocation something of an error. > > Yeah, it's the latter. If we reach this point it's a guaranteed KVM bug. > Because it's likely that the caller holds a lock, triggering the BUG_ON() > has a high chance of hanging the system. The idea is to take the path that > _may_ crash the kernel instead of killing the VM and more than likely > crashing the kernel. And hopefully this code will never be exercised on a > production kernel. That makes sense to me. I agree it's definitely positive to replace a BUG_ON with something else. > > > > + else > > > + p = mc->objects[--mc->nobjs]; > > > + BUG_ON(!p); > > > return p; > > > } > > > > > > -- > > > 2.26.0 > > >