lkml.org 
[lkml]   [2009]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[KVM PATCH v5 1/4] kvm: prepare irqfd for having interrupts disabled during eventfd->release
    Date
    We need to plug some race conditions on eventfd shutdown.  In order to
    do this, we need to change the context in which the release notification
    is delivered so that the wqh lock is now held. However, there is currently
    code in the release callback that assumes it can sleep.

    We have a slight chicken and egg problem where we cant fix the race
    without adding the lock, and we can't add the lock without breaking
    the sleepy code. Normally we could deal with this by making both
    changes in an atomic changeset. However, we want to keep the eventfd
    and kvm specific changes isolated to ease the reviewer burden on upstream
    eventfd (at the specific request of upstream). Therefore, we have this
    intermediate patch.

    This intermediate patch allows the release() method to work in an atomic
    context, at the expense of correctness w.r.t. memory-leaks. Today we have
    a race condition. With this patch applied we leak. Both issues will be
    resolved later in the series. It is the author's opinion that a leak is
    better for bisectability than the hang would be should we leave the sleepy
    code in place after the locking changeover.

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

    virt/kvm/eventfd.c | 89 ++++++++++++++++------------------------------------
    1 files changed, 27 insertions(+), 62 deletions(-)

    diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
    index a9e7de7..9656027 100644
    --- a/virt/kvm/eventfd.c
    +++ b/virt/kvm/eventfd.c
    @@ -28,7 +28,6 @@
    #include <linux/file.h>
    #include <linux/list.h>
    #include <linux/eventfd.h>
    -#include <linux/srcu.h>

    /*
    * --------------------------------------------------------------------
    @@ -38,8 +37,6 @@
    * --------------------------------------------------------------------
    */
    struct _irqfd {
    - struct mutex lock;
    - struct srcu_struct srcu;
    struct kvm *kvm;
    int gsi;
    struct list_head list;
    @@ -53,48 +50,12 @@ static void
    irqfd_inject(struct work_struct *work)
    {
    struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
    - struct kvm *kvm;
    - int idx;
    + struct kvm *kvm = irqfd->kvm;

    - idx = srcu_read_lock(&irqfd->srcu);
    -
    - kvm = rcu_dereference(irqfd->kvm);
    - if (kvm) {
    - mutex_lock(&kvm->irq_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);
    - mutex_unlock(&kvm->irq_lock);
    - }
    -
    - srcu_read_unlock(&irqfd->srcu, idx);
    -}
    -
    -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);
    - 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
    - */
    - synchronize_srcu(&irqfd->srcu);
    - kvm_put_kvm(kvm);
    + mutex_lock(&kvm->irq_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);
    + mutex_unlock(&kvm->irq_lock);
    }

    static int
    @@ -103,26 +64,24 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
    struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
    unsigned long flags = (unsigned long)key;

    + /*
    + * Assume we will be called with interrupts disabled
    + */
    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.
    + * 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()
    + * for now, just remove ourselves from the list and let
    + * the rest dangle. We will fix this up later once
    + * the races in eventfd are fixed
    */
    - remove_wait_queue(irqfd->wqh, &irqfd->wait);
    - flush_work(&irqfd->inject);
    - irqfd_disconnect(irqfd);
    -
    - cleanup_srcu_struct(&irqfd->srcu);
    - kfree(irqfd);
    + __remove_wait_queue(irqfd->wqh, &irqfd->wait);
    + irqfd->wqh = NULL;
    }

    return 0;
    @@ -150,8 +109,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
    if (!irqfd)
    return -ENOMEM;

    - mutex_init(&irqfd->lock);
    - init_srcu_struct(&irqfd->srcu);
    irqfd->kvm = kvm;
    irqfd->gsi = gsi;
    INIT_LIST_HEAD(&irqfd->list);
    @@ -172,8 +129,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)

    events = file->f_op->poll(file, &irqfd->pt);

    - kvm_get_kvm(kvm);
    -
    mutex_lock(&kvm->lock);
    list_add_tail(&irqfd->list, &kvm->irqfds);
    mutex_unlock(&kvm->lock);
    @@ -211,6 +166,16 @@ kvm_irqfd_release(struct kvm *kvm)
    {
    struct _irqfd *irqfd, *tmp;

    - list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list)
    - irqfd_disconnect(irqfd);
    + list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
    + if (irqfd->wqh)
    + remove_wait_queue(irqfd->wqh, &irqfd->wait);
    +
    + flush_work(&irqfd->inject);
    +
    + mutex_lock(&kvm->lock);
    + list_del(&irqfd->list);
    + mutex_unlock(&kvm->lock);
    +
    + kfree(irqfd);
    + }
    }


    \
     
     \ /
      Last update: 2009-06-25 15:31    [W:0.028 / U:0.652 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site