[PATCH] kvm-kmod: fix kvm_request_irq race

[PATCH] kvm-kmod: fix kvm_request_irq race

[Chris Wright]

Commit "32658734: Fix request_irq() for < 2.6.19" is racy between multiple
guests since ioctl is only serialized per guest.  Add mutex and serialize
kvm_request_irq/kvm_free_irq to avoid race.

Signed-off-by: Chris Wright <chrisw@redhat.com>
---
 external-module-compat-comm.h |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h
index 8cb5440..eaad986 100644
--- a/external-module-compat-comm.h
+++ b/external-module-compat-comm.h
@@ -645,6 +645,7 @@ static inline int pci_reset_function(struct pci_dev *dev)
 
 typedef irqreturn_t (*kvm_irq_handler_t)(int, void *);
 static kvm_irq_handler_t kvm_irq_handlers[NR_IRQS];
+static DEFINE_MUTEX(kvm_irq_handlers_mutex);
 
 static irqreturn_t kvm_irq_thunk(int irq, void *dev_id, struct pt_regs *regs)
 {
@@ -655,21 +656,28 @@ static irqreturn_t kvm_irq_thunk(int irq, void *dev_id, struct pt_regs *regs)
 static inline int kvm_request_irq(unsigned int a, kvm_irq_handler_t handler,
 				  unsigned long c, const char *d, void *e)
 {
-	int rc;
-	kvm_irq_handler_t old = kvm_irq_handlers[a];
+	int rc = -EBUSY;
+	kvm_irq_handler_t old;
+
+	mutex_lock(&kvm_irq_handlers_mutex);
+	old = kvm_irq_handlers[a];
 	if (old)
-		return -EBUSY;
+		goto out;
 	kvm_irq_handlers[a] = handler;
 	rc = request_irq(a, kvm_irq_thunk, c, d, e);
 	if (rc)
 		kvm_irq_handlers[a] = NULL;
+out:
+	mutex_unlock(&kvm_irq_handlers_mutex);
 	return rc;
 }
 
 static inline void kvm_free_irq(unsigned int irq, void *dev_id)
 {
+	mutex_lock(&kvm_irq_handlers_mutex);
 	free_irq(irq, dev_id);
 	kvm_irq_handlers[irq] = NULL;
+	mutex_unlock(&kvm_irq_handlers_mutex);
 }
 
 #else

Re: [PATCH] kvm-kmod: fix kvm_request_irq race

[Avi Kivity]

Chris Wright wrote:
> Commit "32658734: Fix request_irq() for < 2.6.19" is racy between multiple
> guests since ioctl is only serialized per guest.  Add mutex and serialize
> kvm_request_irq/kvm_free_irq to avoid race.
>
> Signed-off-by: Chris Wright <chrisw@redhat.com>
> ---
>  external-module-compat-comm.h |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h
> index 8cb5440..eaad986 100644
> --- a/external-module-compat-comm.h
> +++ b/external-module-compat-comm.h
> @@ -645,6 +645,7 @@ static inline int pci_reset_function(struct pci_dev *dev)
>  
>  typedef irqreturn_t (*kvm_irq_handler_t)(int, void *);
>  static kvm_irq_handler_t kvm_irq_handlers[NR_IRQS];
> +static DEFINE_MUTEX(kvm_irq_handlers_mutex);
>  
>   

Since this mutex is in a header file, it can be instantiated multiple 
times.  It will only serialize callers within a translation unit.  
Please define it in a C file.

Would be best to move the the code as well.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] kvm-kmod: fix kvm_request_irq race

[Chris Wright]

* Avi Kivity (avi@redhat.com) wrote:
> Since this mutex is in a header file, it can be instantiated multiple  
> times.  It will only serialize callers within a translation unit.   
> Please define it in a C file.
>
> Would be best to move the the code as well.

Yeah it seems to get linked multiple times (when I tried that I found
duplicates).  I'll look again.

thanks,
-chris

[PATCH kvm-kmod v2] fix kvm_request_irq race

[Chris Wright]

Commit "32658734: Fix request_irq() for < 2.6.19" is racy between multiple
guests since ioctl is only serialized per guest.  Add mutex and serialize
kvm_request_irq/kvm_free_irq to avoid race.  Also move all this to its
own C file to keep from creating per ojbect file private mutexes.

Signed-off-by: Chris Wright <chrisw@redhat.com>
---
 external-module-compat-comm.h |   29 +-------------------------
 ia64/Kbuild                   |    3 +-
 request-irq-compat.c          |   44 +++++++++++++++++++++++++++++++++++++++++
 x86/Kbuild                    |    2 +-
 4 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h
index f6975e0..c014b7f 100644
--- a/external-module-compat-comm.h
+++ b/external-module-compat-comm.h
@@ -656,33 +656,8 @@ static inline int pci_reset_function(struct pci_dev *dev)
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19)
 
 typedef irqreturn_t (*kvm_irq_handler_t)(int, void *);
-static kvm_irq_handler_t kvm_irq_handlers[NR_IRQS];
-
-static irqreturn_t kvm_irq_thunk(int irq, void *dev_id, struct pt_regs *regs)
-{
-	kvm_irq_handler_t handler = kvm_irq_handlers[irq];
-	return handler(irq, dev_id);
-}
-
-static inline int kvm_request_irq(unsigned int a, kvm_irq_handler_t handler,
-				  unsigned long c, const char *d, void *e)
-{
-	int rc;
-	kvm_irq_handler_t old = kvm_irq_handlers[a];
-	if (old)
-		return -EBUSY;
-	kvm_irq_handlers[a] = handler;
-	rc = request_irq(a, kvm_irq_thunk, c, d, e);
-	if (rc)
-		kvm_irq_handlers[a] = NULL;
-	return rc;
-}
-
-static inline void kvm_free_irq(unsigned int irq, void *dev_id)
-{
-	free_irq(irq, dev_id);
-	kvm_irq_handlers[irq] = NULL;
-}
+int kvm_request_irq(unsigned int a, kvm_irq_handler_t handler, unsigned long c, 		    const char *d, void *e);
+void kvm_free_irq(unsigned int irq, void *dev_id);
 
 #else
 
diff --git a/ia64/Kbuild b/ia64/Kbuild
index 5bc6098..e2c8acc 100644
--- a/ia64/Kbuild
+++ b/ia64/Kbuild
@@ -1,7 +1,8 @@
 obj-m := kvm.o kvm-intel.o
 
 kvm-objs := kvm_main.o ioapic.o coalesced_mmio.o kvm-ia64.o kvm_fw.o \
-	irq_comm.o ../anon_inodes.o ../external-module-compat.o
+	irq_comm.o ../anon_inodes.o ../external-module-compat.o \
+	../request-irq-compat.o
 
 ifeq ($(CONFIG_IOMMU_API),y)
 kvm-objs += iommu.o
diff --git a/request-irq-compat.c b/request-irq-compat.c
new file mode 100644
index 0000000..51193cb
--- /dev/null
+++ b/request-irq-compat.c
@@ -0,0 +1,44 @@
+/*
+ * compat for request_irq
+ */
+
+#include <linux/interrupt.h>
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19)
+
+static kvm_irq_handler_t kvm_irq_handlers[NR_IRQS];
+static DEFINE_MUTEX(kvm_irq_handlers_mutex);
+
+static irqreturn_t kvm_irq_thunk(int irq, void *dev_id, struct pt_regs *regs)
+{
+	kvm_irq_handler_t handler = kvm_irq_handlers[irq];
+	return handler(irq, dev_id);
+}
+
+int kvm_request_irq(unsigned int a, kvm_irq_handler_t handler,
+		    unsigned long c, const char *d, void *e)
+{
+	int rc = -EBUSY;
+	kvm_irq_handler_t old;
+
+	mutex_lock(&kvm_irq_handlers_mutex);
+	old = kvm_irq_handlers[a];
+	if (old)
+		goto out;
+	kvm_irq_handlers[a] = handler;
+	rc = request_irq(a, kvm_irq_thunk, c, d, e);
+	if (rc)
+		kvm_irq_handlers[a] = NULL;
+out:
+	mutex_unlock(&kvm_irq_handlers_mutex);
+	return rc;
+}
+
+void kvm_free_irq(unsigned int irq, void *dev_id)
+{
+	mutex_lock(&kvm_irq_handlers_mutex);
+	free_irq(irq, dev_id);
+	kvm_irq_handlers[irq] = NULL;
+	mutex_unlock(&kvm_irq_handlers_mutex);
+}
+
+#endif
diff --git a/x86/Kbuild b/x86/Kbuild
index d3aca00..c5b84e9 100644
--- a/x86/Kbuild
+++ b/x86/Kbuild
@@ -6,7 +6,7 @@ obj-m := kvm.o kvm-intel.o kvm-amd.o
 kvm-objs := kvm_main.o x86.o mmu.o x86_emulate.o ../anon_inodes.o irq.o i8259.o \
 	 lapic.o ioapic.o preempt.o i8254.o coalesced_mmio.o irq_comm.o \
 	 timer.o \
-	 ../external-module-compat.o
+	 ../external-module-compat.o ../request-irq-compat.o
 ifeq ($(EXT_CONFIG_KVM_TRACE),y)
 kvm-objs += kvm_trace.o
 endif

Re: [PATCH kvm-kmod v2] fix kvm_request_irq race

[Avi Kivity]

Chris Wright wrote:
> Commit "32658734: Fix request_irq() for < 2.6.19" is racy between multiple
> guests since ioctl is only serialized per guest.  Add mutex and serialize
> kvm_request_irq/kvm_free_irq to avoid race.  Also move all this to its
> own C file to keep from creating per ojbect file private mutexes.
>
>   

Applied, thanks.

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