From b155913f796b313ce969ae318beb66e3e35d13dc Mon Sep 17 00:00:00 2001

Message-Id: <b155913f796b313ce969ae318beb66e3e35d13dc@dist-git>

From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>

Date: Thu, 11 Jun 2020 18:21:26 +0100

Subject: [PATCH] nodedev: fix race in API usage vs initial device enumeration

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

During startup the udev node device driver impl uses a background thread

to populate the list of devices to avoid blocking the daemon startup

entirely. There is no synchronization to the public APIs, so it is

possible for an application to start calling APIs before the device

initialization is complete.

This was not a problem in the old approach where libvirtd was started

on boot, as initialization would easily complete before any APIs were

called.

With the use of socket activation, however, APIs are invoked from the

very moment the daemon starts. This is easily seen by doing a

  'virsh -c nodedev:///system list'

the first time it runs it will only show one or two devices. The second

time it runs it will show all devices. The solution is to introduce a

flag and condition variable for APIs to synchronize against before

returning any data.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

(cherry picked from commit 008abeb03c262149b756ad5a226ff6cbc5e37e2c)

https://bugzilla.redhat.com/show_bug.cgi?id=1846237

https://bugzilla.redhat.com/show_bug.cgi?id=1845459

Message-Id: <20200611172126.269081-2-berrange@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/conf/virnodedeviceobj.h          |  2 ++

 src/node_device/node_device_driver.c | 44 ++++++++++++++++++++++++++++

 src/node_device/node_device_hal.c    | 15 ++++++++++

 src/node_device/node_device_udev.c   | 13 ++++++++

 4 files changed, 74 insertions(+)

diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h

index c4d3c55d73..c9df8dedab 100644

--- a/src/conf/virnodedeviceobj.h

+++ b/src/conf/virnodedeviceobj.h

@@ -36,6 +36,8 @@ typedef struct _virNodeDeviceDriverState virNodeDeviceDriverState;

 typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr;

 struct _virNodeDeviceDriverState {

     virMutex lock;

+    virCond initCond;

+    bool initialized;

     /* pid file FD, ensures two copies of the driver can't use the same root */

     int lockFD;

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c

index b630be4399..a894f7265f 100644

--- a/src/node_device/node_device_driver.c

+++ b/src/node_device/node_device_driver.c

@@ -162,6 +162,22 @@ nodeDeviceUnlock(void)

 }

+static int

+nodeDeviceWaitInit(void)

+{

+    nodeDeviceLock();

+    while (!driver->initialized) {

+        if (virCondWait(&driver->initCond, &driver->lock) < 0) {

+            virReportSystemError(errno, "%s",

+                                 _("failed to wait on condition"));

+            nodeDeviceUnlock();

+            return -1;

+        }

+    }

+    nodeDeviceUnlock();

+    return 0;

+}

+

 int

 nodeNumOfDevices(virConnectPtr conn,

                  const char *cap,

@@ -172,6 +188,9 @@ nodeNumOfDevices(virConnectPtr conn,

     virCheckFlags(0, -1);

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,

                                             virNodeNumOfDevicesCheckACL);

 }

@@ -189,6 +208,9 @@ nodeListDevices(virConnectPtr conn,

     virCheckFlags(0, -1);

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     return virNodeDeviceObjListGetNames(driver->devs, conn,

                                         virNodeListDevicesCheckACL,

                                         cap, names, maxnames);

@@ -205,6 +227,9 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,

     if (virConnectListAllNodeDevicesEnsureACL(conn) < 0)

         return -1;

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     return virNodeDeviceObjListExport(conn, driver->devs, devices,

                                       virConnectListAllNodeDevicesCheckACL,

                                       flags);

@@ -234,6 +259,9 @@ nodeDeviceLookupByName(virConnectPtr conn,

     virNodeDeviceDefPtr def;

     virNodeDevicePtr device = NULL;

+    if (nodeDeviceWaitInit() < 0)

+        return NULL;

+

     if (!(obj = nodeDeviceObjFindByName(name)))

         return NULL;

     def = virNodeDeviceObjGetDef(obj);

@@ -262,6 +290,9 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,

     virCheckFlags(0, NULL);

+    if (nodeDeviceWaitInit() < 0)

+        return NULL;

+

     if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs,

                                                        wwnn, wwpn)))

         return NULL;

@@ -475,6 +506,10 @@ nodeDeviceCreateXML(virConnectPtr conn,

     const char *virt_type = NULL;

     virCheckFlags(0, NULL);

+

+    if (nodeDeviceWaitInit() < 0)

+        return NULL;

+

     virt_type  = virConnectGetType(conn);

     if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))

@@ -519,6 +554,9 @@ nodeDeviceDestroy(virNodeDevicePtr device)

     char *wwnn = NULL, *wwpn = NULL;

     unsigned int parent_host;

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     if (!(obj = nodeDeviceObjFindByName(device->name)))

         return -1;

     def = virNodeDeviceObjGetDef(obj);

@@ -574,6 +612,9 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,

     if (virConnectNodeDeviceEventRegisterAnyEnsureACL(conn) < 0)

         return -1;

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     if (virNodeDeviceEventStateRegisterID(conn, driver->nodeDeviceEventState,

                                           device, eventID, callback,

                                           opaque, freecb, &callbackID) < 0)

@@ -590,6 +631,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,

     if (virConnectNodeDeviceEventDeregisterAnyEnsureACL(conn) < 0)

         return -1;

+    if (nodeDeviceWaitInit() < 0)

+        return -1;

+

     if (virObjectEventStateDeregisterID(conn,

                                         driver->nodeDeviceEventState,

                                         callbackID, true) < 0)

diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c

index 4cef7c2c12..0d48895b66 100644

--- a/src/node_device/node_device_hal.c

+++ b/src/node_device/node_device_hal.c

@@ -603,6 +603,15 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED,

         VIR_FREE(driver);

         return VIR_DRV_STATE_INIT_ERROR;

     }

+

+    if (virCondInit(&driver->initCond) < 0) {

+        virReportSystemError(errno, "%s",

+                             _("Unable to initialize condition variable"));

+        virMutexDestroy(&driver->lock);

+        VIR_FREE(driver);

+        return VIR_DRV_STATE_INIT_ERROR;

+    }

+

     nodeDeviceLock();

     if (privileged) {

@@ -693,6 +702,11 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED,

     }

     VIR_FREE(udi);

+    nodeDeviceLock();

+    driver->initialized = true;

+    nodeDeviceUnlock();

+    virCondBroadcast(&driver->initCond);

+

     return VIR_DRV_STATE_INIT_COMPLETE;

  failure:

@@ -725,6 +739,7 @@ nodeStateCleanup(void)

         VIR_FREE(driver->stateDir);

         nodeDeviceUnlock();

+        virCondDestroy(&driver->initCond);

         virMutexDestroy(&driver->lock);

         VIR_FREE(driver);

         return 0;

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c

index 4b33dc25d8..ae3d081e66 100644

--- a/src/node_device/node_device_udev.c

+++ b/src/node_device/node_device_udev.c

@@ -1476,6 +1476,7 @@ nodeStateCleanup(void)

         virPidFileRelease(driver->stateDir, "driver", driver->lockFD);

     VIR_FREE(driver->stateDir);

+    virCondDestroy(&driver->initCond);

     virMutexDestroy(&driver->lock);

     VIR_FREE(driver);

@@ -1743,6 +1744,11 @@ nodeStateInitializeEnumerate(void *opaque)

     if (udevEnumerateDevices(udev) != 0)

         goto error;

+    nodeDeviceLock();

+    driver->initialized = true;

+    nodeDeviceUnlock();

+    virCondBroadcast(&driver->initCond);

+

     return;

  error:

@@ -1798,6 +1804,13 @@ nodeStateInitialize(bool privileged,

         VIR_FREE(driver);

         return VIR_DRV_STATE_INIT_ERROR;

     }

+    if (virCondInit(&driver->initCond) < 0) {

+        virReportSystemError(errno, "%s",

+                             _("Unable to initialize condition variable"));

+        virMutexDestroy(&driver->lock);

+        VIR_FREE(driver);

+        return VIR_DRV_STATE_INIT_ERROR;

+    }

     driver->privileged = privileged;

-- 

2.27.0