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

Re: [libvirt] PATCH: Fix misc locking bugs, and add lock checker



On Thu, May 14, 2009 at 10:19:46PM +0200, Daniel Veillard wrote:
> On Thu, May 14, 2009 at 05:53:41PM +0100, Daniel P. Berrange wrote:
> > This patch fixes a number of locking bugs, some serious, some not.
> > 
> > It also adds the CIL lock checker I previously wrote. It is improved since
> > last time, because it explicitly looks for the virDriver static variables
> > and uses that to extract list of functions to be checked. This removes a
> > huge number of false positives. It also now checks for a couple of dead
> > lock scenarios, in addition to previous checks for lock ordering correctness.
> > 
> > The serious locking bugs 
> > 
> >  - qemudDomainMigratePrepare2: didn't unlock VM causing deadlock
> >  - storagePoolRefresh: unlocked the driver too early
> >  - storageVolumeCreateXML: locked the pool without having locked driver
> >  - umlDomainGetAutostart: missing unlock call with later deadlock
> > 
> [...]
> > --- a/src/network_driver.c	Thu May 14 17:38:11 2009 +0100
> > +++ b/src/network_driver.c	Thu May 14 17:47:01 2009 +0100
> > @@ -1217,7 +1217,6 @@ static int networkStart(virNetworkPtr ne
> >  
> >      networkDriverLock(driver);
> >      network = virNetworkFindByUUID(&driver->networks, net->uuid);
> > -    networkDriverUnlock(driver);
> >  
> >      if (!network) {
> >          networkReportError(net->conn, NULL, net, VIR_ERR_INVALID_NETWORK,
> > @@ -1230,6 +1229,7 @@ static int networkStart(virNetworkPtr ne
> >  cleanup:
> >      if (network)
> >          virNetworkObjUnlock(network);
> > +    networkDriverUnlock(driver);
> >      return ret;
> >  }
> 
>   A lot of the changes which are not in that list look like the above,
> instead of unlocking immediately after finding the network structure,
> the unlocking is postponed until the end of the operation. What's the
> reasonning ? Is that a safety against possible changes in the functions
> i.e. just a preventive change or is there something we expected to be
> safe which turns out not ?

It is 

> 
> > @@ -1240,7 +1240,6 @@ static int networkDestroy(virNetworkPtr 
> > @@ -1349,7 +1349,6 @@ static int networkSetAutostart(virNetwor
> > @@ -2229,7 +2230,6 @@ static int qemudDomainSuspend(virDomainP
> > @@ -2282,7 +2280,6 @@ static int qemudDomainResume(virDomainPt
> > @@ -3153,7 +3148,6 @@ static int qemudDomainGetSecurityLabel(v
> > @@ -3617,7 +3612,6 @@ static int qemudDomainStart(virDomainPtr
> > @@ -4144,7 +4136,6 @@ static int qemudDomainAttachDevice(virDo
> > @@ -4296,7 +4288,6 @@ static int qemudDomainDetachDevice(virDo
> > @@ -4359,7 +4351,6 @@ static int qemudDomainSetAutostart(virDo
> > @@ -792,7 +792,6 @@ storagePoolRefresh(virStoragePoolPtr obj
> > @@ -939,7 +939,6 @@ storagePoolSetAutostart(virStoragePoolPt
> > @@ -1553,7 +1553,6 @@ static int umlDomainStart(virDomainPtr d
> > @@ -1684,7 +1684,6 @@ static int umlDomainSetAutostart(virDoma
> 
>   they all share the pattern of changes above

The QEMU ones are all similar - the virDomainSaveStatus method is
taking a 'driver' parameter, meaning we need to keep the lock on
'driver' held for longer. I don't really like this, and think it
would be better to remove the driver parameter from this method.
We're holding coarse locks for far too long already.

> but a hint about the systematic changes about extended locking would
> be good :-)

Here is an example of what the test reports as error, without the
fixes applied. It should help understand the changes I made


================================================================
Function: umlDomainStart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 1564 "uml_driver.c"
ret = umlStartVMDaemon(dom->conn, driver___0, vm);
================================================================


================================================================
Function: umlDomainGetAutostart
----------------------------------------------------------------
  - Total exit points with locked vars: 1
  - At exit on #line 1675
return (ret);
^^^^^^^^^
    variables still locked are
      | struct uml_driver * driver___0
  - Total blocks with lock ordering mistakes: 0
================================================================


================================================================
Function: umlDomainSetAutostart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 4
  - Driver used while unlocked on #line 1704
configFile = virDomainConfigFile(dom->conn,
                                 (char const   *)driver___0->configDir,
                                 (char const   *)(vm->def)->name);
  - Driver used while unlocked on #line 1706
autostartLink = virDomainConfigFile(dom->conn,
                                    (char const   *)driver___0->autostartDir,
                                    (char const   *)(vm->def)->name);
  - Driver used while unlocked on #line 1712
err = virFileMakePath((char const   *)driver___0->autostartDir);
  - Driver used while unlocked on #line 1713
virReportSystemErrorFull(dom->conn, 21, err, "uml_driver.c",
                         "umlDomainSetAutostart", 1715U,
                         (char const   *)tmp___1, driver___0->autostartDir);
================================================================


================================================================
Function: testOpen
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 3
  - Driver used while unlocked on #line 663 "test.c"
tmp___20 = virAlloc((void *)(& privconn->domainEventCallbacks),
                    sizeof(*(privconn->domainEventCallbacks)));
  - Driver used while unlocked on #line 663
privconn->domainEventQueue = virDomainEventQueueNew();
  - Driver used while unlocked on #line 670
privconn->domainEventTimer = virEventAddTimeout(-1, & testDomainEventFlush,
                                                (void *)privconn,
                                                (void (*)(void *opaque ))((void *)0));
================================================================


================================================================
Function: qemudClose
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 1815 "qemu_driver.c"
virDomainEventCallbackListRemoveConn(conn, driver___0->domainEventCallbacks);
================================================================


================================================================
Function: qemudDomainSuspend
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 2259
tmp___4 = qemudSaveDomainStatus(dom->conn, driver___0, vm);
================================================================


================================================================
Function: qemudDomainResume
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 2312
tmp___4 = qemudSaveDomainStatus(dom->conn, driver___0, vm);
================================================================


================================================================
Function: qemudDomainGetSecurityLabel
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 3189
tmp___2 = (*((driver___0->securityDriver)->domainGetSecurityLabel))(dom->conn,
                                                                    vm, seclabel);
================================================================


================================================================
Function: qemudDomainStart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 3630
ret = qemudStartVMDaemon(dom->conn, driver___0, vm, (char const   *)((void *)0),
                         -1);
================================================================


================================================================
Function: qemudDomainAttachDevice
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 4192
tmp___8 = qemudSaveDomainStatus(dom->conn, driver___0, vm);
================================================================


================================================================
Function: qemudDomainDetachDevice
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 4316
tmp___3 = qemudSaveDomainStatus(dom->conn, driver___0, vm);
================================================================


================================================================
Function: qemudDomainSetAutostart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 4
  - Driver used while unlocked on #line 4381
configFile = virDomainConfigFile(dom->conn,
                                 (char const   *)driver___0->configDir,
                                 (char const   *)(vm->def)->name);
  - Driver used while unlocked on #line 4383
autostartLink = virDomainConfigFile(dom->conn,
                                    (char const   *)driver___0->autostartDir,
                                    (char const   *)(vm->def)->name);
  - Driver used while unlocked on #line 4389
err = virFileMakePath((char const   *)driver___0->autostartDir);
  - Driver used while unlocked on #line 4390
virReportSystemErrorFull(dom->conn, 10, err, "qemu_driver.c",
                         "qemudDomainSetAutostart", 4392U,
                         (char const   *)tmp___1, driver___0->autostartDir);
================================================================


================================================================
Function: qemudDomainMigratePrepare2
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Object fetched while locked objects exist #line 4990
vm = virDomainAssignDef(dconn, & driver___0->domains, def);
================================================================


================================================================
Function: storagePoolRefresh
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 827 "storage_driver.c"
virStoragePoolObjRemove(& driver___0->pools, pool);
================================================================


================================================================
Function: storagePoolSetAutostart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 2
  - Driver used while unlocked on #line 962
err = virFileMakePath((char const   *)driver___0->autostartDir);
  - Driver used while unlocked on #line 963
virReportSystemErrorFull(obj->conn, 18, err, "storage_driver.c",
                         "storagePoolSetAutostart", 965U,
                         (char const   *)tmp___1, driver___0->autostartDir);
================================================================


================================================================
Function: storageVolumeCreateXML
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Object locked while driver is unlocked on #line 1277
virStoragePoolObjLock(pool);
================================================================


================================================================
Function: networkStart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 1228 "network_driver.c"
ret = networkStartNetworkDaemon(net->conn, driver___0, network);
================================================================


================================================================
Function: networkDestroy
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 1
  - Driver used while unlocked on #line 1251
ret = networkShutdownNetworkDaemon(net->conn, driver___0, network);
================================================================


================================================================
Function: networkSetAutostart
----------------------------------------------------------------
  - Total exit points with locked vars: 0
  - Total blocks with lock ordering mistakes: 4
  - Driver used while unlocked on #line 1363
configFile = virNetworkConfigFile(net->conn,
                                  (char const   *)driver___0->networkConfigDir,
                                  (char const   *)(network->def)->name);
  - Driver used while unlocked on #line 1365
autostartLink = virNetworkConfigFile(net->conn,
                                     (char const   *)driver___0->networkAutostartDir,
                                     (char const   *)(network->def)->name);
  - Driver used while unlocked on #line 1371
err = virFileMakePath((char const   *)driver___0->networkAutostartDir);
  - Driver used while unlocked on #line 1372
virReportSystemErrorFull(net->conn, 19, *tmp___1, "network_driver.c",
                         "networkSetAutostart", 1374U, (char const   *)tmp___0,
                         driver___0->networkAutostartDir);
================================================================


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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