Re: [KVM PATCH v5 3/4] KVM: Fix races in irqfd using new eventfd_kref_get interface

[Gregory Haskins <ghaskins@novell.com>]

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

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


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