From dbc6ca0acac24c12b30b74b706c848489f008d71 Mon Sep 17 00:00:00 2001

Message-Id: <dbc6ca0acac24c12b30b74b706c848489f008d71@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Fri, 24 Jan 2020 15:05:51 +0100

Subject: [PATCH] qemu_capabilities: Rework domain caps cache

Since v5.6.0-48-g270583ed98 we try to cache domain capabilities,

i.e. store filled virDomainCaps in a hash table in virQEMUCaps

for future use. However, there's a race condition in the way it's

implemented. We use virQEMUCapsGetDomainCapsCache() to obtain the

pointer to the hash table, then we search the hash table for

cached data and if none is found the domcaps is constructed and

put into the table. Problem is that this is all done without any

locking, so if there are two threads trying to do the same, one

will succeed and the other will fail inserting the data into the

table.

Also, the API looks a bit fishy - obtaining pointer to the hash

table is dangerous.

The solution is to use a mutex that guards the whole operation

with the hash table. Then, the API can be changes to return

virDomainCapsPtr directly.

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

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

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

(cherry picked from commit c76009313f8068c848cad6cb517daf42e6716bb9)

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

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

Message-Id: <48a4b2f9ab1e8157e4b7baf1b506e90861a39308.1579874719.git.mprivozn@redhat.com>

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

---

 src/qemu/qemu_capabilities.c | 122 +++++++++++++++++++++++++++++++++--

 src/qemu/qemu_capabilities.h |  12 +++-

 src/qemu/qemu_conf.c         |  65 +++----------------

 3 files changed, 136 insertions(+), 63 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c

index 84c62a4e28..edefb70309 100644

--- a/src/qemu/qemu_capabilities.c

+++ b/src/qemu/qemu_capabilities.c

@@ -594,6 +594,26 @@ struct _virQEMUCapsAccel {

     qemuMonitorCPUDefsPtr cpuModels;

 };

+

+typedef struct _virQEMUDomainCapsCache virQEMUDomainCapsCache;

+typedef virQEMUDomainCapsCache *virQEMUDomainCapsCachePtr;

+struct _virQEMUDomainCapsCache {

+    virObjectLockable parent;

+

+    virHashTablePtr cache;

+};

+

+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDomainCapsCache, virObjectUnref);

+

+static virClassPtr virQEMUDomainCapsCacheClass;

+static void virQEMUDomainCapsCacheDispose(void *obj)

+{

+    virQEMUDomainCapsCachePtr cache = obj;

+

+    virHashFree(cache->cache);

+}

+

+

 /*

  * Update the XML parser/formatter when adding more

  * information to this struct so that it gets cached

@@ -625,7 +645,7 @@ struct _virQEMUCaps {

     virArch arch;

-    virHashTablePtr domCapsCache;

+    virQEMUDomainCapsCachePtr domCapsCache;

     size_t ngicCapabilities;

     virGICCapability *gicCapabilities;

@@ -651,6 +671,9 @@ static int virQEMUCapsOnceInit(void)

     if (!VIR_CLASS_NEW(virQEMUCaps, virClassForObject()))

         return -1;

+    if (!(VIR_CLASS_NEW(virQEMUDomainCapsCache, virClassForObjectLockable())))

+        return -1;

+

     return 0;

 }

@@ -1620,6 +1643,22 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps,

 }

+static virQEMUDomainCapsCachePtr

+virQEMUDomainCapsCacheNew(void)

+{

+    g_autoptr(virQEMUDomainCapsCache) cache = NULL;

+

+    if (virQEMUCapsInitialize() < 0)

+        return NULL;

+

+    if (!(cache = virObjectLockableNew(virQEMUDomainCapsCacheClass)))

+        return NULL;

+

+    if (!(cache->cache = virHashCreate(5, virObjectFreeHashData)))

+        return NULL;

+

+    return g_steal_pointer(&cache);

+}

 virQEMUCapsPtr

@@ -1637,7 +1676,7 @@ virQEMUCapsNew(void)

     if (!(qemuCaps->flags = virBitmapNew(QEMU_CAPS_LAST)))

         goto error;

-    if (!(qemuCaps->domCapsCache = virHashCreate(5, virObjectFreeHashData)))

+    if (!(qemuCaps->domCapsCache = virQEMUDomainCapsCacheNew()))

         goto error;

     return qemuCaps;

@@ -1827,7 +1866,7 @@ void virQEMUCapsDispose(void *obj)

 {

     virQEMUCapsPtr qemuCaps = obj;

-    virHashFree(qemuCaps->domCapsCache);

+    virObjectUnref(qemuCaps->domCapsCache);

     virBitmapFree(qemuCaps->flags);

     VIR_FREE(qemuCaps->package);

@@ -1987,9 +2026,82 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps)

 }

-virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps)

+struct virQEMUCapsSearchDomcapsData {

+    const char *path;

+    const char *machine;

+    virArch arch;

+    virDomainVirtType virttype;

+};

+

+

+static int

+virQEMUCapsSearchDomcaps(const void *payload,

+                         const void *name G_GNUC_UNUSED,

+                         const void *opaque)

 {

-    return qemuCaps->domCapsCache;

+    virDomainCapsPtr domCaps = (virDomainCapsPtr) payload;

+    struct virQEMUCapsSearchDomcapsData *data = (struct virQEMUCapsSearchDomcapsData *) opaque;

+

+    if (STREQ_NULLABLE(data->path, domCaps->path) &&

+        STREQ_NULLABLE(data->machine, domCaps->machine) &&

+        data->arch == domCaps->arch &&

+        data->virttype == domCaps->virttype)

+        return 1;

+

+    return 0;

+}

+

+

+virDomainCapsPtr

+virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps,

+                              const char *machine,

+                              virArch arch,

+                              virDomainVirtType virttype,

+                              virArch hostarch,

+                              bool privileged,

+                              virFirmwarePtr *firmwares,

+                              size_t nfirmwares)

+{

+    virQEMUDomainCapsCachePtr cache = qemuCaps->domCapsCache;

+    virDomainCapsPtr domCaps = NULL;

+    const char *path = virQEMUCapsGetBinary(qemuCaps);

+    struct virQEMUCapsSearchDomcapsData data = {

+        .path = path,

+        .machine = machine,

+        .arch = arch,

+        .virttype = virttype,

+    };

+

+    virObjectLock(cache);

+

+    domCaps = virHashSearch(cache->cache, virQEMUCapsSearchDomcaps, &data, NULL);

+

+    if (!domCaps) {

+        g_autoptr(virDomainCaps) tempDomCaps = NULL;

+        g_autofree char *key = NULL;

+

+        /* hash miss, build new domcaps */

+        if (!(tempDomCaps = virDomainCapsNew(path, machine,

+                                             arch, virttype)))

+            goto cleanup;

+

+        if (virQEMUCapsFillDomainCaps(qemuCaps, hostarch, tempDomCaps,

+                                      privileged, firmwares, nfirmwares) < 0)

+            goto cleanup;

+

+        key = g_strdup_printf("%d:%d:%s:%s", arch, virttype,

+                              NULLSTR(machine), path);

+

+        if (virHashAddEntry(cache->cache, key, tempDomCaps) < 0)

+            goto cleanup;

+

+        domCaps = g_steal_pointer(&tempDomCaps);

+    }

+

+    virObjectRef(domCaps);

+ cleanup:

+    virObjectUnlock(cache);

+    return domCaps;

 }

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h

index 193c19fc81..d76c1dbfa9 100644

--- a/src/qemu/qemu_capabilities.h

+++ b/src/qemu/qemu_capabilities.h

@@ -571,7 +571,17 @@ const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCaps);

 virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps);

 unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps);

 const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps);

-virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps);

+

+virDomainCapsPtr

+virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps,

+                              const char *machine,

+                              virArch arch,

+                              virDomainVirtType virttype,

+                              virArch hostarch,

+                              bool privileged,

+                              virFirmwarePtr *firmwares,

+                              size_t nfirmwares);

+

 unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps);

 int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,

                                  virDomainVirtType type,

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c

index e33ef4895e..029996427e 100644

--- a/src/qemu/qemu_conf.c

+++ b/src/qemu/qemu_conf.c

@@ -1338,31 +1338,6 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,

 }

-struct virQEMUDriverSearchDomcapsData {

-    const char *path;

-    const char *machine;

-    virArch arch;

-    virDomainVirtType virttype;

-};

-

-

-static int

-virQEMUDriverSearchDomcaps(const void *payload,

-                           const void *name G_GNUC_UNUSED,

-                           const void *opaque)

-{

-    virDomainCapsPtr domCaps = (virDomainCapsPtr) payload;

-    struct virQEMUDriverSearchDomcapsData *data = (struct virQEMUDriverSearchDomcapsData *) opaque;

-

-    if (STREQ_NULLABLE(data->path, domCaps->path) &&

-        STREQ_NULLABLE(data->machine, domCaps->machine) &&

-        data->arch == domCaps->arch &&

-        data->virttype == domCaps->virttype)

-        return 1;

-

-    return 0;

-}

-

 /**

  * virQEMUDriverGetDomainCapabilities:

  *

@@ -1381,40 +1356,16 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,

                                    virArch arch,

                                    virDomainVirtType virttype)

 {

-    g_autoptr(virDomainCaps) domCaps = NULL;

     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);

-    virHashTablePtr domCapsCache = virQEMUCapsGetDomainCapsCache(qemuCaps);

-    struct virQEMUDriverSearchDomcapsData data = {

-        .path = virQEMUCapsGetBinary(qemuCaps),

-        .machine = machine,

-        .arch = arch,

-        .virttype = virttype,

-    };

-    domCaps = virHashSearch(domCapsCache,

-                            virQEMUDriverSearchDomcaps, &data, NULL);

-    if (!domCaps) {

-        g_autofree char *key = NULL;

-

-        /* hash miss, build new domcaps */

-        if (!(domCaps = virDomainCapsNew(data.path, data.machine,

-                                         data.arch, data.virttype)))

-            return NULL;

-

-        if (virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps,

-                                      driver->privileged,

-                                      cfg->firmwares, cfg->nfirmwares) < 0)

-            return NULL;

-

-        key = g_strdup_printf("%d:%d:%s:%s", data.arch, data.virttype,

-                              NULLSTR(data.machine), NULLSTR(data.path));

-

-        if (virHashAddEntry(domCapsCache, key, domCaps) < 0)

-            return NULL;

-    }

-

-    virObjectRef(domCaps);

-    return g_steal_pointer(&domCaps);

+    return virQEMUCapsGetDomainCapsCache(qemuCaps,

+                                         machine,

+                                         arch,

+                                         virttype,

+                                         driver->hostarch,

+                                         driver->privileged,

+                                         cfg->firmwares,

+                                         cfg->nfirmwares);

 }

-- 

2.25.0