The rule inside kvm enforces that the vcpu->mutex is taken *inside*
kvm->lock. The rule is violated by the pkvm_create_hyp_vm which acquires
the kvm->lock while already holding the vcpu->mutex lock from
kvm_vcpu_ioctl. Follow the rule by taking the config lock while getting the
VM handle and make sure that this is cleaned on VM destroy under the
same lock.
Signed-off-by: Sebastian Ene <[email protected]>
Cc: [email protected]
---
arch/arm64/kvm/pkvm.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
index 8350fb8fee0b..b7be96a53597 100644
--- a/arch/arm64/kvm/pkvm.c
+++ b/arch/arm64/kvm/pkvm.c
@@ -101,6 +101,17 @@ void __init kvm_hyp_reserve(void)
hyp_mem_base);
}
+static void __pkvm_destroy_hyp_vm(struct kvm *host_kvm)
+{
+ if (host_kvm->arch.pkvm.handle) {
+ WARN_ON(kvm_call_hyp_nvhe(__pkvm_teardown_vm,
+ host_kvm->arch.pkvm.handle));
+ }
+
+ host_kvm->arch.pkvm.handle = 0;
+ free_hyp_memcache(&host_kvm->arch.pkvm.teardown_mc);
+}
+
/*
* Allocates and donates memory for hypervisor VM structs at EL2.
*
@@ -181,7 +192,7 @@ static int __pkvm_create_hyp_vm(struct kvm *host_kvm)
return 0;
destroy_vm:
- pkvm_destroy_hyp_vm(host_kvm);
+ __pkvm_destroy_hyp_vm(host_kvm);
return ret;
free_vm:
free_pages_exact(hyp_vm, hyp_vm_sz);
@@ -194,23 +205,19 @@ int pkvm_create_hyp_vm(struct kvm *host_kvm)
{
int ret = 0;
- mutex_lock(&host_kvm->lock);
+ mutex_lock(&host_kvm->arch.config_lock);
if (!host_kvm->arch.pkvm.handle)
ret = __pkvm_create_hyp_vm(host_kvm);
- mutex_unlock(&host_kvm->lock);
+ mutex_unlock(&host_kvm->arch.config_lock);
return ret;
}
void pkvm_destroy_hyp_vm(struct kvm *host_kvm)
{
- if (host_kvm->arch.pkvm.handle) {
- WARN_ON(kvm_call_hyp_nvhe(__pkvm_teardown_vm,
- host_kvm->arch.pkvm.handle));
- }
-
- host_kvm->arch.pkvm.handle = 0;
- free_hyp_memcache(&host_kvm->arch.pkvm.teardown_mc);
+ mutex_lock(&host_kvm->arch.config_lock);
+ __pkvm_destroy_hyp_vm(host_kvm);
+ mutex_unlock(&host_kvm->arch.config_lock);
}
int pkvm_init_host_vm(struct kvm *host_kvm)
--
2.43.0.429.g432eaa2c6b-goog
On Tue, Jan 23, 2024 at 04:48:19PM +0000, Sebastian Ene wrote:
> The rule inside kvm enforces that the vcpu->mutex is taken *inside*
> kvm->lock. The rule is violated by the pkvm_create_hyp_vm which acquires
^~~~~~~~~~~~~~~~~~
nit: always suffix function names with '()'
> the kvm->lock while already holding the vcpu->mutex lock from
> kvm_vcpu_ioctl. Follow the rule by taking the config lock while getting the
> VM handle and make sure that this is cleaned on VM destroy under the
> same lock.
It is always better to describe a lock in terms of what data it
protects, the critical section(s) are rather obvious here.
Avoid the circular locking dependency altogether by protecting the hyp
vm handle with the config_lock, much like we already do for other
forms of VM-scoped data.
> Signed-off-by: Sebastian Ene <[email protected]>
> Cc: [email protected]
nitpicks aside, this looks fine.
Reviewed-by: Oliver Upton <[email protected]>
--
Thanks,
Oliver
On Tue, Jan 23, 2024 at 05:35:26PM +0000, Oliver Upton wrote:
> On Tue, Jan 23, 2024 at 04:48:19PM +0000, Sebastian Ene wrote:
> > The rule inside kvm enforces that the vcpu->mutex is taken *inside*
> > kvm->lock. The rule is violated by the pkvm_create_hyp_vm which acquires
Hi Oliver,
> ^~~~~~~~~~~~~~~~~~
>
> nit: always suffix function names with '()'
>
> > the kvm->lock while already holding the vcpu->mutex lock from
> > kvm_vcpu_ioctl. Follow the rule by taking the config lock while getting the
> > VM handle and make sure that this is cleaned on VM destroy under the
> > same lock.
>
> It is always better to describe a lock in terms of what data it
> protects, the critical section(s) are rather obvious here.
>
> Avoid the circular locking dependency altogether by protecting the hyp
> vm handle with the config_lock, much like we already do for other
> forms of VM-scoped data.
>
> > Signed-off-by: Sebastian Ene <[email protected]>
> > Cc: [email protected]
>
> nitpicks aside, this looks fine.
>
> Reviewed-by: Oliver Upton <[email protected]>
>
Thanks for the suggestions, I updated the comit message and I will push
a V2 of the patch with the above and the Reviewed-by tag.
Thanks,
Seb
> --
> Thanks,
> Oliver