[KVM PATCH 0/4] irqfd fixes/enhancements

[KVM PATCH 0/4] irqfd fixes/enhancements

[Gregory Haskins]

(Applies to kvm.git/master:c27b64a0)

The following series represents my queue of fixes for issues in irqfd, 
which include enhancements to eventfd.  This is a respin of the patches
originally proposed yesterday, called "[KVM-RFC PATCH 0/2] eventfd
enhancements for irqfd/iosignalfd", which you can find a link to the thread
here:

http://www.gossamer-threads.com/lists/linux/kernel/1091852

Aside from introducing a few minor fixes (patches 1/4, 2/4), the general
solution for module reference counting has been extracted from the
(controversial) SRCU enhancement.  We can address the SRCU topic at a later
time.  For now, fixing the race condition is a higher priority item IMO.
Hopefully, this more focused series is more palatable for the
reviewers/maintainers involved.

---

Gregory Haskins (4):
      eventfd: add module reference counting support for registered notifiers
      eventfd: add generalized notifier interface
      KVM: fix irqfd error checking
      KVM: update inaccurate irqfd comment


 fs/eventfd.c            |   77 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/eventfd.h |   37 +++++++++++++++++++++
 virt/kvm/eventfd.c      |   82 ++++++++++++++++++-----------------------------
 3 files changed, 146 insertions(+), 50 deletions(-)

-- 

[KVM PATCH 1/4] KVM: update inaccurate irqfd comment

[Gregory Haskins]

We no longer hold the fget() for the irqfd lifetime, so clean up the comments
related to that.

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---

 virt/kvm/eventfd.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 2c8028c..a0e329f 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -156,9 +156,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 
-	/*
-	 * Embed the file* lifetime in the irqfd.
-	 */
 	file = eventfd_fget(fd);
 	if (IS_ERR(file)) {
 		ret = PTR_ERR(file);

[KVM PATCH 2/4] KVM: fix irqfd error checking

[Gregory Haskins]

Michael Tsirkin pointed out that f_ops->poll() does not return a standard
"int" error, yet we are treating it as such. Lets fix this.

Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---

 virt/kvm/eventfd.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index a0e329f..a9e7de7 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -144,6 +144,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
 	int ret;
+	unsigned int events;
 
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
@@ -169,9 +170,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
 	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
 
-	ret = file->f_op->poll(file, &irqfd->pt);
-	if (ret < 0)
-		goto fail;
+	events = file->f_op->poll(file, &irqfd->pt);
 
 	kvm_get_kvm(kvm);
 
@@ -180,6 +179,12 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	mutex_unlock(&kvm->lock);
 
 	/*
+	 * Check if there was an event already queued
+	 */
+	if (events & POLLIN)
+		schedule_work(&irqfd->inject);
+
+	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise
 	 * we might race against the POLLHUP
 	 */
@@ -188,9 +193,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	return 0;
 
 fail:
-	if (irqfd->wqh)
-		remove_wait_queue(irqfd->wqh, &irqfd->wait);
-
 	if (file && !IS_ERR(file))
 		fput(file);
 

[KVM PATCH 3/4] eventfd: add generalized notifier interface

[Gregory Haskins]

We currently open-code notification registration via the f_ops->poll()
method of the eventfd.  Lets abstract this into a notification API
extension of eventfd, while still using the wait-queue as the underlying
mechanism.  This will allow us to manage the notification mechanism
internal to eventfd without requiring the clients to change.  This also
gives us the opportunity to implement race-free release() callbacks,
which we do later in the series.

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
CC: Davide Libenzi <davidel@xmailserver.org>
---

 fs/eventfd.c            |   69 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/eventfd.h |   34 +++++++++++++++++++++
 virt/kvm/eventfd.c      |   75 ++++++++++++++++++-----------------------------
 3 files changed, 132 insertions(+), 46 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 72f5f8d..f9d7e1d 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -245,3 +245,72 @@ SYSCALL_DEFINE1(eventfd, unsigned int, count)
 	return sys_eventfd2(count, 0);
 }
 
+static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
+				   int sync, void *key)
+{
+	struct eventfd_notifier *en;
+	unsigned long flags = (unsigned long)key;
+
+	en = container_of(wait, struct eventfd_notifier, wait);
+
+	if (flags & POLLIN)
+		/*
+		 * The POLLIN wake_up is called with interrupts disabled.
+		 */
+		en->ops->signal(en);
+
+	if (flags & POLLHUP) {
+		/*
+		 * The POLLHUP is called unlocked, so it theoretically should
+		 * be safe to remove ourselves from the wqh using the locked
+		 * variant of remove_wait_queue()
+		 */
+		remove_wait_queue(en->wqh, &en->wait);
+		en->ops->release(en);
+	}
+
+	return 0;
+}
+
+static void eventfd_notifier_ptable_enqueue(struct file *file,
+					    wait_queue_head_t *wqh,
+					    poll_table *pt)
+{
+	struct eventfd_notifier *en;
+
+	en = container_of(pt, struct eventfd_notifier, pt);
+
+	en->wqh = wqh;
+	add_wait_queue(wqh, &en->wait);
+}
+
+int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en)
+{
+	unsigned int events;
+
+	if (file->f_op != &eventfd_fops)
+		return -EINVAL;
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&en->wait, eventfd_notifier_wakeup);
+	init_poll_funcptr(&en->pt, eventfd_notifier_ptable_enqueue);
+
+	events = file->f_op->poll(file, &en->pt);
+
+	return (events & POLLIN) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_register);
+
+int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)
+{
+	if (!en->wqh)
+		return -EINVAL;
+
+	remove_wait_queue(en->wqh, &en->wait);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(eventfd_notifier_unregister);
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index f45a8ae..802b59d 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -8,6 +8,32 @@
 #ifndef _LINUX_EVENTFD_H
 #define _LINUX_EVENTFD_H
 
+#include <linux/wait.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/list.h>
+
+struct eventfd_notifier;
+
+struct eventfd_notifier_ops {
+	void (*signal)(struct eventfd_notifier *en);
+	void (*release)(struct eventfd_notifier *en);
+};
+
+struct eventfd_notifier {
+	poll_table                         pt;
+	wait_queue_head_t                 *wqh;
+	wait_queue_t                       wait;
+	const struct eventfd_notifier_ops *ops;
+};
+
+static inline void eventfd_notifier_init(struct eventfd_notifier *en,
+					 const struct eventfd_notifier_ops *ops)
+{
+	memset(en, 0, sizeof(*en));
+	en->ops = ops;
+}
+
 #ifdef CONFIG_EVENTFD
 
 /* For O_CLOEXEC and O_NONBLOCK */
@@ -29,12 +55,20 @@
 
 struct file *eventfd_fget(int fd);
 int eventfd_signal(struct file *file, int n);
+int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en);
+int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en);
 
 #else /* CONFIG_EVENTFD */
 
 #define eventfd_fget(fd) ERR_PTR(-ENOSYS)
 static inline int eventfd_signal(struct file *file, int n)
 { return 0; }
+static inline int eventfd_notifier_register(struct file *file,
+					    struct eventfd_notifier *en)
+{ return -ENOENT; }
+static inline int eventfd_notifier_unregister(struct file *file,
+					      struct eventfd_notifier *en)
+{ return -ENOENT; }
 
 #endif /* CONFIG_EVENTFD */
 
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index a9e7de7..7afa483 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -43,9 +43,7 @@ struct _irqfd {
 	struct kvm               *kvm;
 	int                       gsi;
 	struct list_head          list;
-	poll_table                pt;
-	wait_queue_head_t        *wqh;
-	wait_queue_t              wait;
+	struct eventfd_notifier   notifier;
 	struct work_struct        inject;
 };
 
@@ -97,54 +95,43 @@ irqfd_disconnect(struct _irqfd *irqfd)
 	kvm_put_kvm(kvm);
 }
 
-static int
-irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+static void
+irqfd_eventfd_signal(struct eventfd_notifier *notifier)
 {
-	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
-	unsigned long flags = (unsigned long)key;
-
-	if (flags & POLLIN)
-		/*
-		 * The POLLIN wake_up is called with interrupts disabled.
-		 * Therefore we need to defer the IRQ injection until later
-		 * since we need to acquire the kvm->lock to do so.
-		 */
-		schedule_work(&irqfd->inject);
+	struct _irqfd *irqfd = container_of(notifier, struct _irqfd, notifier);
 
-	if (flags & POLLHUP) {
-		/*
-		 * The POLLHUP is called unlocked, so it theoretically should
-		 * be safe to remove ourselves from the wqh using the locked
-		 * variant of remove_wait_queue()
-		 */
-		remove_wait_queue(irqfd->wqh, &irqfd->wait);
-		flush_work(&irqfd->inject);
-		irqfd_disconnect(irqfd);
-
-		cleanup_srcu_struct(&irqfd->srcu);
-		kfree(irqfd);
-	}
-
-	return 0;
+	/*
+	 * The ->signal() is called with interrupts disabled.
+	 * Therefore we need to defer the IRQ injection until later
+	 * since we need to acquire the kvm->lock to do so.
+	 */
+	schedule_work(&irqfd->inject);
 }
 
 static void
-irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
-			poll_table *pt)
+irqfd_eventfd_release(struct eventfd_notifier *notifier)
 {
-	struct _irqfd *irqfd = container_of(pt, struct _irqfd, pt);
+	struct _irqfd *irqfd = container_of(notifier, struct _irqfd, notifier);
 
-	irqfd->wqh = wqh;
-	add_wait_queue(wqh, &irqfd->wait);
+	flush_work(&irqfd->inject);
+	irqfd_disconnect(irqfd);
+
+	cleanup_srcu_struct(&irqfd->srcu);
+	kfree(irqfd);
 }
 
+const static struct eventfd_notifier_ops irqfd_eventfd_ops = {
+	.signal = irqfd_eventfd_signal,
+	.release = irqfd_eventfd_release,
+};
+
+
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;
 	int ret;
-	unsigned int events;
 
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
 	if (!irqfd)
@@ -163,14 +150,10 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 		goto fail;
 	}
 
-	/*
-	 * Install our own custom wake-up handling so we are notified via
-	 * a callback whenever someone signals the underlying eventfd
-	 */
-	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
-	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
-
-	events = file->f_op->poll(file, &irqfd->pt);
+	eventfd_notifier_init(&irqfd->notifier, &irqfd_eventfd_ops);
+	ret = eventfd_notifier_register(file, &irqfd->notifier);
+	if (ret < 0)
+		goto fail;
 
 	kvm_get_kvm(kvm);
 
@@ -181,12 +164,12 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 	/*
 	 * Check if there was an event already queued
 	 */
-	if (events & POLLIN)
+	if (ret > 0)
 		schedule_work(&irqfd->inject);
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise
-	 * we might race against the POLLHUP
+	 * we might race against the notifier->release()
 	 */
 	fput(file);
 

[KVM PATCH 4/4] eventfd: add module reference counting support for registered notifiers

[Gregory Haskins]

Michael Tsirkin found a race condition in the irqfd code where we may
allow the underlying eventfd object to race with the rmmod of kvm.ko.

Since we now use eventfd_notifier for irqfd, lets add a struct module *owner
field to properly maintain references to our registered signal handlers.

Found-by: Michael S. Tsirkin <mst@redhat.com>
CC: Davide Libenzi <davidel@xmailserver.org>
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---

 fs/eventfd.c            |    8 ++++++++
 include/linux/eventfd.h |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index f9d7e1d..4a073ee 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -260,6 +260,8 @@ static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
 		en->ops->signal(en);
 
 	if (flags & POLLHUP) {
+		struct module *owner = en->owner;
+
 		/*
 		 * The POLLHUP is called unlocked, so it theoretically should
 		 * be safe to remove ourselves from the wqh using the locked
@@ -267,6 +269,8 @@ static int eventfd_notifier_wakeup(wait_queue_t *wait, unsigned mode,
 		 */
 		remove_wait_queue(en->wqh, &en->wait);
 		en->ops->release(en);
+
+		module_put(owner);
 	}
 
 	return 0;
@@ -291,6 +295,9 @@ int eventfd_notifier_register(struct file *file, struct eventfd_notifier *en)
 	if (file->f_op != &eventfd_fops)
 		return -EINVAL;
 
+	if (!try_module_get(en->owner))
+		return -EINVAL;
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd
@@ -310,6 +317,7 @@ int eventfd_notifier_unregister(struct file *file, struct eventfd_notifier *en)
 		return -EINVAL;
 
 	remove_wait_queue(en->wqh, &en->wait);
+	module_put(en->owner);
 
 	return 0;
 }
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index 802b59d..7e015f0 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -12,6 +12,7 @@
 #include <linux/poll.h>
 #include <linux/file.h>
 #include <linux/list.h>
+#include <linux/module.h>
 
 struct eventfd_notifier;
 
@@ -21,6 +22,7 @@ struct eventfd_notifier_ops {
 };
 
 struct eventfd_notifier {
+	struct module                     *owner;
 	poll_table                         pt;
 	wait_queue_head_t                 *wqh;
 	wait_queue_t                       wait;
@@ -31,6 +33,7 @@ static inline void eventfd_notifier_init(struct eventfd_notifier *en,
 					 const struct eventfd_notifier_ops *ops)
 {
 	memset(en, 0, sizeof(*en));
+	en->owner = THIS_MODULE;
 	en->ops = ops;
 }
 

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

[Davide Libenzi]

On Thu, 18 Jun 2009, Gregory Haskins wrote:

> We currently open-code notification registration via the f_ops->poll()
> method of the eventfd.  Lets abstract this into a notification API
> extension of eventfd, while still using the wait-queue as the underlying
> mechanism.  This will allow us to manage the notification mechanism
> internal to eventfd without requiring the clients to change.  This also
> gives us the opportunity to implement race-free release() callbacks,
> which we do later in the series.

Another attempt to push KVM stuff into eventfd. NACK.


- Davide

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

[Gregory Haskins]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Davide Libenzi wrote:
> On Thu, 18 Jun 2009, Gregory Haskins wrote:
>
>   
>> We currently open-code notification registration via the f_ops->poll()
>> method of the eventfd.  Lets abstract this into a notification API
>> extension of eventfd, while still using the wait-queue as the underlying
>> mechanism.  This will allow us to manage the notification mechanism
>> internal to eventfd without requiring the clients to change.  This also
>> gives us the opportunity to implement race-free release() callbacks,
>> which we do later in the series.
>>     
>
> Another attempt to push KVM stuff into eventfd.

??  I do not believe there is a single iota of KVM specific code in that
patch.  Can you elaborate?

-Greg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]

Re: [KVM PATCH 3/4] eventfd: add generalized notifier interface

[Gregory Haskins]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

Davide Libenzi wrote:
> On Thu, 18 Jun 2009, Gregory Haskins wrote:
>
>   
>> We currently open-code notification registration via the f_ops->poll()
>> method of the eventfd.  Lets abstract this into a notification API
>> extension of eventfd, while still using the wait-queue as the underlying
>> mechanism.  This will allow us to manage the notification mechanism
>> internal to eventfd without requiring the clients to change.  This also
>> gives us the opportunity to implement race-free release() callbacks,
>> which we do later in the series.
>>     
>
> Another attempt to push KVM stuff into eventfd.

Also, could you please detail how you expect someone to use the POLLHUP
release in a race free way without patches 3/4 + 4/4?  That is not a KVM
specific problem either, afaict.

-Greg



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]

Re: [KVM PATCH 0/4] irqfd fixes/enhancements

[Avi Kivity]

On 06/18/2009 08:44 PM, Gregory Haskins wrote:
> (Applies to kvm.git/master:c27b64a0)
>
> The following series represents my queue of fixes for issues in irqfd,
> which include enhancements to eventfd.  This is a respin of the patches
> originally proposed yesterday, called "[KVM-RFC PATCH 0/2] eventfd
> enhancements for irqfd/iosignalfd", which you can find a link to the thread
> here:
>
> http://www.gossamer-threads.com/lists/linux/kernel/1091852
>
> Aside from introducing a few minor fixes (patches 1/4, 2/4), the general
> solution for module reference counting has been extracted from the
> (controversial) SRCU enhancement.  We can address the SRCU topic at a later
> time.  For now, fixing the race condition is a higher priority item IMO.
> Hopefully, this more focused series is more palatable for the
> reviewers/maintainers involved.
>    

Applied 1-2, thanks.

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