[PATCH 0/3][v2] Fix racy in kvm_free_assigned_irq

[PATCH 0/3][v2] Fix racy in kvm_free_assigned_irq

[Sheng Yang]

Hi Avi

I just add a comment for kvm_free_assigned_irq(). The other things are all
the same as the patchset you have reviewed.

Thanks.
--
regards
Yang, Sheng

[PATCH 1/3] KVM: Remove duplicated prototype of kvm_arch_destroy_vm

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm_host.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 545a133..dab985b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -237,7 +237,6 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   int user_alloc);
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg);
-void kvm_arch_destroy_vm(struct kvm *kvm);
 
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu);
 int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu);
-- 
1.5.4.5

[PATCH 2/3] KVM: Add kvm_arch_sync_events to sync with asynchronize events

[Sheng Yang]

kvm_arch_sync_events is introduced to quiet down all other events may happen
contemporary with VM destroy process, like IRQ handler and work struct for
assigned device.

For kvm_arch_sync_events is called at the very beginning of kvm_destroy_vm(), so
the state of KVM here is legal and can provide a environment to quiet down other
events.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/ia64/kvm/kvm-ia64.c   |    4 ++++
 arch/powerpc/kvm/powerpc.c |    4 ++++
 arch/s390/kvm/kvm-s390.c   |    4 ++++
 arch/x86/kvm/x86.c         |    4 ++++
 include/linux/kvm_host.h   |    1 +
 virt/kvm/kvm_main.c        |    1 +
 6 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index b342b7e..1477f91 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1337,6 +1337,10 @@ static void kvm_release_vm_pages(struct kvm *kvm)
 	}
 }
 
+void kvm_arch_sync_events(struct kvm *kvm)
+{
+}
+
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	kvm_iommu_unmap_guest(kvm);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 13226f1..f30be9e 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -125,6 +125,10 @@ static void kvmppc_free_vcpus(struct kvm *kvm)
 	}
 }
 
+void kvm_arch_sync_events(struct kvm *kvm)
+{
+}
+
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	kvmppc_free_vcpus(kvm);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 54b6534..cbfe91e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -212,6 +212,10 @@ static void kvm_free_vcpus(struct kvm *kvm)
 	}
 }
 
+void kvm_arch_sync_events(struct kvm *kvm)
+{
+}
+
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	kvm_free_vcpus(kvm);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 18bba94..6613140 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4214,6 +4214,10 @@ static void kvm_free_vcpus(struct kvm *kvm)
 
 }
 
+void kvm_arch_sync_events(struct kvm *kvm)
+{
+}
+
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	kvm_free_all_assigned_devices(kvm);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index dab985b..3cf0ede 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -284,6 +284,7 @@ void kvm_free_physmem(struct kvm *kvm);
 struct  kvm *kvm_arch_create_vm(void);
 void kvm_arch_destroy_vm(struct kvm *kvm);
 void kvm_free_all_assigned_devices(struct kvm *kvm);
+void kvm_arch_sync_events(struct kvm *kvm);
 
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5703557..b1f5dfe 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -883,6 +883,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 {
 	struct mm_struct *mm = kvm->mm;
 
+	kvm_arch_sync_events(kvm);
 	spin_lock(&kvm_lock);
 	list_del(&kvm->vm_list);
 	spin_unlock(&kvm_lock);
-- 
1.5.4.5

[PATCH 3/3] KVM: Fix racy in kvm_free_assigned_irq

[Sheng Yang]

In the past, kvm_get_kvm() and kvm_put_kvm() was called in assigned device irq
handler and interrupt_work, in order to prevent cancel_work_sync() in
kvm_free_assigned_irq got a illegal state when waiting for interrupt_work done.
But it's tricky and still got two problems:

1. A bug ignored two conditions that cancel_work_sync() would return true result
in a additional kvm_put_kvm().

2. If interrupt type is MSI, we would got a window between cancel_work_sync()
and free_irq(), which interrupt would be injected again...

This patch discard the reference count used for irq handler and interrupt_work,
and ensure the legal state by moving the free function at the very beginning of
kvm_destroy_vm(). And the patch fix the second bug by disable irq before
cancel_work_sync(), which may result in nested disable of irq but OK for we are
going to free it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/x86.c  |    2 +-
 virt/kvm/kvm_main.c |   27 +++++++++++++++++++--------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6613140..2dbd501 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4216,11 +4216,11 @@ static void kvm_free_vcpus(struct kvm *kvm)
 
 void kvm_arch_sync_events(struct kvm *kvm)
 {
+	kvm_free_all_assigned_devices(kvm);
 }
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
-	kvm_free_all_assigned_devices(kvm);
 	kvm_iommu_unmap_guest(kvm);
 	kvm_free_pit(kvm);
 	kfree(kvm->arch.vpic);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b1f5dfe..7a102d7 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -173,7 +173,6 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 		assigned_dev->host_irq_disabled = false;
 	}
 	mutex_unlock(&assigned_dev->kvm->lock);
-	kvm_put_kvm(assigned_dev->kvm);
 }
 
 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
@@ -181,8 +180,6 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
 
-	kvm_get_kvm(assigned_dev->kvm);
-
 	schedule_work(&assigned_dev->interrupt_work);
 
 	disable_irq_nosync(irq);
@@ -213,6 +210,7 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	}
 }
 
+/* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
 static void kvm_free_assigned_irq(struct kvm *kvm,
 				  struct kvm_assigned_dev_kernel *assigned_dev)
 {
@@ -228,11 +226,24 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
 	if (!assigned_dev->irq_requested_type)
 		return;
 
-	if (cancel_work_sync(&assigned_dev->interrupt_work))
-		/* We had pending work. That means we will have to take
-		 * care of kvm_put_kvm.
-		 */
-		kvm_put_kvm(kvm);
+	/*
+	 * In kvm_free_device_irq, cancel_work_sync return true if:
+	 * 1. work is scheduled, and then cancelled.
+	 * 2. work callback is executed.
+	 *
+	 * The first one ensured that the irq is disabled and no more events
+	 * would happen. But for the second one, the irq may be enabled (e.g.
+	 * for MSI). So we disable irq here to prevent further events.
+	 *
+	 * Notice this maybe result in nested disable if the interrupt type is
+	 * INTx, but it's OK for we are going to free it.
+	 *
+	 * If this function is a part of VM destroy, please ensure that till
+	 * now, the kvm state is still legal for probably we also have to wait
+	 * interrupt_work done.
+	 */
+	disable_irq_nosync(assigned_dev->host_irq);
+	cancel_work_sync(&assigned_dev->interrupt_work);
 
 	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 
-- 
1.5.4.5

Re: [PATCH 0/3][v2] Fix racy in kvm_free_assigned_irq

[Avi Kivity]

Sheng Yang wrote:
> Hi Avi
>
> I just add a comment for kvm_free_assigned_irq(). The other things are all
> the same as the patchset you have reviewed.
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/3][v2] Fix racy in kvm_free_assigned_irq

[Mark McLoughlin]

On Tue, 2009-01-06 at 11:21 +0200, Avi Kivity wrote:
> Sheng Yang wrote:
> > Hi Avi
> >
> > I just add a comment for kvm_free_assigned_irq(). The other things are all
> > the same as the patchset you have reviewed.
> >   
> 
> Applied, thanks.

These would make sense for 2.6.29, right?

I just saw the oops below when I killed a busy guest with an assigned
device, and it looks like it might be related.

Cheers,
Mark.

BUG: unable to handle kernel paging request at 0000000000200200
IP: [<ffffffff811945e4>] list_del+0x10/0x85
PGD 171c82067 PUD 1741d8067 PMD 0 
Oops: 0000 [#1] SMP 
last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:01:00.0/local_cpus
CPU 1 
Modules linked in: fuse i915 drm i2c_algo_bit ipt_MASQUERADE iptable_nat nf_nat sco bnep l2cap bluetooth sunrpc bridge stp xt_physdev ip6t_REJECT nf_conntrack_ipv6 ip6table_filter ip6_tables ipv6 dm_multipath kvm_intel kvm uinput snd_hda_codec_idt snd_hda_intel snd_hda_codec snd_hwdep snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd e1000e i2c_i801 soundcore snd_page_alloc i2c_core pcspkr joydev ata_generic pata_acpi [last unloaded: freq_table]
Pid: 17, comm: events/1 Tainted: G        W  2.6.29-rc3 #30
RIP: 0010:[<ffffffff811945e4>]  [<ffffffff811945e4>] list_del+0x10/0x85
RSP: 0018:ffff88017e165dc0  EFLAGS: 00010286
RAX: 0000000000200200 RBX: ffff8801628eca98 RCX: 0000000000000000
RDX: 0000000000050001 RSI: ffffffffa010ab98 RDI: ffff8801628eca98
RBP: ffff88017e165dd0 R08: 0000000000000002 R09: 0000000000000000
R10: 0000000000000000 R11: ffff88017e081140 R12: ffff880164b01c80
R13: ffff88016c1a91a8 R14: ffff88017e165e70 R15: 0000000000000006
FS:  0000000000000000(0000) GS:ffff88017e07b000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000200200 CR3: 0000000171c0c000 CR4: 00000000000426e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process events/1 (pid: 17, threadinfo ffff88017e164000, task ffff88017e180000)
Stack:
 ffffffffa010ab98 ffff8801628ec000 ffff88017e165df0 ffffffffa010aba4
 ffff88016c1a91b0 ffff88016c1a9188 ffff88017e165e40 ffffffffa010b93a
 ffff88017e160148 000000007e160148 ffff88017e165e70 ffff88016c1a91b0
Call Trace:
 [<ffffffffa010ab98>] ? kvm_put_kvm+0x32/0xac [kvm]
 [<ffffffffa010aba4>] kvm_put_kvm+0x3e/0xac [kvm]

Message from  [<ffffffffa010b93a>] kvm_assigned_dev_interrupt_work_handler+0x18d/0x19c [kvm]
 [<ffffffff810598a8>] run_workqueue+0x103/0x20a
 [<ffffffff81059856>] ? run_workqueue+0xb1/0x20a
 [<ffffffffa010b7ad>] ? kvm_assigned_dev_interrupt_work_handler+0x0/0x19c [kvm]
 [<ffffffff81059a8f>] worker_thread+0xe0/0xf1
 [<ffffffff8105d6c8>] ? autoremove_wake_function+0x0/0x38
 [<ffffffff810599af>] ? worker_thread+0x0/0xf1
 [<ffffffff8105d350>] kthread+0x49/0x76
 [<ffffffff8101262a>] child_rip+0xa/0x20
 [<ffffffff81393972>] ? _spin_unlock_irq+0x2b/0x37
 [<ffffffff8106b1f8>] ? trace_hardirqs_on+0xd/0xf
 [<ffffffff81011f3e>] ? restore_args+0x0/0x30
 [<ffffffff8105d307>] ? kthread+0x0/0x76
 [<ffffffff81012620>] ? child_rip+0x0/0x20
Code: 7c 24 18 e8 98 00 00 00 4c 89 ef e8 7b fe ff ff 59 5b 41 5c 41 5d c9 c3 90 90 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 8b 47 08 <48> 8b 00 48 39 f8 74 20 49 89 c0 48 89 f9 48 c7 c2 2a 7b 4d 81 
RIP  [<ffffffff811945e4>] list_del+0x10/0x85
 RSP <ffff88017e165dc0>
CR2: 0000000000200200

Re: [PATCH 0/3][v2] Fix racy in kvm_free_assigned_irq

[Marcelo Tosatti]

On Fri, Feb 06, 2009 at 10:49:18AM +0000, Mark McLoughlin wrote:
> On Tue, 2009-01-06 at 11:21 +0200, Avi Kivity wrote:
> > Sheng Yang wrote:
> > > Hi Avi
> > >
> > > I just add a comment for kvm_free_assigned_irq(). The other things are all
> > > the same as the patchset you have reviewed.
> > >   
> > 
> > Applied, thanks.
> 
> These would make sense for 2.6.29, right?

Yep, these have been pushed to branch kvm-updates/2.6.29.

> I just saw the oops below when I killed a busy guest with an assigned
> device, and it looks like it might be related.

Yep. kvm_put_kvm from kvm_assigned_dev_interrupt_work_handler is gone.

> Cheers,
> Mark.
> 
> BUG: unable to handle kernel paging request at 0000000000200200
> IP: [<ffffffff811945e4>] list_del+0x10/0x85
> PGD 171c82067 PUD 1741d8067 PMD 0 

[PATCH 0/3] Fix racy in kvm_free_assigned_irq

[Sheng Yang]

[PATCH 3/3] KVM: Fix racy in kvm_free_assigned_irq

[Sheng Yang]

In the past, kvm_get_kvm() and kvm_put_kvm() was called in assigned device irq
handler and interrupt_work, in order to prevent cancel_work_sync() in
kvm_free_assigned_irq got a illegal state when waiting for interrupt_work done.
But it's tricky and still got two problems:

1. A bug ignored two conditions that cancel_work_sync() would return true result
in a additional kvm_put_kvm().

2. If interrupt type is MSI, we would got a window between cancel_work_sync()
and free_irq(), which interrupt would be injected again...

This patch discard the reference count used for irq handler and interrupt_work,
and ensure the legal state by moving the free function at the very beginning of
kvm_destroy_vm(). And the patch fix the second bug by disable irq before
cancel_work_sync(), which may result in nested disable of irq but OK for we are
going to free it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/x86.c  |    2 +-
 virt/kvm/kvm_main.c |   26 ++++++++++++++++++--------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0a045fc..56418ad 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4194,11 +4194,11 @@ static void kvm_free_vcpus(struct kvm *kvm)
 
 void kvm_arch_sync_events(struct kvm *kvm)
 {
+	kvm_free_all_assigned_devices(kvm);
 }
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
-	kvm_free_all_assigned_devices(kvm);
 	kvm_iommu_unmap_guest(kvm);
 	kvm_free_pit(kvm);
 	kfree(kvm->arch.vpic);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d348788..fc014b8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -173,7 +173,6 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 		assigned_dev->host_irq_disabled = false;
 	}
 	mutex_unlock(&assigned_dev->kvm->lock);
-	kvm_put_kvm(assigned_dev->kvm);
 }
 
 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
@@ -181,8 +180,6 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
 
-	kvm_get_kvm(assigned_dev->kvm);
-
 	schedule_work(&assigned_dev->interrupt_work);
 
 	disable_irq_nosync(irq);
@@ -228,11 +225,24 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
 	if (!assigned_dev->irq_requested_type)
 		return;
 
-	if (cancel_work_sync(&assigned_dev->interrupt_work))
-		/* We had pending work. That means we will have to take
-		 * care of kvm_put_kvm.
-		 */
-		kvm_put_kvm(kvm);
+	/*
+	 * In kvm_free_device_irq, cancel_work_sync return true if:
+	 * 1. work is scheduled, and then cancelled.
+	 * 2. work callback is executed.
+	 *
+	 * The first one ensured that the irq is disabled and no more events
+	 * would happen. But for the second one, the irq may be enabled (e.g.
+	 * for MSI). So we disable irq here to prevent further events.
+	 *
+	 * Notice this maybe result in nested disable if the interrupt type is
+	 * INTx, but it's OK for we are going to free it.
+	 *
+	 * If this function is a part of VM destroy, please ensure that till
+	 * now, the kvm state is still legal for probably we also have to wait
+	 * interrupt_work done.
+	 */
+	disable_irq_nosync(assigned_dev->host_irq);
+	cancel_work_sync(&assigned_dev->interrupt_work);
 
 	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 
-- 
1.5.4.5