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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list