| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1e9c77a9-adec-0a2d-5483-70cb2332d529@redhat.com>
Date: Mon, 3 May 2021 15:29:14 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Ben Gardon <bgardon@...gle.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: Peter Xu <peterx@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Peter Shier <pshier@...gle.com>,
Junaid Shahid <junaids@...gle.com>,
Jim Mattson <jmattson@...gle.com>,
Yulei Zhang <yulei.kernel@...il.com>,
Wanpeng Li <kernellwp@...il.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Xiao Guangrong <xiaoguangrong.eric@...il.com>
Subject: Re: [PATCH v2 6/7] KVM: mmu: Add slots_arch_lock for memslot arch
fields
On 29/04/21 23:18, Ben Gardon wrote:
> Add a new lock to protect the arch-specific fields of memslots if they
> need to be modified in a kvm->srcu read critical section. A future
> commit will use this lock to lazily allocate memslot rmaps for x86.
Here there should be a blurb about the possible races that can happen
and why we decided for the slots_arch_lock.
> Signed-off-by: Ben Gardon <bgardon@...gle.com>
> ---
> include/linux/kvm_host.h | 9 +++++++++
> virt/kvm/kvm_main.c | 31 ++++++++++++++++++++++++++-----
> 2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 8895b95b6a22..2d5e797fbb08 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -472,6 +472,15 @@ struct kvm {
> #endif /* KVM_HAVE_MMU_RWLOCK */
>
> struct mutex slots_lock;
> +
> + /*
> + * Protects the arch-specific fields of struct kvm_memory_slots in
> + * use by the VM. To be used under the slots_lock (above) or in a
> + * kvm->srcu read cirtical section where acquiring the slots_lock
> + * would lead to deadlock with the synchronize_srcu in
> + * install_new_memslots.
> + */
I think usage under slots_lock need not be mentioned here. More like this:
/*
* Protects the arch-specific fields of struct kvm_memory_slots
* in use by the VM. Usually these are initialized by
* kvm_arch_prepare_memory_region and then protected by
* kvm->srcu; however, if they need to be initialized outside
* kvm_arch_prepare_memory_region, slots_arch_lock can
* be used instead as it is also held when calling
* kvm_arch_prepare_memory_region itself. Note that using
* slots_lock would lead to deadlock with install_new_memslots,
* because it is held during synchronize_srcu:
*
* idx = srcu_read_lock(&kvm->srcu);
* mutex_lock(&kvm->slots_lock);
* mutex_lock(&kvm->slots_lock);
* synchronize_srcu(&kvm->srcu);
*/
(Though a better place for this is in
Documentation/virtual/kvm/locking.rst).
Paolo
> + struct mutex slots_arch_lock;
> struct mm_struct *mm; /* userspace tied to this vm */
> struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];
> struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c8010f55e368..97b03fa2d0c8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -908,6 +908,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> mutex_init(&kvm->lock);
> mutex_init(&kvm->irq_lock);
> mutex_init(&kvm->slots_lock);
> + mutex_init(&kvm->slots_arch_lock);
> INIT_LIST_HEAD(&kvm->devices);
>
> BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
> @@ -1280,6 +1281,10 @@ 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);
> +
> + /* Acquired in kvm_set_memslot. */
> + mutex_unlock(&kvm->slots_arch_lock);
> +
> synchronize_srcu_expedited(&kvm->srcu);
>
> /*
> @@ -1351,6 +1356,9 @@ static int kvm_set_memslot(struct kvm *kvm,
> struct kvm_memslots *slots;
> int r;
>
> + /* Released in install_new_memslots. */
> + mutex_lock(&kvm->slots_arch_lock);
> +
> slots = kvm_dup_memslots(__kvm_memslots(kvm, as_id), change);
> if (!slots)
> return -ENOMEM;
> @@ -1364,10 +1372,9 @@ static int kvm_set_memslot(struct kvm *kvm,
> 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.
> + * We can re-use the memory from the old memslots.
> + * It will be overwritten with a copy of the new memslots
> + * after reacquiring the slots_arch_lock below.
> */
> slots = install_new_memslots(kvm, as_id, slots);
>
> @@ -1379,6 +1386,17 @@ static int kvm_set_memslot(struct kvm *kvm,
> * - kvm_is_visible_gfn (mmu_check_root)
> */
> kvm_arch_flush_shadow_memslot(kvm, slot);
> +
> + /* Released in install_new_memslots. */
> + mutex_lock(&kvm->slots_arch_lock);
> +
> + /*
> + * The arch-specific fields of the memslots could have changed
> + * between releasing the slots_arch_lock in
> + * install_new_memslots and here, so get a fresh copy of the
> + * slots.
> + */
> + kvm_copy_memslots(__kvm_memslots(kvm, as_id), slots);
> }
>
> r = kvm_arch_prepare_memory_region(kvm, new, mem, change);
> @@ -1394,8 +1412,11 @@ static int kvm_set_memslot(struct kvm *kvm,
> 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;
> }
>
Powered by blists - more mailing lists