lkml.org 
[lkml]   [2009]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [KVM PATCH v5 3/4] KVM: Fix races in irqfd using new eventfd_kref_get interface
    On Sun, Jun 28, 2009 at 08:50:28AM -0400, Gregory Haskins wrote:
    > Michael S. Tsirkin wrote:
    > > On Thu, Jun 25, 2009 at 09:28:27AM -0400, Gregory Haskins wrote:
    > >
    > >> @@ -65,25 +134,39 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
    > >> unsigned long flags = (unsigned long)key;
    > >>
    > >> /*
    > >> - * Assume we will be called with interrupts disabled
    > >> + * Called with interrupts disabled
    > >> */
    > >> if (flags & POLLIN)
    > >> - /*
    > >> - * Defer the IRQ injection until later since we need to
    > >> - * acquire the kvm->lock to do so.
    > >> - */
    > >> + /* An event has been signaled, inject an interrupt */
    > >> schedule_work(&irqfd->inject);
    > >>
    > >> if (flags & POLLHUP) {
    > >> - /*
    > >> - * 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
    > >> - */
    > >> + /* The eventfd is closing, detach from KVM */
    > >> + struct kvm *kvm = irqfd->kvm;
    > >> + unsigned long flags;
    > >> +
    > >> __remove_wait_queue(irqfd->wqh, &irqfd->wait);
    > >> - irqfd->wqh = NULL;
    > >> +
    > >> + spin_lock_irqsave(&kvm->irqfds.lock, flags);
    > >> +
    > >> + if (irqfd->active) {
    > >> + /*
    > >> + * If the item is still active we can be sure that
    > >> + * no-one else is trying to shutdown this object at
    > >> + * the same time.
    > >> + *
    > >> + * Defer the shutdown to a thread so we can flush
    > >> + * all remaining inject jobs. We use a slow-work
    > >> + * item to prevent a deadlock against the work-queue
    > >> + */
    > >> + irqfd_deactivate(irqfd);
    > >> + slow_work_enqueue(&irqfd->shutdown);
    > >>
    > >
    > > Greg, in your patch for slow-work module removal, you write:
    > > "Callers must ensure that their module has at least
    > > one reference held while the work is enqueued."
    > > Where does this guarantee come from, in this case?
    > >
    > The general guarantee comes from the fact that modules naturally have to
    > have a reference to be able to call the enqueue function to begin with,
    > or the calling function was already racy. In this particular case, we
    > can guarantee that the kvm vm fd is held while our slow-work is active,
    > and all slow work is flushed before it is released. (I guess I am
    > assuming that VFS takes a module reference when an fd is opened, but I
    > have not verified that it actually does. If it doesn't, I suppose KVM
    > is already racy w.r.t. unloading, independent of my patches)
    >
    > -Greg
    >

    that could be the case, as we have, for example:

    static struct file_operations kvm_vm_fops = {
    .release = kvm_vm_release,
    .unlocked_ioctl = kvm_vm_ioctl,
    .compat_ioctl = kvm_vm_ioctl,
    .mmap = kvm_vm_mmap,
    };

    with no owner field.

    Avi, shouldn't we initialize the owner field to prevent
    kvm module from going away while files are open?

    --
    MST


    \
     
     \ /
      Last update: 2009-06-28 15:21    [W:0.036 / U:1.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site