linux-kernel - Re: [KVM PATCH v2 2/2] kvm: use POLLHUP to close an irqfd instead of an explicit ioctl

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] [day] [month] [year] [list]
Message-ID: <4A3BB7B8.1050509@novell.com>
Date:	Fri, 19 Jun 2009 12:07:20 -0400
From:	Gregory Haskins <ghaskins@...ell.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	Rusty Russell <rusty@...tcorp.com.au>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, avi@...hat.com,
	davidel@...ilserver.org, paulmck@...ux.vnet.ibm.com,
	akpm@...ux-foundation.org
Subject: Re: [KVM PATCH v2 2/2] kvm: use POLLHUP to close an irqfd instead
 of an explicit ioctl

Michael S. Tsirkin wrote:
> On Thu, Jun 18, 2009 at 12:29:31PM -0400, Gregory Haskins wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> On Thu, Jun 18, 2009 at 10:03:36AM -0400, Gregory Haskins wrote:
>>>   
>>>       
>>>> Michael S. Tsirkin wrote:
>>>>     
>>>>         
>>>>> On Thu, Jun 18, 2009 at 08:00:39AM -0400, Gregory Haskins wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> Michael S. Tsirkin wrote:
>>>>>>     
>>>>>>
>>>>>>         
>>>>>>             
>>>>>>> BTW, Gregory, this can be used to fix the race in the design: create a
>>>>>>> thread and let it drop the module reference with module_put_and_exit.
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> I had thought of doing something like this initially too, but I think
>>>>>> its racy as well.  Ultimately, you need to make sure the eventfd
>>>>>> callback is completely out before its safe to run, and deferring to a
>>>>>> thread would not change this race.  The only sane way I can see to do
>>>>>> that is to have the caller infrastructure annotate the event somehow
>>>>>> (either directly with a module_put(), or indirectly with some kind of
>>>>>> state transition that can be tracked with something like
>>>>>> synchronize_sched().
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Here's what one could do: create a thread for each irqfd, and increment
>>>>> module ref count, put that thread to sleep.  When done with
>>>>> irqfd, don't delete it and don't decrement module refcount, wake thread
>>>>> instead.  thread kills irqfd and calls module_put_and_exit.
>>>>>
>>>>> I don't think it's racy
>>>>>       
>>>>>           
>>>> I believe it is. How would you prevent the thread from doing the
>>>> module_put_and_exit() before the eventfd callback thread is known to
>>>> have exited the relevant .text section?
>>>>     
>>>>         
>>> Right.
>>>
>>>   
>>>       
>>>> All this talk does give me an idea, tho.  Ill make a patch.
>>>>     
>>>>         
>>> OK, but ask yourself whether this bag of tricks is worth it, and whether
>>> we'll find another hole later. Let's reserve the trickiness for
>>> fast-path, where it's needed, and keep at least the assign/deassign simple.
>>>   
>>>       
>> Understood.  OTOH, going back to the model where two steps are needed
>> for close() is ugly too, so I don't want to just give up and revert that
>> fix too easily.  At some point we will call it one way or the other, but
>> I am not there quite yet.
>>     
>>>   
>>>       
>>>>>   
>>>>>       
>>>>>           
>>>>>>> Which will work, but I guess at this point we should ask ourselves
>>>>>>> whether all the hearburn with srcu, threads and module references is
>>>>>>> better than just asking the user to call and ioctl.
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> I am starting to agree with you, here. :)
>>>>>>
>>>>>> Note one thing: the SRCU stuff is mostly orthogonal from the rest of the
>>>>>> conversation re: the module_put() races.  I only tied it into the
>>>>>> current thread because the eventfd_notifier_register() thread gave me a
>>>>>> convenient way to hook some other context to do the module_put().  In
>>>>>> the long term, the srcu changes are for the can_sleep() stuff.  So on
>>>>>> that note, lets see if I can convince Davide that the srcu stuff is not
>>>>>> so evil before we revert the POLLHUP patches, since the module_put() fix
>>>>>> is trivial once that is in place.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Can this help with DEASSIGN as well? We need it for migration.
>>>>>   
>>>>>       
>>>>>           
>>>> No, but afaict you do not need this for migration anyway.  Migrate the
>>>> GSI and re-call kvm_irqfd() on the other side.  Would the fd even be
>>>> relevant across a migration anyway?  I would think not, but admittedly I
>>>> know little about how qemu/kvm migration actually works.
>>>>     
>>>>         
>>> Yes but that's not live migration. For live migration, the trick is that
>>> you are running locally but send changes to remote guest.  For that, we
>>> need to put qemu in the middle between the device and the guest, so it
>>> can detect activity and update the remote side.
>>>
>>> And the best way to do that is to take poll eventfd that device assigns
>>> and push eventfd that kvm polls. To switch between this setup
>>> and the one where kvm polls the ventfd from device directly,
>>> you need deassign.
>>>   
>>>       
>> So its still not clear why the distinction between
>> deassign-the-gsi-but-leave-the-fd-valid is needed over a simple
>> close().  Can you elaborate?
>>     
>
>
> The fd needs to be left assigned to the device, so that we can poll
> the fd and get events, then forward them to kvm.
>   

Ah, ok.  Now I get what you are trying to do.

Well, per the PM I sent you this morning, I figured out the magic to
resolve the locking issues.  So we should be able to add DEASSIGN logic
soon, I hope.

-Greg



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