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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4A3C44DA.7000503@novell.com>
Date:	Fri, 19 Jun 2009 22:09:30 -0400
From:	Gregory Haskins <ghaskins@...ell.com>
To:	Davide Libenzi <davidel@...ilserver.org>
CC:	mst@...hat.com, kvm@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	avi@...hat.com, paulmck@...ux.vnet.ibm.com,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [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



Download attachment "signature.asc" of type "application/pgp-signature" (267 bytes)