Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

Gregory Haskins
2009-06-20T02:09:30Z

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

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

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