[libvirt] [PATCH 0/4] qemu: fix usage of disk and storage source private data
There were a few cases where we'd use private data for a storage source which was not allocated. Fix it by checking whether it is allocated. Qemu command line parser allocated disk definition via VIR_ALLOC rather than the proper function. This resulted into the disk private data strucutre not being allocated. That one is required. Peter Krempa (4): qemu: parse: Allocate disk definition with private data qemu: Tolerate storage source private data being NULL qemu: domain: Don't allocate storage source private data if not needed qemu: process: Setup disk secrets when preparing disks src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- src/qemu/qemu_parse_command.c | 4 +--- src/qemu/qemu_process.c | 14 +++++++++----- 5 files changed, 44 insertions(+), 23 deletions(-) -- 2.14.3
Use virDomainDiskDefNew instead of VIR_ALLOC in qemuParseCommandLineDisk. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510781 --- src/qemu/qemu_parse_command.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 533316385..c7c7bba03 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -658,10 +658,8 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, 0) < 0) return NULL; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDiskDefNew(xmlopt))) goto cleanup; - if (VIR_ALLOC(def->src) < 0) - goto error; if (qemuDomainIsPSeries(dom)) def->bus = VIR_DOMAIN_DISK_BUS_SCSI; -- 2.14.3
In some cases it does not make sense to pursue that the private data will be allocated (especially when we don't need to put anything in it). Ensure that the code works without it. This also fixes few crashes pointed out in https://bugzilla.redhat.com/show_bug.cgi?id=1510323 --- src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 364196783..49a7b92fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1501,12 +1501,17 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, { int actualType = virStorageSourceGetActualType(disk->src); qemuDomainStorageSourcePrivatePtr srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); - qemuDomainSecretInfoPtr secinfo = srcpriv->secinfo; - qemuDomainSecretInfoPtr encinfo = srcpriv->encinfo; + qemuDomainSecretInfoPtr secinfo = NULL; + qemuDomainSecretInfoPtr encinfo = NULL; virJSONValuePtr srcprops = NULL; char *source = NULL; int ret = -1; + if (srcpriv) { + secinfo = srcpriv->secinfo; + encinfo = srcpriv->encinfo; + } + if (qemuDiskSourceNeedsProps(disk->src) && !(srcprops = qemuDiskSourceGetProps(disk->src))) goto cleanup; @@ -2183,8 +2188,13 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, bool driveBoot = false; virDomainDiskDefPtr disk = def->disks[i]; qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); - qemuDomainSecretInfoPtr secinfo = srcPriv->secinfo; - qemuDomainSecretInfoPtr encinfo = srcPriv->encinfo; + qemuDomainSecretInfoPtr secinfo = NULL; + qemuDomainSecretInfoPtr encinfo = NULL; + + if (srcPriv) { + secinfo = srcPriv->secinfo; + encinfo = srcPriv->encinfo; + } if (disk->info.bootIndex) { bootindex = disk->info.bootIndex; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ce63b4a4d..72a57d89e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -259,6 +259,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + qemuDomainSecretInfoPtr secinfo = NULL; const char *format = NULL; char *sourcestr = NULL; @@ -268,6 +269,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; } + if (srcPriv) + secinfo = srcPriv->secinfo; + if (disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -300,7 +304,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } if (!virStorageSourceIsEmpty(newsrc)) { - if (qemuGetDriveSourceString(newsrc, srcPriv->secinfo, &sourcestr) < 0) + if (qemuGetDriveSourceString(newsrc, secinfo, &sourcestr) < 0) goto error; if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) { @@ -371,8 +375,8 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; qemuDomainStorageSourcePrivatePtr srcPriv; - qemuDomainSecretInfoPtr secinfo; - qemuDomainSecretInfoPtr encinfo; + qemuDomainSecretInfoPtr secinfo = NULL; + qemuDomainSecretInfoPtr encinfo = NULL; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -384,13 +388,16 @@ qemuDomainAttachDiskGeneric(virConnectPtr conn, goto error; srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); - secinfo = srcPriv->secinfo; + if (srcPriv) { + secinfo = srcPriv->secinfo; + encinfo = srcPriv->encinfo; + } + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto error; } - encinfo = srcPriv->encinfo; if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; -- 2.14.3
--- src/qemu/qemu_domain.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 61d28337b..db5af1019 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1402,13 +1402,18 @@ qemuDomainSecretStorageSourcePrepare(virConnectPtr conn, const char *encalias) { qemuDomainStorageSourcePrivatePtr srcPriv; + bool hasAuth = qemuDomainSecretDiskCapable(src); + bool hasEnc = qemuDomainDiskHasEncryptionSecret(src); + + if (!hasAuth && !hasEnc) + return 0; if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) return -1; srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); - if (qemuDomainSecretDiskCapable(src)) { + if (hasAuth) { virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) @@ -1421,7 +1426,7 @@ qemuDomainSecretStorageSourcePrepare(virConnectPtr conn, return -1; } - if (qemuDomainDiskHasEncryptionSecret(src)) { + if (hasEnc) { if (!(srcPriv->encinfo = qemuDomainSecretInfoNew(conn, priv, encalias, VIR_SECRET_USAGE_TYPE_VOLUME, NULL, -- 2.14.3
Setup everything related to disks in one place rather than calling in from various places. The change to ordering of the setup steps is necessary since secrets need the master key to be present. --- src/qemu/qemu_domain.c | 5 +---- src/qemu/qemu_process.c | 14 +++++++++----- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index db5af1019..fd78e43e3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1647,10 +1647,7 @@ qemuDomainSecretPrepare(virConnectPtr conn, size_t i; int ret = -1; - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainSecretDiskPrepare(conn, priv, vm->def->disks[i]) < 0) - goto cleanup; - } + /* disk aliases are prepared when preparing disks */ for (i = 0; i < vm->def->nhostdevs; i++) { if (qemuDomainSecretHostdevPrepare(conn, priv, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 18dd3aa46..6d242b1b5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5343,6 +5343,7 @@ static int qemuProcessPrepareDomainStorage(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, + qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg, unsigned int flags) { @@ -5363,6 +5364,9 @@ qemuProcessPrepareDomainStorage(virConnectPtr conn, if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0) return -1; + + if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) + return -1; } return 0; @@ -5467,18 +5471,18 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuProcessSetupGraphics(driver, vm, flags) < 0) goto cleanup; - VIR_DEBUG("Setting up storage"); - if (qemuProcessPrepareDomainStorage(conn, driver, vm, cfg, flags) < 0) - goto cleanup; - VIR_DEBUG("Create domain masterKey"); if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; + VIR_DEBUG("Setting up storage"); + if (qemuProcessPrepareDomainStorage(conn, driver, vm, priv, cfg, flags) < 0) + goto cleanup; + VIR_DEBUG("Prepare chardev source backends for TLS"); qemuDomainPrepareChardevSource(vm->def, cfg); - VIR_DEBUG("Add secrets to disks, hostdevs, and chardevs"); + VIR_DEBUG("Add secrets to hostdevs and chardevs"); if (qemuDomainSecretPrepare(conn, driver, vm) < 0) goto cleanup; -- 2.14.3
On 11/09/2017 07:20 AM, Peter Krempa wrote:
Setup everything related to disks in one place rather than calling in from various places.
The change to ordering of the setup steps is necessary since secrets need the master key to be present. --- src/qemu/qemu_domain.c | 5 +---- src/qemu/qemu_process.c | 14 +++++++++----- 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index db5af1019..fd78e43e3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1647,10 +1647,7 @@ qemuDomainSecretPrepare(virConnectPtr conn, size_t i; int ret = -1;
- for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainSecretDiskPrepare(conn, priv, vm->def->disks[i]) < 0) - goto cleanup; - } + /* disk aliases are prepared when preparing disks */
You mean secrets.... John [...]
On 11/09/2017 07:20 AM, Peter Krempa wrote:
There were a few cases where we'd use private data for a storage source which was not allocated. Fix it by checking whether it is allocated.
Qemu command line parser allocated disk definition via VIR_ALLOC rather than the proper function. This resulted into the disk private data strucutre not being allocated. That one is required.
Peter Krempa (4): qemu: parse: Allocate disk definition with private data qemu: Tolerate storage source private data being NULL qemu: domain: Don't allocate storage source private data if not needed qemu: process: Setup disk secrets when preparing disks
src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- src/qemu/qemu_parse_command.c | 4 +--- src/qemu/qemu_process.c | 14 +++++++++----- 5 files changed, 44 insertions(+), 23 deletions(-)
with the edit in patch 4 handled... Reviewed-by: John Ferlan <jferlan@redhat.com> (series) John
participants (2)
-
John Ferlan -
Peter Krempa