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.