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 sebastianene@google.com Cc: stable@vger.kernel.org --- 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)
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 sebastianene@google.com Cc: stable@vger.kernel.org
nitpicks aside, this looks fine.
Reviewed-by: Oliver Upton oliver.upton@linux.dev
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 sebastianene@google.com Cc: stable@vger.kernel.org
nitpicks aside, this looks fine.
Reviewed-by: Oliver Upton oliver.upton@linux.dev
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
linux-stable-mirror@lists.linaro.org
-
Oliver Upton -
Sebastian Ene