Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister

[Jim Fehlig <jfehlig@suse.com>]

Ian Campbell wrote:
> On Fri, 2013-01-11 at 17:51 +0000, Jim Fehlig wrote:
>   
>> Ian Campbell wrote:
>>     
>>> On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote:
>>>   
>>>       
>>>> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"):
>>>>     
>>>>         
>>>>> I'm not surprised that the original patch makes Bamvor's symptoms go
>>>>> away.  Bamvor had one of the possible races (the fd-related one) but
>>>>> not the other.
>>>>>       
>>>>>           
>>>> Here (followups to this message, shortly) is v3 of my two-patch series
>>>> which after conversation with Ian C I think fully fixes the race, and
>>>> which I have tested now.
>>>>     
>>>>         
>>> Is this version now tested and ready to be applied?
>>>   
>>>       
>> Hi Ian,
>>
>> I have been doing quite a bit of testing with this version, but have one
>> remaining issue wrt races between the libvirt libxl driver and libxl. 
>> Earlier in this thread you mentioned this potential solution
>>
>> "The other scheme which springs to mind is to do reference counting, with
>> the application holding a reference whenever the event is present in its
>> event loop (such that there is any chance of the event being generated)
>> and libxl holding a reference while it considers the event to be active"
>>
>> I thought this was a good approach, particularly since libvirt has
>> excellent support for it.  When libxl registers an fd/timer, I create an
>> object containing the details with an initial reference count of 1.  If
>> the fd/timer is successfully injected into libvirt's event loop, I take
>> another reference on the object.  The object is only destroyed after
>> libxl has deregistered the fd/timer *and* it has been removed from
>> libvirt's event loop.  For each fd/timer object, I also increment the
>> reference count on my libxl_ctx object.  This approach works well IMO. 
>> It ensures the libxl_ctx exists for the life of all fd/timer objects.
>>     
>
> Is taking a reference count on the ctx for each fd/timer strictly
> necessary?
>
> You can guarantee that the ctx lifetime is greater than the fd/timer
> lifetime because if you were to destroy the ctx then it would teardown
> the fd/timer as part of ctx_free (I think? More of an Ian J question).
>   

Yes, but the teardown of timers in particular is asynchronous.  libxl
calls the modify timeout hook with abs_t of {0,0}, the timer fires on
next iteration of event loop invoking the callback, which calls
libxl_osevent_occurred_timeout() to finally cleanup the timeout on the
libxl side.  But in the meantime, the associated ctx has been freed. 
Taking a ref count on the ctx avoids this race.
> Without those extra references I think the problem you describe below
> doesn't happen.
>   

Right, but then the ctx disappears before all fds/timers have been
cleaned up.

>   
>> The only wrench in this machinery is that watch_efd is not deregistered
>> until calling libxl_ctx_free().  But I never get to that point since
>> that fd registration holds a reference on my libxl_ctx :(.  My first
>> thought was to cleanup/deregister that fd on domain death, but I didn't
>> have much success creating a patch.  Perhaps I should look at that again...
>>     
>
> I'd be worried about libxl internal uses of this watch which you cannot
> easily control preventing you from doing this.
>   

Agreed :/.

Jim