linux-kernel - Re: [PATCH] KVM: arm64: vgic: Fix soft lockup during VM teardown

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 Jan 2023 09:28:47 -0600
From:   Shanker Donthineni <sdonthineni@...dia.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     James Morse <james.morse@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        linux-kernel@...r.kernel.org, Vikram Sethi <vsethi@...dia.com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Oliver Upton <oliver.upton@...ux.dev>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH] KVM: arm64: vgic: Fix soft lockup during VM teardown



On 1/20/23 06:00, Marc Zyngier wrote:
> What I am asking agin is: is there any overlap between any vgic ioctl
> and the teardown of the VM? Do you ever see kvm_vm_release() being
> called before kvm_device_release()?
> 
> Because that's the overlap I've been talking all along.

I've not observed overlap of kvm_vm_release() with ioctls kvm_device_ioctl(),
kvm_vcpu_ioctl() and kvm_vm_ioctl() for 3hrs. Test created hundreds of VMs and
teardowns.


Debug code:
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4f26b244f6d0..9c8409a894f5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -800,7 +800,7 @@ struct kvm {
         bool dirty_ring_with_bitmap;
         bool vm_bugged;
         bool vm_dead;
-
+       atomic_t checkoverlap;
  #ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
         struct notifier_block pm_notifier;
  #endif
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9c60384b5ae0..a5a9071aeb19 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1370,9 +1370,13 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
  {
         struct kvm *kvm = filp->private_data;

+       atomic_inc(&kvm->checkoverlap);
+       pr_err("%s(%d) start kvm=%px\n",__func__,__LINE__, kvm);
+
         kvm_irqfd_release(kvm);

         kvm_put_kvm(kvm);
+       pr_err("%s(%d) end kvm=%px\n",__func__,__LINE__, kvm);
         return 0;
  }

@@ -4073,6 +4077,8 @@ static long kvm_vcpu_ioctl(struct file *filp,
         if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
                 return -EINVAL;

+if (atomic_read(&vcpu->kvm->checkoverlap)) pr_err("%s(%d) overlap kvm=%px\n", __func__, __LINE__, vcpu->kvm);
+
         /*
          * Some architectures have vcpu ioctls that are asynchronous to vcpu
          * execution; mutex_lock() would break them.
@@ -4346,6 +4352,8 @@ static long kvm_device_ioctl(struct file *filp, unsigned int ioctl,
         if (dev->kvm->mm != current->mm || dev->kvm->vm_dead)
                 return -EIO;

+       if (atomic_read(&dev->kvm->checkoverlap)) pr_err("%s(%d) overlap kvm=%px\n", __func__, __LINE__, dev->kvm);
+
         switch (ioctl) {
         case KVM_SET_DEVICE_ATTR:
                 return kvm_device_ioctl_attr(dev, dev->ops->set_attr, arg);
@@ -4731,6 +4739,8 @@ static long kvm_vm_ioctl(struct file *filp,

         if (kvm->mm != current->mm || kvm->vm_dead)
                 return -EIO;
+if (atomic_read(&kvm->checkoverlap)) pr_err("%s(%d) overlap kvm=%px\n", __func__, __LINE__, kvm);
+
         switch (ioctl) {
         case KVM_CREATE_VCPU:
                 r = kvm_vm_ioctl_create_vcpu(kvm, arg);