[libvirt] libvirtd deadlock on shutdown

[Daniel P. Berrange]

On Wed, Jun 20, 2012 at 03:31:02PM -0600, Eric Blake wrote:
> On 06/20/2012 03:07 PM, Jim Fehlig wrote:
> > I'm looking into a libvirtd deadlock on daemon shutdown.  The deadlock
> > occurs when shutting down virNetServer.  If the handling of a job is in
> > flight in virNetServerHandleJob(), the virNetServer lock is acquired
> > when freeing job->prog (src/rpc/virnetserver.c:167).  But the lock is
> > already held in virNetServerFree(), which is blocked in
> > virThreadPoolFree() waiting for all the workers to finish.  No progress
> > can be made.
> > 
> > The attached hack fixes the problem, but I'm not convinced this is an
> > appropriate fix.  Is it necessary to hold the virNetServer lock when
> > calling virNetServerProgramFree(job->prog)?  I notice the lock is not
> > held in the error path of virNetServerHandleJob().
> > 
> 
> > +++ b/src/rpc/virnetserver.c
> > @@ -774,7 +774,9 @@ void virNetServerFree(virNetServerPtr srv)
> >      for (i = 0 ; i < srv->nservices ; i++)
> >          virNetServerServiceToggle(srv->services[i], false);
> >  
> > +    virNetServerUnlock(srv);
> >      virThreadPoolFree(srv->workers);
> > +    virNetServerLock(srv);
> 
> As written, this can't be right, because it reads a field of srv outside
> the locks.  But maybe you meant:
> 
> <type> tmp = srv->workers;
> srv->workers = NULL;
> virNetServerUnlock(srv);
> virThreadPoolFree(tmp);
> virNetServerLock(srv);
> 
> as a possible fix that doesn't violate the safety rules of reading
> fields from srv outside a lock.

While you are technically correct in the most general sense, there
is not actually any harm in dereferencing  srv->workers here, since
now the ref count is 0, only the current thread & the workers are
allowed to be accessing 'srv', an the workers don't touch the
'srv->workers' field at all.

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