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

[Gregory Haskins]

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: signature.asc
Description: OpenPGP digital signature