irqfd: use POLLHUP notification for close()

virt/kvm/eventfd.c +15 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

virt/kvm/eventfd.c +15 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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()
+		 */

Quoted 24 lines · Gregory Haskins

virt/kvm/eventfd.c +19 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up

virt/kvm/eventfd.c line 97

Quoted 6 lines

virt/kvm/eventfd.c +4 -0

+	 * until then
+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Quoted 36 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

virt/kvm/eventfd.c +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 3 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Quoted 26 lines · Michael S. Tsirkin

Quoted 24 lines · Gregory Haskins

virt/kvm/eventfd.c +19 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up

Quoted 9 lines

virt/kvm/eventfd.c line 97

Quoted 6 lines

virt/kvm/eventfd.c +4 -0

+	 * until then
+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Yep, by design. It becomes part of the eventfd and is thus associated with its lifetime. Consider it as if we made our own anon-fd implementation for irqfd and the lifetime looks similar. The difference is that we are reusing eventfd and its interface semantics.

Quoted 36 lines

Quoted 36 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

virt/kvm/eventfd.c +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 7 lines

Quoted 3 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Well, we hold a reference to struct kvm until we call irqfd_disconnect(). If kvm closes first, we disconnect and disassociate all references to kvm leaving irqfd->kvm = NULL. Likewise, if irqfd closes first, we disassociate with kvm with the above quoted logic. In either case, we are holding a kvm reference up until that "disconnect" point. Therefore kvm should not be able to disappear before that disconnect, and after that point we do not care.

If that is not sufficient to prevent kvm.ko from going away in the middle, then IMO kvm_get_kvm() has a bug, not irqfd. ;) However, I believe everything is actually ok here.

-Greg

Quoted 89 lines · Gregory Haskins

Quoted 27 lines · Michael S. Tsirkin

Quoted 24 lines · Gregory Haskins

(quoted diff) +19 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +4 -0

+	 * until then
+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Yep, by design. It becomes part of the eventfd and is thus associated with its lifetime. Consider it as if we made our own anon-fd implementation for irqfd and the lifetime looks similar. The difference is that we are reusing eventfd and its interface semantics.

Quoted 37 lines

Quoted 37 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

(quoted diff) +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 8 lines

Quoted 4 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Quoted 8 lines

Well, we hold a reference to struct kvm until we call irqfd_disconnect(). If kvm closes first, we disconnect and disassociate all references to kvm leaving irqfd->kvm = NULL. Likewise, if irqfd closes first, we disassociate with kvm with the above quoted logic. In either case, we are holding a kvm reference up until that "disconnect" point. Therefore kvm should not be able to disappear before that disconnect, and after that point we do not care.

Yes, we do care.

Here's the scenario in more detail:

- kvm is closed
- irq disconnect is called
- kvm is put
- kvm module is removed: all irqs are disconnected
- eventfd closes and triggers callback into removed kvm module
- crash

Quoted 5 lines

If that is not sufficient to prevent kvm.ko from going away in the middle, then IMO kvm_get_kvm() has a bug, not irqfd. ;) However, I believe everything is actually ok here.

Quoted 3 lines

-Greg

BTW, why can't we remove irqfds in kvm_release?

Quoted 102 lines · Michael S. Tsirkin

Quoted 89 lines · Gregory Haskins

Quoted 27 lines · Michael S. Tsirkin

Quoted 24 lines · Gregory Haskins

(quoted diff) +19 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +4 -0

+	 * until then
+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Yep, by design. It becomes part of the eventfd and is thus associated with its lifetime. Consider it as if we made our own anon-fd implementation for irqfd and the lifetime looks similar. The difference is that we are reusing eventfd and its interface semantics.

Quoted 37 lines

Quoted 37 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

(quoted diff) +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 8 lines

Quoted 4 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Quoted 8 lines

Well, we hold a reference to struct kvm until we call irqfd_disconnect(). If kvm closes first, we disconnect and disassociate all references to kvm leaving irqfd->kvm = NULL. Likewise, if irqfd closes first, we disassociate with kvm with the above quoted logic. In either case, we are holding a kvm reference up until that "disconnect" point. Therefore kvm should not be able to disappear before that disconnect, and after that point we do not care.

Yes, we do care.

Quoted 9 lines

Here's the scenario in more detail:

- kvm is closed
- irq disconnect is called
- kvm is put
- kvm module is removed: all irqs are disconnected
- eventfd closes and triggers callback into removed kvm module
- crash

[ lightbulb turns on]

Ah, now I see the point you were making. I thought you were talking about the .text in kvm_set_irq() (which would be protected by my kvm_get_kvm() reference afaict). But you are actually talking about the irqfd .text itself. Indeed, you are correct that is this currently a race. Good catch!

Quoted 10 lines

Quoted 8 lines

If that is not sufficient to prevent kvm.ko from going away in the middle, then IMO kvm_get_kvm() has a bug, not irqfd. ;) However, I believe everything is actually ok here.

-Greg

Quoted 2 lines

BTW, why can't we remove irqfds in kvm_release?

Well, this would be ideal but we run into that bi-directional reference thing that we talked about earlier and we both agree is non-trivial to solve. Solving this locking problem would incidentally also pave the way for restoring the DEASSIGN feature, so patches welcome!

In the meantime, I think we can close the hole you found with the following patch (build-tested only):

commit f3a8dccc9e815599438e9feb0ea53e8eb10ad2b3

Author: Gregory Haskins <[email protected]>
Date:   Sun Jun 14 23:37:49 2009 -0400

KVM: make irqfd take kvm.ko module reference

Michael Tsirkin pointed out that we currently have a race between someone holding an irqfd reference and an rmmod against kvm.ko. This patch closes that hole by making sure that irqfd holds a kvm.ko reference for its lifetime.

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

Signed-off-by Gregory Haskins <[email protected]>

virt/kvm/eventfd.c +1 -0

index 2c8028c..67e4eca 100644
@@ -29,6 +29,7 @@
 #include <linux/list.h>
 #include <linux/eventfd.h>
 #include <linux/srcu.h>
+#include <linux/module.h>
 
 /*
  * --------------------------------------------------------------------

@@ -123,6 +124,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int

sync, void
*key)

                cleanup_srcu_struct(&irqfd->srcu);
                kfree(irqfd);
+               module_put(THIS_MODULE);
        }
 
        return 0;

virt/kvm/eventfd.c +1 -0

@@ -176,6 +178,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
        if (ret < 0)
                goto fail;
 
+       __module_get(THIS_MODULE);
        kvm_get_kvm(kvm);
 
        mutex_lock(&kvm->lock);

Quoted 142 lines · Gregory Haskins

Quoted 111 lines · Michael S. Tsirkin

Quoted 97 lines · Gregory Haskins

Quoted 29 lines · Michael S. Tsirkin

Quoted 25 lines · Gregory Haskins

(quoted diff) +20 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up
+	 * until then

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +3 -0

+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Yep, by design. It becomes part of the eventfd and is thus associated with its lifetime. Consider it as if we made our own anon-fd implementation for irqfd and the lifetime looks similar. The difference is that we are reusing eventfd and its interface semantics.

Quoted 38 lines

Quoted 38 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

(quoted diff) +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 10 lines

Quoted 5 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Quoted 9 lines

Well, we hold a reference to struct kvm until we call irqfd_disconnect(). If kvm closes first, we disconnect and disassociate all references to kvm leaving irqfd->kvm = NULL. Likewise, if irqfd closes first, we disassociate with kvm with the above quoted logic. In either case, we are holding a kvm reference up until that "disconnect" point. Therefore kvm should not be able to disappear before that disconnect, and after that point we do not care.

Yes, we do care.

Quoted 10 lines

Here's the scenario in more detail:

- kvm is closed
- irq disconnect is called
- kvm is put
- kvm module is removed: all irqs are disconnected
- eventfd closes and triggers callback into removed kvm module
- crash

[ lightbulb turns on]

Ah, now I see the point you were making. I thought you were talking about the .text in kvm_set_irq() (which would be protected by my kvm_get_kvm() reference afaict). But you are actually talking about the irqfd .text itself. Indeed, you are correct that is this currently a race. Good catch!

Quoted 10 lines

Quoted 10 lines

If that is not sufficient to prevent kvm.ko from going away in the middle, then IMO kvm_get_kvm() has a bug, not irqfd. ;) However, I believe everything is actually ok here.

-Greg

Quoted 8 lines

Quoted 3 lines

BTW, why can't we remove irqfds in kvm_release?

Well, this would be ideal but we run into that bi-directional reference thing that we talked about earlier and we both agree is non-trivial to solve. Solving this locking problem would incidentally also pave the way for restoring the DEASSIGN feature, so patches welcome!

So far the only workable approach that I see is reverting the POLLHUP patch. I agree it looks pretty, but DEASSIGN and closing the races is more important IMO. And locking will definitely become much simpler.

Quoted 37 lines

In the meantime, I think we can close the hole you found with the following patch (build-tested only):

commit f3a8dccc9e815599438e9feb0ea53e8eb10ad2b3

Author: Gregory Haskins <[email protected]>
Date:   Sun Jun 14 23:37:49 2009 -0400

KVM: make irqfd take kvm.ko module reference

Michael Tsirkin pointed out that we currently have a race between someone holding an irqfd reference and an rmmod against kvm.ko. This patch closes that hole by making sure that irqfd holds a kvm.ko reference for its lifetime.

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

Signed-off-by Gregory Haskins <[email protected]>

virt/kvm/eventfd.c +1 -0

index 2c8028c..67e4eca 100644
@@ -29,6 +29,7 @@
 #include <linux/list.h>
 #include <linux/eventfd.h>
 #include <linux/srcu.h>
+#include <linux/module.h>
 
 /*
  * --------------------------------------------------------------------

@@ -123,6 +124,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int

sync, void
*key)

                cleanup_srcu_struct(&irqfd->srcu);
                kfree(irqfd);
+               module_put(THIS_MODULE);
        }

Quoted 2 lines

        return 0;

module_put(THIS_MODULE) is always a bug unless you know that someone has a reference to the current module: the module could go away between this call and returning from function.

Quoted 13 lines

virt/kvm/eventfd.c +1 -0

@@ -176,6 +178,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
        if (ret < 0)
                goto fail;
 
+       __module_get(THIS_MODULE);
        kvm_get_kvm(kvm);
 
        mutex_lock(&kvm->lock);

Quoted 9 lines

Quoted 9 lines

(quoted diff) +0 -0

@@ -123,6 +124,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int

sync, void
*key)

                cleanup_srcu_struct(&irqfd->srcu);
                kfree(irqfd);
+               module_put(THIS_MODULE);
        }

Quoted 7 lines

Quoted 2 lines

        return 0;

module_put(THIS_MODULE) is always a bug unless you know that someone has a reference to the current module: the module could go away between this call and returning from function.

Hmm. I understand what you are saying conceptually (i.e. the .text could get yanked before we hit the next line of code, in this case the "return 0"). However, holding a reference when you _know_ someone else holds a reference to me says that one of the references is redundant. In addition, there is certainly plenty of precedence for module_put(THIS_MODULE) all throughout the kernel (including module_put_and_exit()). Are those broken as well?

Quoted 7 lines · Gregory Haskins

Hmm. I understand what you are saying conceptually (i.e. the .text could get yanked before we hit the next line of code, in this case the "return 0"). However, holding a reference when you _know_ someone else holds a reference to me says that one of the references is redundant. In addition, there is certainly plenty of precedence for module_put(THIS_MODULE) all throughout the kernel (including module_put_and_exit()). Are those broken as well?

Maybe not, but I don't know why. It works fine as long as you don't unload any modules though :) Rusty, could you enlighten us please?

Quoted 11 lines · Michael S. Tsirkin

Quoted 7 lines · Gregory Haskins

Hmm. I understand what you are saying conceptually (i.e. the .text could get yanked before we hit the next line of code, in this case the "return 0"). However, holding a reference when you _know_ someone else holds a reference to me says that one of the references is redundant. In addition, there is certainly plenty of precedence for module_put(THIS_MODULE) all throughout the kernel (including module_put_and_exit()). Are those broken as well?

Maybe not, but I don't know why. It works fine as long as you don't unload any modules though :) Rusty, could you enlighten us please?

Yep, they're almost all broken. A few have comments indicating that someone else is holding a reference (eg. loopback).

But at some point you give up playing whack-a-mole for random drivers.

module_put_and_exit() does *not* have this problem, BTW.

Rusty.

Quoted 20 lines · Rusty Russell

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Gregory Haskins

Hmm. I understand what you are saying conceptually (i.e. the .text could get yanked before we hit the next line of code, in this case the "return 0"). However, holding a reference when you _know_ someone else holds a reference to me says that one of the references is redundant. In addition, there is certainly plenty of precedence for module_put(THIS_MODULE) all throughout the kernel (including module_put_and_exit()). Are those broken as well?

Quoted 3 lines

Maybe not, but I don't know why. It works fine as long as you don't unload any modules though :) Rusty, could you enlighten us please?

Yep, they're almost all broken. A few have comments indicating that someone else is holding a reference (eg. loopback).

But at some point you give up playing whack-a-mole for random drivers.

Quoted 4 lines

module_put_and_exit() does *not* have this problem, BTW.

Rusty.

I see that, the .text for module_put_and_exit is never modular itself. Thanks, Rusty!

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

Quoted 22 lines · Michael S. Tsirkin

Quoted 20 lines · Rusty Russell

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Gregory Haskins

Hmm. I understand what you are saying conceptually (i.e. the .text could get yanked before we hit the next line of code, in this case the "return 0"). However, holding a reference when you _know_ someone else holds a reference to me says that one of the references is redundant. In addition, there is certainly plenty of precedence for module_put(THIS_MODULE) all throughout the kernel (including module_put_and_exit()). Are those broken as well?

Quoted 3 lines

Maybe not, but I don't know why. It works fine as long as you don't unload any modules though :) Rusty, could you enlighten us please?

Yep, they're almost all broken. A few have comments indicating that someone else is holding a reference (eg. loopback).

But at some point you give up playing whack-a-mole for random drivers.

Quoted 8 lines

Quoted 4 lines

module_put_and_exit() does *not* have this problem, BTW.

Rusty.

I see that, the .text for module_put_and_exit is never modular itself. Thanks, Rusty!

Ah! That is the trick I wasn't understanding.

Quoted 3 lines

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Quoted 4 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Thanks Michael (and Rusty),
-Greg

Quoted 7 lines · Gregory Haskins

Quoted 4 lines · Michael S. Tsirkin

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Quoted 9 lines

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Here's what one could do: create a thread for each irqfd, and increment module ref count, put that thread to sleep. When done with irqfd, don't delete it and don't decrement module refcount, wake thread instead. thread kills irqfd and calls module_put_and_exit.

I don't think it's racy

Quoted 8 lines

Quoted 6 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Quoted 9 lines

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Can this help with DEASSIGN as well? We need it for migration.

Quoted 24 lines · Michael S. Tsirkin

Quoted 7 lines · Gregory Haskins

Quoted 4 lines · Michael S. Tsirkin

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Quoted 9 lines

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Here's what one could do: create a thread for each irqfd, and increment module ref count, put that thread to sleep. When done with irqfd, don't delete it and don't decrement module refcount, wake thread instead. thread kills irqfd and calls module_put_and_exit.

Quoted 1 line

I don't think it's racy

I believe it is. How would you prevent the thread from doing the module_put_and_exit() before the eventfd callback thread is known to have exited the relevant .text section?

All this talk does give me an idea, tho. Ill make a patch.

Quoted 18 lines

Quoted 17 lines

Quoted 6 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Quoted 2 lines

Can this help with DEASSIGN as well? We need it for migration.

No, but afaict you do not need this for migration anyway. Migrate the GSI and re-call kvm_irqfd() on the other side. Would the fd even be relevant across a migration anyway? I would think not, but admittedly I know little about how qemu/kvm migration actually works.

Quoted 29 lines · Gregory Haskins

Quoted 27 lines · Michael S. Tsirkin

Quoted 9 lines · Gregory Haskins

Quoted 5 lines · Michael S. Tsirkin

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Quoted 10 lines

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Here's what one could do: create a thread for each irqfd, and increment module ref count, put that thread to sleep. When done with irqfd, don't delete it and don't decrement module refcount, wake thread instead. thread kills irqfd and calls module_put_and_exit.

Quoted 6 lines

Quoted 2 lines

I don't think it's racy

I believe it is. How would you prevent the thread from doing the module_put_and_exit() before the eventfd callback thread is known to have exited the relevant .text section?

Right.

Quoted 2 lines

All this talk does give me an idea, tho. Ill make a patch.

OK, but ask yourself whether this bag of tricks is worth it, and whether we'll find another hole later. Let's reserve the trickiness for fast-path, where it's needed, and keep at least the assign/deassign simple.

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Quoted 8 lines

Quoted 3 lines

Can this help with DEASSIGN as well? We need it for migration.

No, but afaict you do not need this for migration anyway. Migrate the GSI and re-call kvm_irqfd() on the other side. Would the fd even be relevant across a migration anyway? I would think not, but admittedly I know little about how qemu/kvm migration actually works.

Yes but that's not live migration. For live migration, the trick is that you are running locally but send changes to remote guest. For that, we need to put qemu in the middle between the device and the guest, so it can detect activity and update the remote side.

And the best way to do that is to take poll eventfd that device assigns and push eventfd that kvm polls. To switch between this setup and the one where kvm polls the ventfd from device directly, you need deassign.

Quoted 41 lines · Michael S. Tsirkin

Quoted 29 lines · Gregory Haskins

Quoted 27 lines · Michael S. Tsirkin

Quoted 9 lines · Gregory Haskins

Quoted 5 lines · Michael S. Tsirkin

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Quoted 10 lines

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Here's what one could do: create a thread for each irqfd, and increment module ref count, put that thread to sleep. When done with irqfd, don't delete it and don't decrement module refcount, wake thread instead. thread kills irqfd and calls module_put_and_exit.

Quoted 6 lines

Quoted 2 lines

I don't think it's racy

I believe it is. How would you prevent the thread from doing the module_put_and_exit() before the eventfd callback thread is known to have exited the relevant .text section?

Right.

Quoted 7 lines

Quoted 2 lines

All this talk does give me an idea, tho. Ill make a patch.

OK, but ask yourself whether this bag of tricks is worth it, and whether we'll find another hole later. Let's reserve the trickiness for fast-path, where it's needed, and keep at least the assign/deassign simple.

Understood. OTOH, going back to the model where two steps are needed for close() is ugly too, so I don't want to just give up and revert that fix too easily. At some point we will call it one way or the other, but I am not there quite yet.

Quoted 35 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Quoted 8 lines

Quoted 3 lines

Can this help with DEASSIGN as well? We need it for migration.

No, but afaict you do not need this for migration anyway. Migrate the GSI and re-call kvm_irqfd() on the other side. Would the fd even be relevant across a migration anyway? I would think not, but admittedly I know little about how qemu/kvm migration actually works.

Yes but that's not live migration. For live migration, the trick is that you are running locally but send changes to remote guest. For that, we need to put qemu in the middle between the device and the guest, so it can detect activity and update the remote side.

Quoted 5 lines

And the best way to do that is to take poll eventfd that device assigns and push eventfd that kvm polls. To switch between this setup and the one where kvm polls the ventfd from device directly, you need deassign.

So its still not clear why the distinction between deassign-the-gsi-but-leave-the-fd-valid is needed over a simple close(). Can you elaborate?

Quoted 110 lines · Gregory Haskins

Quoted 49 lines · Michael S. Tsirkin

Quoted 34 lines · Gregory Haskins

Quoted 31 lines · Michael S. Tsirkin

Quoted 11 lines · Gregory Haskins

Quoted 6 lines · Michael S. Tsirkin

BTW, Gregory, this can be used to fix the race in the design: create a thread and let it drop the module reference with module_put_and_exit.

Quoted 11 lines

I had thought of doing something like this initially too, but I think its racy as well. Ultimately, you need to make sure the eventfd callback is completely out before its safe to run, and deferring to a thread would not change this race. The only sane way I can see to do that is to have the caller infrastructure annotate the event somehow (either directly with a module_put(), or indirectly with some kind of state transition that can be tracked with something like synchronize_sched().

Here's what one could do: create a thread for each irqfd, and increment module ref count, put that thread to sleep. When done with irqfd, don't delete it and don't decrement module refcount, wake thread instead. thread kills irqfd and calls module_put_and_exit.

Quoted 8 lines

Quoted 3 lines

I don't think it's racy

I believe it is. How would you prevent the thread from doing the module_put_and_exit() before the eventfd callback thread is known to have exited the relevant .text section?

Right.

Quoted 8 lines

Quoted 3 lines

All this talk does give me an idea, tho. Ill make a patch.

OK, but ask yourself whether this bag of tricks is worth it, and whether we'll find another hole later. Let's reserve the trickiness for fast-path, where it's needed, and keep at least the assign/deassign simple.

Understood. OTOH, going back to the model where two steps are needed for close() is ugly too, so I don't want to just give up and revert that fix too easily. At some point we will call it one way or the other, but I am not there quite yet.

Quoted 46 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 5 lines

Quoted 7 lines

Which will work, but I guess at this point we should ask ourselves whether all the hearburn with srcu, threads and module references is better than just asking the user to call and ioctl.

I am starting to agree with you, here. :)

Note one thing: the SRCU stuff is mostly orthogonal from the rest of the conversation re: the module_put() races. I only tied it into the current thread because the eventfd_notifier_register() thread gave me a convenient way to hook some other context to do the module_put(). In the long term, the srcu changes are for the can_sleep() stuff. So on that note, lets see if I can convince Davide that the srcu stuff is not so evil before we revert the POLLHUP patches, since the module_put() fix is trivial once that is in place.

Quoted 10 lines

Quoted 4 lines

Can this help with DEASSIGN as well? We need it for migration.

No, but afaict you do not need this for migration anyway. Migrate the GSI and re-call kvm_irqfd() on the other side. Would the fd even be relevant across a migration anyway? I would think not, but admittedly I know little about how qemu/kvm migration actually works.

Yes but that's not live migration. For live migration, the trick is that you are running locally but send changes to remote guest. For that, we need to put qemu in the middle between the device and the guest, so it can detect activity and update the remote side.

And the best way to do that is to take poll eventfd that device assigns and push eventfd that kvm polls. To switch between this setup and the one where kvm polls the ventfd from device directly, you need deassign.

Quoted 4 lines

So its still not clear why the distinction between deassign-the-gsi-but-leave-the-fd-valid is needed over a simple close(). Can you elaborate?

The fd needs to be left assigned to the device, so that we can poll the fd and get events, then forward them to kvm.

Quoted 89 lines · Gregory Haskins

Quoted 27 lines · Michael S. Tsirkin

Quoted 24 lines · Gregory Haskins

(quoted diff) +19 -2

+static void
+irqfd_disconnect(struct _irqfd *irqfd)
+{
+	struct kvm *kvm;
+
+	mutex_lock(&irqfd->lock);
+
+	kvm = rcu_dereference(irqfd->kvm);
+	rcu_assign_pointer(irqfd->kvm, NULL);
+
+	mutex_unlock(&irqfd->lock);
+
+	if (!kvm)
+		return;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	list_del(&irqfd->list);
 	mutex_unlock(&kvm->lock);
+
+	/*
+	 * It is important to not drop the kvm reference until the next grace
+	 * period because there might be lockless references in flight up

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +4 -0

+	 * until then
+	 */
+	synchronize_srcu(&irqfd->srcu);
+	kvm_put_kvm(kvm);
 }

So irqfd object will persist after kvm goes away, until eventfd is closed?

Yep, by design. It becomes part of the eventfd and is thus associated with its lifetime. Consider it as if we made our own anon-fd implementation for irqfd and the lifetime looks similar. The difference is that we are reusing eventfd and its interface semantics.

Quoted 37 lines

Quoted 37 lines

static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) {

(quoted diff) +22 -6

 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+	unsigned long flags = (unsigned long)key;
 
-	/*
-	 * The 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->work);
+	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);
+
+	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);
+	}

Quoted 8 lines

Quoted 4 lines

return 0;
}

And it is removed by this function when eventfd is closed. But what prevents the kvm module from going away, meanwhile?

Quoted 8 lines

Well, we hold a reference to struct kvm until we call irqfd_disconnect(). If kvm closes first, we disconnect and disassociate all references to kvm leaving irqfd->kvm = NULL. Likewise, if irqfd closes first, we disassociate with kvm with the above quoted logic. In either case, we are holding a kvm reference up until that "disconnect" point. Therefore kvm should not be able to disappear before that disconnect, and after that point we do not care.

Yes, we do care.

Here's the scenario in more detail:

- kvm is closed
- irq disconnect is called
- kvm is put
- kvm module is removed: all irqs are disconnected
- eventfd closes and triggers callback into removed kvm module
- crash

Quoted 5 lines

If that is not sufficient to prevent kvm.ko from going away in the middle, then IMO kvm_get_kvm() has a bug, not irqfd. ;) However, I believe everything is actually ok here.

Quoted 3 lines

-Greg

BTW, why can't we remove irqfds in kvm_release?

(Applies to kvm.git/master:25deed73)

Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly.

(Applies to kvm.git/master:25deed73)

Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly.

[Review notes:
*) Paul has looked at the SRCU design and, to my knowledge, didn't find
any holes.
*) Michael, Avi, and myself agree that while the removal of the DEASSIGN
vector is not desirable, the fix on close() is more important in the short-term. We can always add DEASSIGN support again in the future with a CAP bit.
]

(Applies to kvm.git/master:25deed73)

Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly.

[Review notes:
*) Paul has looked at the SRCU design and, to my knowledge, didn't find
any holes.
*) Michael, Avi, and myself agree that while the removal of the DEASSIGN
vector is not desirable, the fix on close() is more important in the short-term. We can always add DEASSIGN support again in the future with a CAP bit.
]

[ Resending with correct address for Davide. Pls don't reply
to the original one, you'll get bounces. ]

Quoted 5 lines · Gregory Haskins

(Applies to kvm.git/master:25deed73)

Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly.

Quoted 9 lines

[Review notes:
*) Paul has looked at the SRCU design and, to my knowledge, didn't find
any holes.
*) Michael, Avi, and myself agree that while the removal of the DEASSIGN
vector is not desirable, the fix on close() is more important in the short-term. We can always add DEASSIGN support again in the future with a CAP bit.
]

So, I've been thinking about this, and this approach has another
problem: it depends on pollhup on close which is AFAIK an eventfd-specific feature.

This will prevent us from supporting polling
other useful file types, such as sockets and pipes, down the road, with this interface.

And there's DEASSIGN issue which is needed for migration

and MSI vector
remapping.

I didn't realise these implications when I suggested deassign on close. To me, it now looks like we are better off reverting this patch. We can later add 'deassign on close' support with CAP bit after all :)

Avi, Gregory, what's your take?

Quoted 21 lines · Michael S. Tsirkin

[ Resending with correct address for Davide. Pls don't reply
to the original one, you'll get bounces. ]

Quoted 5 lines · Gregory Haskins

(Applies to kvm.git/master:25deed73)

Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly.

Quoted 10 lines

[Review notes:
*) Paul has looked at the SRCU design and, to my knowledge, didn't find
any holes.
*) Michael, Avi, and myself agree that while the removal of the DEASSIGN
vector is not desirable, the fix on close() is more important in the short-term. We can always add DEASSIGN support again in the future with a CAP bit.
]

Quoted 4 lines

So, I've been thinking about this, and this approach has another
problem: it depends on pollhup on close which is AFAIK an eventfd-specific feature.

Thats ok with me, as we are already married to eventfd for other reasons (see eventfd_fget()).

Quoted 5 lines

This will prevent us from supporting polling
other useful file types, such as sockets and pipes, down the road, with this interface.

I am thinking that we would add explicit support in the future if there are other fd types that might want to also inject interrupts. For instance, perhaps POLLHUP is added to pipes if/when they are patched as
a valid transport for irqfd. Or perhaps irqfd is abstracted such that
eventfd_fget/POLLHUP are eventfd specific assign/deassign implementation details.

Another option is that we s/irqfd/irq-eventfd to leave room for other
interfaces like irq-pollfd, irq-socketfd, etc. IOW, there is no reason
to make the current irqfd code "one-fd-interface to rule them all" per
se. The real abstraction is the kvm_set_irq() + gsi interface anyway. The current irqfd code is a thin shim in front of that. Perhaps each fd type would be better served with code to specifically handle each type,
for its hard to predict what the requirements for translating, say, a
pipe-write into a gsi-inject will be apriori.

Quoted 8 lines

I didn't realise these implications when I suggested deassign on close. To me, it now looks like we are better off reverting this patch. We can later add 'deassign on close' support with CAP bit after all :)

Avi, Gregory, what's your take?

I like the design with the single-call close in place, so my vote is to keep it as it is now.