lkml.org 
[lkml]   [2009]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions
    Davide Libenzi wrote:
    > On Fri, 19 Jun 2009, Davide Libenzi wrote:
    >
    >
    >> On Fri, 19 Jun 2009, Gregory Haskins wrote:
    >>
    >>
    >>> I am fairly confident it is not that simple after having thought about
    >>> this issue over the last few days. But I've been wrong in the past.
    >>> Propose a patch and I will review it for races/correctness, if you
    >>> like. Perhaps a combination of that plus your asymmetrical locking
    >>> scheme would work. One of the challenges you will hit is avoiding ABBA
    >>> between your "get" lock and the wqh, but good luck!
    >>>
    >> A patch for what? The eventfd patch is a one-liner.
    >>

    Yes, understood. What I was trying to gently say is that the one-liner
    proposal alone is still broken afaict. However, if there is another
    solution that works that you like better than 133-liner I posted, I am
    more than willing to help analyze it. In the end, I only care that this
    is fixed.

    >> It seems hard to believe that the thing cannot be handled on your side.
    >> Once the wake_up_locked() is turned into a wake_up(), what other races are
    >> there?
    >>
    >
    > AFAICS, the IRQfd code simply registers the callback to ->poll() and waits
    > for two events.
    > In the POLLIN event, you schedule_work(&irqfd->inject) and there are no
    > races there AFAICS (you basically do not care of anything eventfd memory
    > related at all).
    > For POLLHUP, you do:
    >
    > spin_lock(irqfd->slock);
    > if (irqfd->wqh)
    > schedule_work(&irqfd->inject);
    > irqfd->wqh = NULL;
    > spin_unlock(irqfd->slock);
    >
    > In your work function you notice the POLLHUP condition and take proper
    > action (dunno what it is in your case).
    > In your kvm_irqfd_release() function:
    >
    > spin_lock(irqfd->slock);
    > if (irqfd->wqh)
    > remove_wait_queue(irqfd->wqh, &irqfd->wait);
    > irqfd->wqh = NULL;
    > spin_unlock(irqfd->slock);
    >
    > Any races in there?
    >

    Yes, for one you have an ABBA deadlock on the irqfd->slock and wqh->lock
    (recall wqh has to be locked to fix that other race I mentioned).

    (As a hint, I think I fixed 4-5 races with these patches, so there are a
    few others still lurking as well)

    -Greg


    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2009-06-20 04:13    [W:9.091 / U:0.452 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site