render / rpms / libvirt

Forked from rpms/libvirt 10 months ago
Clone
fbe740
From b155913f796b313ce969ae318beb66e3e35d13dc Mon Sep 17 00:00:00 2001
fbe740
Message-Id: <b155913f796b313ce969ae318beb66e3e35d13dc@dist-git>
fbe740
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
fbe740
Date: Thu, 11 Jun 2020 18:21:26 +0100
fbe740
Subject: [PATCH] nodedev: fix race in API usage vs initial device enumeration
fbe740
MIME-Version: 1.0
fbe740
Content-Type: text/plain; charset=UTF-8
fbe740
Content-Transfer-Encoding: 8bit
fbe740
fbe740
During startup the udev node device driver impl uses a background thread
fbe740
to populate the list of devices to avoid blocking the daemon startup
fbe740
entirely. There is no synchronization to the public APIs, so it is
fbe740
possible for an application to start calling APIs before the device
fbe740
initialization is complete.
fbe740
fbe740
This was not a problem in the old approach where libvirtd was started
fbe740
on boot, as initialization would easily complete before any APIs were
fbe740
called.
fbe740
fbe740
With the use of socket activation, however, APIs are invoked from the
fbe740
very moment the daemon starts. This is easily seen by doing a
fbe740
fbe740
  'virsh -c nodedev:///system list'
fbe740
fbe740
the first time it runs it will only show one or two devices. The second
fbe740
time it runs it will show all devices. The solution is to introduce a
fbe740
flag and condition variable for APIs to synchronize against before
fbe740
returning any data.
fbe740
fbe740
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
fbe740
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
fbe740
(cherry picked from commit 008abeb03c262149b756ad5a226ff6cbc5e37e2c)
fbe740
fbe740
https://bugzilla.redhat.com/show_bug.cgi?id=1846237
fbe740
https://bugzilla.redhat.com/show_bug.cgi?id=1845459
fbe740
Message-Id: <20200611172126.269081-2-berrange@redhat.com>
fbe740
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
fbe740
---
fbe740
 src/conf/virnodedeviceobj.h          |  2 ++
fbe740
 src/node_device/node_device_driver.c | 44 ++++++++++++++++++++++++++++
fbe740
 src/node_device/node_device_hal.c    | 15 ++++++++++
fbe740
 src/node_device/node_device_udev.c   | 13 ++++++++
fbe740
 4 files changed, 74 insertions(+)
fbe740
fbe740
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
fbe740
index c4d3c55d73..c9df8dedab 100644
fbe740
--- a/src/conf/virnodedeviceobj.h
fbe740
+++ b/src/conf/virnodedeviceobj.h
fbe740
@@ -36,6 +36,8 @@ typedef struct _virNodeDeviceDriverState virNodeDeviceDriverState;
fbe740
 typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr;
fbe740
 struct _virNodeDeviceDriverState {
fbe740
     virMutex lock;
fbe740
+    virCond initCond;
fbe740
+    bool initialized;
fbe740
 
fbe740
     /* pid file FD, ensures two copies of the driver can't use the same root */
fbe740
     int lockFD;
fbe740
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
fbe740
index b630be4399..a894f7265f 100644
fbe740
--- a/src/node_device/node_device_driver.c
fbe740
+++ b/src/node_device/node_device_driver.c
fbe740
@@ -162,6 +162,22 @@ nodeDeviceUnlock(void)
fbe740
 }
fbe740
 
fbe740
 
fbe740
+static int
fbe740
+nodeDeviceWaitInit(void)
fbe740
+{
fbe740
+    nodeDeviceLock();
fbe740
+    while (!driver->initialized) {
fbe740
+        if (virCondWait(&driver->initCond, &driver->lock) < 0) {
fbe740
+            virReportSystemError(errno, "%s",
fbe740
+                                 _("failed to wait on condition"));
fbe740
+            nodeDeviceUnlock();
fbe740
+            return -1;
fbe740
+        }
fbe740
+    }
fbe740
+    nodeDeviceUnlock();
fbe740
+    return 0;
fbe740
+}
fbe740
+
fbe740
 int
fbe740
 nodeNumOfDevices(virConnectPtr conn,
fbe740
                  const char *cap,
fbe740
@@ -172,6 +188,9 @@ nodeNumOfDevices(virConnectPtr conn,
fbe740
 
fbe740
     virCheckFlags(0, -1);
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,
fbe740
                                             virNodeNumOfDevicesCheckACL);
fbe740
 }
fbe740
@@ -189,6 +208,9 @@ nodeListDevices(virConnectPtr conn,
fbe740
 
fbe740
     virCheckFlags(0, -1);
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     return virNodeDeviceObjListGetNames(driver->devs, conn,
fbe740
                                         virNodeListDevicesCheckACL,
fbe740
                                         cap, names, maxnames);
fbe740
@@ -205,6 +227,9 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
fbe740
     if (virConnectListAllNodeDevicesEnsureACL(conn) < 0)
fbe740
         return -1;
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     return virNodeDeviceObjListExport(conn, driver->devs, devices,
fbe740
                                       virConnectListAllNodeDevicesCheckACL,
fbe740
                                       flags);
fbe740
@@ -234,6 +259,9 @@ nodeDeviceLookupByName(virConnectPtr conn,
fbe740
     virNodeDeviceDefPtr def;
fbe740
     virNodeDevicePtr device = NULL;
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return NULL;
fbe740
+
fbe740
     if (!(obj = nodeDeviceObjFindByName(name)))
fbe740
         return NULL;
fbe740
     def = virNodeDeviceObjGetDef(obj);
fbe740
@@ -262,6 +290,9 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
fbe740
 
fbe740
     virCheckFlags(0, NULL);
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return NULL;
fbe740
+
fbe740
     if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs,
fbe740
                                                        wwnn, wwpn)))
fbe740
         return NULL;
fbe740
@@ -475,6 +506,10 @@ nodeDeviceCreateXML(virConnectPtr conn,
fbe740
     const char *virt_type = NULL;
fbe740
 
fbe740
     virCheckFlags(0, NULL);
fbe740
+
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return NULL;
fbe740
+
fbe740
     virt_type  = virConnectGetType(conn);
fbe740
 
fbe740
     if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
fbe740
@@ -519,6 +554,9 @@ nodeDeviceDestroy(virNodeDevicePtr device)
fbe740
     char *wwnn = NULL, *wwpn = NULL;
fbe740
     unsigned int parent_host;
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     if (!(obj = nodeDeviceObjFindByName(device->name)))
fbe740
         return -1;
fbe740
     def = virNodeDeviceObjGetDef(obj);
fbe740
@@ -574,6 +612,9 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
fbe740
     if (virConnectNodeDeviceEventRegisterAnyEnsureACL(conn) < 0)
fbe740
         return -1;
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     if (virNodeDeviceEventStateRegisterID(conn, driver->nodeDeviceEventState,
fbe740
                                           device, eventID, callback,
fbe740
                                           opaque, freecb, &callbackID) < 0)
fbe740
@@ -590,6 +631,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
fbe740
     if (virConnectNodeDeviceEventDeregisterAnyEnsureACL(conn) < 0)
fbe740
         return -1;
fbe740
 
fbe740
+    if (nodeDeviceWaitInit() < 0)
fbe740
+        return -1;
fbe740
+
fbe740
     if (virObjectEventStateDeregisterID(conn,
fbe740
                                         driver->nodeDeviceEventState,
fbe740
                                         callbackID, true) < 0)
fbe740
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
fbe740
index 4cef7c2c12..0d48895b66 100644
fbe740
--- a/src/node_device/node_device_hal.c
fbe740
+++ b/src/node_device/node_device_hal.c
fbe740
@@ -603,6 +603,15 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED,
fbe740
         VIR_FREE(driver);
fbe740
         return VIR_DRV_STATE_INIT_ERROR;
fbe740
     }
fbe740
+
fbe740
+    if (virCondInit(&driver->initCond) < 0) {
fbe740
+        virReportSystemError(errno, "%s",
fbe740
+                             _("Unable to initialize condition variable"));
fbe740
+        virMutexDestroy(&driver->lock);
fbe740
+        VIR_FREE(driver);
fbe740
+        return VIR_DRV_STATE_INIT_ERROR;
fbe740
+    }
fbe740
+
fbe740
     nodeDeviceLock();
fbe740
 
fbe740
     if (privileged) {
fbe740
@@ -693,6 +702,11 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED,
fbe740
     }
fbe740
     VIR_FREE(udi);
fbe740
 
fbe740
+    nodeDeviceLock();
fbe740
+    driver->initialized = true;
fbe740
+    nodeDeviceUnlock();
fbe740
+    virCondBroadcast(&driver->initCond);
fbe740
+
fbe740
     return VIR_DRV_STATE_INIT_COMPLETE;
fbe740
 
fbe740
  failure:
fbe740
@@ -725,6 +739,7 @@ nodeStateCleanup(void)
fbe740
 
fbe740
         VIR_FREE(driver->stateDir);
fbe740
         nodeDeviceUnlock();
fbe740
+        virCondDestroy(&driver->initCond);
fbe740
         virMutexDestroy(&driver->lock);
fbe740
         VIR_FREE(driver);
fbe740
         return 0;
fbe740
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
fbe740
index 4b33dc25d8..ae3d081e66 100644
fbe740
--- a/src/node_device/node_device_udev.c
fbe740
+++ b/src/node_device/node_device_udev.c
fbe740
@@ -1476,6 +1476,7 @@ nodeStateCleanup(void)
fbe740
         virPidFileRelease(driver->stateDir, "driver", driver->lockFD);
fbe740
 
fbe740
     VIR_FREE(driver->stateDir);
fbe740
+    virCondDestroy(&driver->initCond);
fbe740
     virMutexDestroy(&driver->lock);
fbe740
     VIR_FREE(driver);
fbe740
 
fbe740
@@ -1743,6 +1744,11 @@ nodeStateInitializeEnumerate(void *opaque)
fbe740
     if (udevEnumerateDevices(udev) != 0)
fbe740
         goto error;
fbe740
 
fbe740
+    nodeDeviceLock();
fbe740
+    driver->initialized = true;
fbe740
+    nodeDeviceUnlock();
fbe740
+    virCondBroadcast(&driver->initCond);
fbe740
+
fbe740
     return;
fbe740
 
fbe740
  error:
fbe740
@@ -1798,6 +1804,13 @@ nodeStateInitialize(bool privileged,
fbe740
         VIR_FREE(driver);
fbe740
         return VIR_DRV_STATE_INIT_ERROR;
fbe740
     }
fbe740
+    if (virCondInit(&driver->initCond) < 0) {
fbe740
+        virReportSystemError(errno, "%s",
fbe740
+                             _("Unable to initialize condition variable"));
fbe740
+        virMutexDestroy(&driver->lock);
fbe740
+        VIR_FREE(driver);
fbe740
+        return VIR_DRV_STATE_INIT_ERROR;
fbe740
+    }
fbe740
 
fbe740
     driver->privileged = privileged;
fbe740
 
fbe740
-- 
fbe740
2.27.0
fbe740