On Tue, Jun 16, 2020 at 5:53 PM Sean Christopherson <sean.j.christopherson@xxxxxxxxx> 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 > > <sean.j.christopherson@xxxxxxxxx> 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 <sean.j.christopherson@xxxxxxxxx> Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx> > > > --- > > > 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 > > > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm