Skip to contentSkip to conversation

kvm: fix irqfd assign/deassign race

PATCH Applied Michael S. Tsirkin 3 participants 3 messages Sep 19, 2010 – Sep 20, 2010 KVM

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

PATCH 1/1 kvm: fix irqfd assign/deassign race

I think I see the following (theoretical) race:

During irqfd assign, we drop irqfds lock before we schedule inject work. Therefore, deassign running on another CPU could cause shutdown and flush to run before inject, causing user after free in inject.

A simple fix it to schedule inject under the lock.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

If the issue is real, this might be a 2.6.36 and -stable candidate. Comments?

 virt/kvm/eventfd.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
virt/kvm/eventfd.c +2 -1
index 66cf65b..c1f1e3c 100644
@@ -218,7 +218,6 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);
-	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * Check if there was an event already pending on the eventfd

@@ -227,6 +226,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	if (events & POLLIN)
 		schedule_work(&irqfd->inject);
 
+	spin_unlock_irq(&kvm->irqfds.lock);
+
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise
 	 * we might race against the POLLHUP
Signature · 1 line
1.7.2.2
Acked-by on v1 1/1
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line

On 9/19/2010 at 01:02 PM, in message <[email protected]>,

Quoted 8 lines · Michael S. Tsirkin

I think I see the following (theoretical) race:

During irqfd assign, we drop irqfds lock before we schedule inject work. Therefore, deassign running on another CPU could cause shutdown and flush to run before inject, causing user after free in inject.

A simple fix it to schedule inject under the lock.

I swear there was some reason why the schedule_work() was done outside of the lock, but I can't for the life of me remember why anymore (it obviously was a failing on my part to _comment_ why if there was such a reason). So, short of recalling what that reason was, and the fact that Michael's theory seems rational and legit...

Acked-by Gregory Haskins <[email protected]>
Quoted 31 lines
Signed-off-by Michael S. Tsirkin <[email protected]>

---

If the issue is real, this might be a 2.6.36 and -stable candidate. Comments?

 virt/kvm/eventfd.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
virt/kvm/eventfd.c +2 -1
index 66cf65b..c1f1e3c 100644
@@ -218,7 +218,6 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);
-	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * Check if there was an event already pending on the eventfd

@@ -227,6 +226,8 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	if (events & POLLIN)
 		schedule_work(&irqfd->inject);
 
+	spin_unlock_irq(&kvm->irqfds.lock);
+
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise
 	 * we might race against the POLLHUP
Applied
#2 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin

I think I see the following (theoretical) race:

During irqfd assign, we drop irqfds lock before we schedule inject work. Therefore, deassign running on another CPU could cause shutdown and flush to run before inject, causing user after free in inject.

A simple fix it to schedule inject under the lock.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Applied, thanks.