[PATCH 0/3] qemu_capabilities: Reword domain caps cache
It's fairly easy to reproduce the bug (see its description in 3/3). Just put a sleep here: diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..eeae967420 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1399,6 +1399,7 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver, key = g_strdup_printf("%d:%d:%s:%s", data.arch, data.virttype, NULLSTR(data.machine), NULLSTR(data.path)); + sleep(5); if (virHashAddEntry(domCapsCache, key, domCaps) < 0) return NULL; } And then try to get domcaps from two consoles at once: virsh domcapabilities One will succeed, the other will fail with an error. Michal Prívozník (3): cpu.c: Check properly for virCapabilitiesGetNodeInfo() retval qemu_conf: Avoid dereferencing NULL in virQEMUDriverGetHost{NUMACaps,CPU} qemu_capabilities: Reword domain caps cache src/cpu/cpu.c | 2 +- src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 14 ++++- src/qemu/qemu_conf.c | 86 +++++++++---------------- 4 files changed, 155 insertions(+), 65 deletions(-) -- 2.24.1
The virCapabilitiesGetNodeInfo() function has the usual return value semantics for integeres: a negative value means an error, zero or a positive value means success. However, the function call done in virCPUProbeHost() doesn't check for the return value accordingly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index d9288cc85a..ae3a0acc10 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -456,7 +456,7 @@ virCPUProbeHost(virArch arch) { virNodeInfo nodeinfo; - if (virCapabilitiesGetNodeInfo(&nodeinfo)) + if (virCapabilitiesGetNodeInfo(&nodeinfo) < 0) return NULL; return virCPUGetHost(arch, VIR_CPU_TYPE_HOST, &nodeinfo, NULL); -- 2.24.1
On Fri, Jan 24, 2020 at 11:27:16 +0100, Michal Privoznik wrote:
The virCapabilitiesGetNodeInfo() function has the usual return value semantics for integeres: a negative value means an error, zero or a positive value means success. However, the function call done in virCPUProbeHost() doesn't check for the return value accordingly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When fixing [1] I've ran attached reproducer and had it spawn 1024 threads and query capabilities XML in each one of them. This lead libvirtd to hit the RLIMIT_NOFILE limit which was kind of expected. What wasn't expected was a subsequent segfault. It happened because virCPUProbeHost failed and returned NULL. We've taken the NULL and passed it to virCapabilitiesHostNUMARef() which dereferenced it. Code inspection showed the same flas in virQEMUDriverGetHostNUMACaps(), so I'm fixing both places. 1: https://bugzilla.redhat.com/show_bug.cgi?id=1791790 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_conf.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..8040f8ab3a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1201,32 +1201,41 @@ virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver, virCapsHostNUMAPtr virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver) { + virCapsHostNUMAPtr hostnuma; + qemuDriverLock(driver); if (!driver->hostnuma) driver->hostnuma = virCapabilitiesHostNUMANewHost(); + hostnuma = driver->hostnuma; qemuDriverUnlock(driver); - virCapabilitiesHostNUMARef(driver->hostnuma); + if (hostnuma) + virCapabilitiesHostNUMARef(hostnuma); - return driver->hostnuma; + return hostnuma; } virCPUDefPtr virQEMUDriverGetHostCPU(virQEMUDriverPtr driver) { + virCPUDefPtr hostcpu; + qemuDriverLock(driver); if (!driver->hostcpu) driver->hostcpu = virCPUProbeHost(virArchFromHost()); + hostcpu = driver->hostcpu; + qemuDriverUnlock(driver); - virCPUDefRef(driver->hostcpu); + if (hostcpu) + virCPUDefRef(hostcpu); - return driver->hostcpu; + return hostcpu; } -- 2.24.1
On Fri, Jan 24, 2020 at 11:27:17 +0100, Michal Privoznik wrote:
When fixing [1] I've ran attached reproducer and had it spawn 1024 threads and query capabilities XML in each one of them. This lead libvirtd to hit the RLIMIT_NOFILE limit which was kind of expected. What wasn't expected was a subsequent segfault. It happened because virCPUProbeHost failed and returned NULL. We've taken the NULL and passed it to virCapabilitiesHostNUMARef() which dereferenced it. Code inspection showed the same flas in virQEMUDriverGetHostNUMACaps(), so I'm fixing both places.
1: https://bugzilla.redhat.com/show_bug.cgi?id=1791790
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_conf.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com> Consider fixing the whitespace discrepancy though:
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..8040f8ab3a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1201,32 +1201,41 @@ virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver, virCapsHostNUMAPtr virQEMUDriverGetHostNUMACaps(virQEMUDriverPtr driver) { + virCapsHostNUMAPtr hostnuma; + qemuDriverLock(driver);
if (!driver->hostnuma) driver->hostnuma = virCapabilitiesHostNUMANewHost();
+ hostnuma = driver->hostnuma; qemuDriverUnlock(driver);
No newline before unlock.
- virCapabilitiesHostNUMARef(driver->hostnuma); + if (hostnuma) + virCapabilitiesHostNUMARef(hostnuma);
- return driver->hostnuma; + return hostnuma; }
virCPUDefPtr virQEMUDriverGetHostCPU(virQEMUDriverPtr driver) { + virCPUDefPtr hostcpu; + qemuDriverLock(driver);
if (!driver->hostcpu) driver->hostcpu = virCPUProbeHost(virArchFromHost());
+ hostcpu = driver->hostcpu; + qemuDriverUnlock(driver);
Newline before unlock.
- virCPUDefRef(driver->hostcpu); + if (hostcpu) + virCPUDefRef(hostcpu);
- return driver->hostcpu; + return hostcpu; }
-- 2.24.1
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> --- src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 14 ++++- src/qemu/qemu_conf.c | 69 +++++--------------- 3 files changed, 141 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 17b0134ab8..ef3bfb7093 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -598,6 +598,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 @@ -629,7 +649,7 @@ struct _virQEMUCaps { virArch arch; - virHashTablePtr domCapsCache; + virQEMUDomainCapsCachePtr domCapsCache; size_t ngicCapabilities; virGICCapability *gicCapabilities; @@ -655,6 +675,9 @@ static int virQEMUCapsOnceInit(void) if (!VIR_CLASS_NEW(virQEMUCaps, virClassForObject())) return -1; + if (!(VIR_CLASS_NEW(virQEMUDomainCapsCache, virClassForObjectLockable()))) + return -1; + return 0; } @@ -1625,6 +1648,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 @@ -1642,7 +1681,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; @@ -1832,7 +1871,7 @@ void virQEMUCapsDispose(void *obj) { virQEMUCapsPtr qemuCaps = obj; - virHashFree(qemuCaps->domCapsCache); + virObjectUnref(qemuCaps->domCapsCache); virBitmapFree(qemuCaps->flags); VIR_FREE(qemuCaps->package); @@ -1992,9 +2031,78 @@ 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, + virQEMUCapsGetDomainCapsCacheFiller fillDomainCaps, + void *opaque) +{ + 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 ((fillDomainCaps)(tempDomCaps, qemuCaps, opaque) < 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 ebfdb4b981..395d4afaee 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -575,7 +575,19 @@ const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCaps); virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps); unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps); const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps); -virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps); + +typedef int (*virQEMUCapsGetDomainCapsCacheFiller)(virDomainCapsPtr domCaps, + virQEMUCapsPtr qemuCaps, + void *opaque); + +virDomainCapsPtr +virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps, + const char *machine, + virArch arch, + virDomainVirtType virttype, + virQEMUCapsGetDomainCapsCacheFiller fillDomainCaps, + void *opaque); + 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 8040f8ab3a..e0cb8be675 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1337,31 +1337,20 @@ 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) +virQEMUDriverGetDomainCapabilitiesFiller(virDomainCapsPtr domCaps, + virQEMUCapsPtr qemuCaps, + 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; + virQEMUDriverPtr driver = opaque; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - return 0; + return virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps, + driver->privileged, + cfg->firmwares, cfg->nfirmwares); } + /** * virQEMUDriverGetDomainCapabilities: * @@ -1380,40 +1369,12 @@ 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, + virQEMUDriverGetDomainCapabilitiesFiller, + driver); } -- 2.24.1
On Fri, Jan 24, 2020 at 11:27:18 +0100, Michal Privoznik wrote:
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> --- src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 14 ++++- src/qemu/qemu_conf.c | 69 +++++--------------- 3 files changed, 141 insertions(+), 60 deletions(-)
[...]
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8040f8ab3a..e0cb8be675 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1337,31 +1337,20 @@ 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) +virQEMUDriverGetDomainCapabilitiesFiller(virDomainCapsPtr domCaps, + virQEMUCapsPtr qemuCaps, + 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; + virQEMUDriverPtr driver = opaque; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- return 0; + return virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps, + driver->privileged, + cfg->firmwares, cfg->nfirmwares);
This is weird. You have +virQEMUDriverGetDomainCapabilitiesFiller in qemu_conf.c which internally uses virQEMUCapsFillDomainCaps exported by qemu_capabilities.c and pass it to virQEMUCapsGetDomainCapsCache which is defined in qemu_capabilities.c. Can't we just get rid of the callback? The rest looks good.
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> --- diff to v1: - Dropped the callback in favor of passing necessary values as arguments 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 17b0134ab8..4d7008396e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -598,6 +598,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 @@ -629,7 +649,7 @@ struct _virQEMUCaps { virArch arch; - virHashTablePtr domCapsCache; + virQEMUDomainCapsCachePtr domCapsCache; size_t ngicCapabilities; virGICCapability *gicCapabilities; @@ -655,6 +675,9 @@ static int virQEMUCapsOnceInit(void) if (!VIR_CLASS_NEW(virQEMUCaps, virClassForObject())) return -1; + if (!(VIR_CLASS_NEW(virQEMUDomainCapsCache, virClassForObjectLockable()))) + return -1; + return 0; } @@ -1625,6 +1648,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 @@ -1642,7 +1681,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; @@ -1832,7 +1871,7 @@ void virQEMUCapsDispose(void *obj) { virQEMUCapsPtr qemuCaps = obj; - virHashFree(qemuCaps->domCapsCache); + virObjectUnref(qemuCaps->domCapsCache); virBitmapFree(qemuCaps->flags); VIR_FREE(qemuCaps->package); @@ -1992,9 +2031,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 ebfdb4b981..e110805456 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -575,7 +575,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 1204b189fa..e500da1515 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.24.1
On Fri, Jan 24, 2020 at 13:40:03 +0100, Michal Privoznik wrote:
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> ---
diff to v1: - Dropped the callback in favor of passing necessary values as arguments
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa