[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] RFC: libxl race fixes



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 ?


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]