[libvirt] [PATCH] Release VM lock before acquiring virDomainObjListPtr lock

[Daniel P. Berrange]

On Tue, Apr 09, 2013 at 12:22:38PM +0200, Peter Krempa wrote:
> On 04/09/13 11:08, Daniel P. Berrange wrote:
> >On Mon, Apr 08, 2013 at 09:15:28PM -0600, Eric Blake wrote:
> >>On 02/11/2013 09:46 AM, Daniel P. Berrange wrote:
> >>>From: "Daniel P. Berrange" <berrange at redhat.com>
> >>>
> >>>When removing a VM from the virDomainObjListPtr, we must not
> >>>be holding the VM lock while acquiring the list lock. Re-order
> >>>code to ensure that we can release the VM lock early.
> >>>---
> >>>  src/conf/domain_conf.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >>>index 5e16ddf..d92e54a 100644
> >>>--- a/src/conf/domain_conf.c
> >>>+++ b/src/conf/domain_conf.c
> >>>@@ -2115,11 +2115,10 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
> >>>  {
> >>>      char uuidstr[VIR_UUID_STRING_BUFLEN];
> >>>
> >>>-    virObjectLock(doms);
> >>>      virUUIDFormat(dom->def->uuid, uuidstr);
> >>>-
> >>>      virObjectUnlock(dom);
> >>>
> >>>+    virObjectLock(doms);
> >>
> >>This patch seems to be implicated in Peter's latest proof of a
> >>use-after-free data race:
> >>https://www.redhat.com/archives/libvir-list/2013-April/msg00674.html
> >
> >The caller of this method should own a reference on virDomainObjPtr.
> >This method will result in that reference being released, as the
> >dom is removed from the list.
> >
> >If any other thread experiances a use-after-free, then that thread
> >must have been missing a reference.
> 
> Actually, by default no reference updating is being done when the
> domain object is taken from the list. The only place where
> references are updated on domain objects is monitor code, right
> before the locks are dropped.
> 
> I'm working on patches that adds reference tracking when the domain
> object pointer is acquired from the list to avoid this kind of
> issue.

Well that's a big change in the locking model. I agree it would actually
make sense to do that, but I think we need a more targetted fix for this
particular problem first which is practical to backport to stable
releases.


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 :|