diff mbox series

KVM: arm64: Fix circular locking dependency

Message ID 20240123164818.1306122-2-sebastianene@google.com
State New
Headers
Series KVM: arm64: Fix circular locking dependency |

Commit Message

Sebastian Ene Jan. 23, 2024, 4:48 p.m. UTC 
  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(-)

Comments

Oliver Upton Jan. 23, 2024, 5:35 p.m. UTC | #1 
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>
Sebastian Ene Jan. 24, 2024, 9:07 a.m. UTC | #2 
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
diff mbox series

Patch

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)