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=-12.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 57A5AC433B4 for ; Wed, 28 Apr 2021 23:42:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2E22161408 for ; Wed, 28 Apr 2021 23:42:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230245AbhD1Xnl (ORCPT ); Wed, 28 Apr 2021 19:43:41 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:27341 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229488AbhD1Xnj (ORCPT ); Wed, 28 Apr 2021 19:43:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619653374; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WCGTGrknZeE7eCqq8B0KPNhvevrJ02XQwZZ1Z+q/y2w=; b=UHg8RRDPkpVtEfaUubvOaZSNPNxbnlw9WXZuXbGlzrs5FyL48ILq/pLsQIZgM+vkRRrlQh f92jZ9v/O7qYdc/ysXj0F6C8uhZfB8rajj6Lg0UEct+ml3ZR/YzHUCuAkCpo0slltUrkLw Xq/YKw6qZY8/MlHrQlVT6jpMBvJ0eRs= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-50-EHauhDVCNjiWrUqn3b9KAw-1; Wed, 28 Apr 2021 19:42:52 -0400 X-MC-Unique: EHauhDVCNjiWrUqn3b9KAw-1 Received: by mail-ej1-f71.google.com with SMTP id x21-20020a1709064bd5b029037c44cb861cso12903799ejv.4 for ; Wed, 28 Apr 2021 16:42:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WCGTGrknZeE7eCqq8B0KPNhvevrJ02XQwZZ1Z+q/y2w=; b=E6yOUPoELyMIiBbjjgRBFs59UssJmh/ygqhdBy2dtZRg6Y71lIYywY0xZu1+X2NYXG 6gxPzGpbOnChcj+exFhy69mraKjpiwnaEyPj2Q5UzokSnEDXeC6VDJrkQs6HzWueq05+ cxFk54fYU5IcFoXAe4HFjY2qEMq5NehtZfo/lj/esZcyIH9MD9aRE67XFBpyptwFfx0a MlxaSDSKkxCiG+yJJA+ZBy3gzmN2cNFV6JuSobVwheagAExuxdM0kEjM3NBJ8RkaH6tn BXJyJmv+vQs2W4mYGsSZOTOjBHE8atU+QcEhxCwoRI/mSTnJJyVwVMWju5ASlJm8ddvd bIBg== X-Gm-Message-State: AOAM533tpIUMgQXMwTxN8lv0H2Nk+McioVQDpjB1cegkx1TOvzkLdUsC B0sCX6NkcswKkF5hBqYbIvShlNOiVUarox4tFpsopDgG+sV/7fvwghCL9UOWh1GMtc88eOvY7FP wIyJ6SoLygvHf53i/jyAmETU3 X-Received: by 2002:a17:906:2e17:: with SMTP id n23mr4703497eji.266.1619653370834; Wed, 28 Apr 2021 16:42:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwvq4EViWh9nuBMUH/uXQuJHoWkOTGTEPkDOPHSwlXpSrpAi3mHmdhuz3EwJdYuJevA/tJwUA== X-Received: by 2002:a17:906:2e17:: with SMTP id n23mr4703476eji.266.1619653370672; Wed, 28 Apr 2021 16:42:50 -0700 (PDT) Received: from ?IPv6:2001:b07:6468:f312:63a7:c72e:ea0e:6045? ([2001:b07:6468:f312:63a7:c72e:ea0e:6045]) by smtp.gmail.com with ESMTPSA id i19sm710063ejd.114.2021.04.28.16.42.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Apr 2021 16:42:50 -0700 (PDT) To: Ben Gardon Cc: LKML , kvm , Peter Xu , Sean Christopherson , Peter Shier , Junaid Shahid , Jim Mattson , Yulei Zhang , Wanpeng Li , Vitaly Kuznetsov , Xiao Guangrong References: <20210427223635.2711774-1-bgardon@google.com> <20210427223635.2711774-6-bgardon@google.com> <997f9fe3-847b-8216-c629-1ad5fdd2ffae@redhat.com> <5b4a0c30-118c-da1f-281c-130438a1c833@redhat.com> From: Paolo Bonzini Subject: Re: [PATCH 5/6] KVM: x86/mmu: Protect kvm->memslots with a mutex Message-ID: <16b2f0f3-c9a8-c455-fff0-231c2fe04a8e@redhat.com> Date: Thu, 29 Apr 2021 01:42:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/04/21 23:46, Ben Gardon wrote: > On Wed, Apr 28, 2021 at 2:41 PM Paolo Bonzini wrote: >> >> On 28/04/21 22:40, Ben Gardon wrote: >>> ... However with the locking you propose below, we might still run >>> into issues on a move or delete, which would mean we'd still need the >>> separate memory allocation for the rmaps array. Or we do some >>> shenanigans where we try to copy the rmap pointers from the other set >>> of memslots. >> >> If that's (almost) as easy as passing old to >> kvm_arch_prepare_memory_region, that would be totally okay. > > Unfortunately it's not quite that easy because it's all the slots > _besides_ the one being modified where we'd need to copy the rmaps. Ah, now I understand the whole race. And it seems to me that if one kvm_dup_memslots within the new lock fixed a bug, two kvm_dup_memslots within the new lock are going to fix two bugs. :) Seriously: unless I'm missing another case (it's late here...), it's not ugly and it's still relatively easy to explain. diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2799c6660cce..48929dd5fb29 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1270,7 +1270,7 @@ static int check_memory_region_flags(const struct kvm_userspace_memory_region *m return 0; } -static struct kvm_memslots *install_new_memslots(struct kvm *kvm, +static void install_new_memslots(struct kvm *kvm, int as_id, struct kvm_memslots *slots) { struct kvm_memslots *old_memslots = __kvm_memslots(kvm, as_id); @@ -1280,7 +1280,9 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm, slots->generation = gen | KVM_MEMSLOT_GEN_UPDATE_IN_PROGRESS; rcu_assign_pointer(kvm->memslots[as_id], slots); + mutex_unlock(&kvm->slots_arch_lock); synchronize_srcu_expedited(&kvm->srcu); + kvfree(old_memslots); /* * Increment the new memslot generation a second time, dropping the @@ -1302,8 +1304,6 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm, kvm_arch_memslots_updated(kvm, gen); slots->generation = gen; - - return old_memslots; } /* @@ -1342,6 +1342,7 @@ static int kvm_set_memslot(struct kvm *kvm, struct kvm_memslots *slots; int r; + mutex_lock(&kvm->slots_arch_lock); slots = kvm_dup_memslots(__kvm_memslots(kvm, as_id), change); if (!slots) return -ENOMEM; @@ -1353,14 +1354,7 @@ static int kvm_set_memslot(struct kvm *kvm, */ slot = id_to_memslot(slots, old->id); slot->flags |= KVM_MEMSLOT_INVALID; - - /* - * We can re-use the old memslots, the only difference from the - * newly installed memslots is the invalid flag, which will get - * dropped by update_memslots anyway. We'll also revert to the - * old memslots if preparing the new memory region fails. - */ - slots = install_new_memslots(kvm, as_id, slots); + install_new_memslots(kvm, as_id, slots); /* From this point no new shadow pages pointing to a deleted, * or moved, memslot will be created. @@ -1370,6 +1364,9 @@ static int kvm_set_memslot(struct kvm *kvm, * - kvm_is_visible_gfn (mmu_check_root) */ kvm_arch_flush_shadow_memslot(kvm, slot); + + mutex_lock(&kvm->slots_arch_lock); + slots = kvm_dup_memslots(__kvm_memslots(kvm, as_id), change); } r = kvm_arch_prepare_memory_region(kvm, new, mem, change); @@ -1377,16 +1374,17 @@ static int kvm_set_memslot(struct kvm *kvm, goto out_slots; update_memslots(slots, new, change); - slots = install_new_memslots(kvm, as_id, slots); + install_new_memslots(kvm, as_id, slots); kvm_arch_commit_memory_region(kvm, mem, old, new, change); - - kvfree(slots); return 0; out_slots: - if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) + if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) { + slot = id_to_memslot(slots, old->id); + slot->flags &= ~KVM_MEMSLOT_INVALID; slots = install_new_memslots(kvm, as_id, slots); + } kvfree(slots); return r; } One could optimize things a bit by reusing the allocation and only doing a memcpy from the new memslots array to the old one under the slots_arch_lock. (Plus the above still lacks a mutex_init and should be split in two patches, with the mutex going in the second; but you get the idea and code sometimes is easier than words). Paolo