[libvirt] RFC: libxl race fixes

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Thu, Jan 17, 2013 at 10:29:35PM -0700, Jim Fehlig wrote:
>   
>> Jim Fehlig wrote:
>>     
>>> I've been investigating some races in the libxl driver and would like to
>>> get comments on some potential solutions.
>>>
>>> The first race is in the fd/timeout event handling code, which maps
>>> libxl's osevent interface to libvirt's event loop interface.  This
>>> mapping opens the possibility for libvirt's event loop to invoke
>>> event callbacks after libxl has already deregistered the event,
>>> potentially accessing an event object that has already been freed.
>>>
>>> One solution to this race I've found successful is reference counting the
>>> objects associated with the events.  When libxl registers an event, an
>>> object encapsulating the event is created and it's reference count is set
>>> to 1.  When the event is injected into libvirt's event loop, another
>>> reference is taken on the object.  When libxl deregisters the event, it's
>>> reference count is decremented.  Once the event is removed from libvirt's
>>> event loop, the final reference is decremented and the object is disposed.
>>>   
>>>       
>> While rebasing this solution to use danpb's recent virObjectLockable
>> change, I revisited using only a lock in the libxlDomainObject,
>> acquiring the lock in the registration, deregistration, modify, and
>> callback functions.  I recall some problems with this approach before,
>> but now find it to be sufficient.  Perhaps a misplaced lock drove me to
>> the unneeded complexity of this double lock nonsense...
>>
>>     
>>> This approach ensures the object is not disposed until it is removed
>>> from libvirt's event loop *and* libxl had explicitly deregistered the event.
>>> The notion of an event being 'disabled' found in libvirt's event loop impl
>>> also had to be added to the libxl event object, to ensure the driver doesn't
>>> call into libxl for a previously deregistered event.
>>>
>>> The second race is between destroying a vm (i.e., calling
>>> privateDataFreeFunc, which frees the libxl ctx) and deregistration/cleanup
>>> of all events that have been registered by libxl.
>>>
>>> One solution for this race is to convert libxlDomainObjPrivate to a
>>> virObject and increment its reference for each fd and timeout registration.
>>>   
>>>       
>> Only change here is using the new virObjectLockable.
>>     
>
> I'm surprised that you need to have a separate lock for libxlDomainObjPrivate.
> The lifetime of that object is tied to the lifetime of the virDomainObjPtr,
> which already has a lock. Isn't it sufficient to do locking on the latter,
> whenever the libxlDomainObjPrivate need protecting ?
>   

Yeah, I thought it would be sufficient too, but fd/timeout events can
happen during vm operations where the virDomainObj is locked.  E.g.
during vm creation, the virDomainObj is locked but fd/timeout events
occur as libxl reads/writes to xenstore, sets watches, reads a
kernel/initrd from host, etc.  It seems cleaner to have a lock in the
libxlDomainObjPrivate for accessing libxl's event registrations.

Regards,
Jim