[PATCH 00/16] qemu: reduce direct usage of cfg->user
In the qemu driver, cfg->user and cfg->group are `user` and `group` from qemu.conf, the default qemu process user and owner for the security DAC driver. These are driver wide default configuration. They are used throughout driver code for storage access and launching sub processes. However code using cfg->user and cfg->group directly is usually subtly wrong, because these values can be overridden for individual VMs with XML like: <domain> <seclabel type='static' model='dac' relabel='yes'> <label>+1234:+4567</label> </seclabel> </domain> Most of the qemu storage code already gets this right (where these values can also be overridden at the <disk> level). This series fixes many other users by reusing the existing qemuDomainGetImageIds function used by the storage code. https://redhat.atlassian.net/browse/RHEL-119651 Cole Robinson (16): qemu: require non-NULL 'cfg' in qemuDomainGetImageIds() qemu: tweak qemuDomainGetImageIds() argument handling qemu: saveimage: use qemuDomainGetImageIds() qemu: domain taint: use qemuDomainGetImageIds() qemu: migrate to file: use qemuDomainGetImageIds() qemu: core dump: use qemuDomainGetImageIds() qemu: storage stat: use qemuDomainGetImageIds() qemu: tpm: use qemuDomainGetImageIds() qemu: block copy: use qemuDomainGetImageIds() qemu: cpu compare: use qemuDomainGetImageIds() qemu: nvram: use qemuDomainGetImageIds() qemu: simplify qemuProcessPrepareDomainStorage() args qemu: nbdkit: use qemuDomainGetImageIds() qemu: remove qemuDomainOpenFile() TODO comment qemu: don't check virParseOwnershipIds() error qemu: use qemuDomainGetImageIds() in qemuDomainOpenFile() src/qemu/qemu_backup.c | 12 +-- src/qemu/qemu_domain.c | 150 +++++++++++++++++++------------------- src/qemu/qemu_domain.h | 14 ++-- src/qemu/qemu_driver.c | 25 +++++-- src/qemu/qemu_hotplug.c | 6 +- src/qemu/qemu_migration.c | 5 +- src/qemu/qemu_process.c | 20 ++--- src/qemu/qemu_saveimage.c | 5 +- src/qemu/qemu_snapshot.c | 6 +- src/qemu/qemu_tpm.c | 6 +- 10 files changed, 130 insertions(+), 119 deletions(-) -- 2.53.0
Every caller passes this in, we can depend on it being non-NULL. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d4bd2a0b98..f494c6469b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6285,17 +6285,9 @@ qemuDomainGetImageIds(virQEMUDriverConfig *cfg, virSecurityDeviceLabelDef *disklabel; if (uid) - *uid = -1; + *uid = cfg->user; if (gid) - *gid = -1; - - if (cfg) { - if (uid) - *uid = cfg->user; - - if (gid) - *gid = cfg->group; - } + *gid = cfg->group; if ((vmlabel = virDomainDefGetSecurityLabelDef(def, "dac")) && vmlabel->label) -- 2.53.0
+ mark `def` as `const` + allow NULL `def` + allow NULL `src` Upcoming patches will need these Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 8 +++++--- src/qemu/qemu_domain.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f494c6469b..d3daa0fe17 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6276,7 +6276,7 @@ qemuDomainCleanupRun(virQEMUDriver *driver, void qemuDomainGetImageIds(virQEMUDriverConfig *cfg, - virDomainDef *def, + const virDomainDef *def, virStorageSource *src, virStorageSource *parentSrc, uid_t *uid, gid_t *gid) @@ -6289,7 +6289,8 @@ qemuDomainGetImageIds(virQEMUDriverConfig *cfg, if (gid) *gid = cfg->group; - if ((vmlabel = virDomainDefGetSecurityLabelDef(def, "dac")) && + if (def && + (vmlabel = virDomainDefGetSecurityLabelDef(def, "dac")) && vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid); @@ -6298,7 +6299,8 @@ qemuDomainGetImageIds(virQEMUDriverConfig *cfg, disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); - if ((disklabel = virStorageSourceGetSecurityLabelDef(src, "dac")) && + if (src && + (disklabel = virStorageSourceGetSecurityLabelDef(src, "dac")) && disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b321a64e96..c8f0d2326c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -728,7 +728,7 @@ bool qemuDomainDiskChangeSupported(virDomainDiskDef *disk, virDomainDiskDef *orig_disk); void qemuDomainGetImageIds(virQEMUDriverConfig *cfg, - virDomainDef *def, + const virDomainDef *def, virStorageSource *src, virStorageSource *parentSrc, uid_t *uid, -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_saveimage.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 64fbcd5f51..193d267d46 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -458,6 +458,8 @@ qemuSaveImageCreateFd(virDomainObj *vm, VIR_AUTOCLOSE fd = -1; int directFlag = 0; unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; + uid_t uid; + gid_t gid; if (!sparse && flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) { wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE; @@ -469,7 +471,8 @@ qemuSaveImageCreateFd(virDomainObj *vm, } } - fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, + qemuDomainGetImageIds(cfg, vm->def, NULL, NULL, &uid, &gid); + fd = virQEMUFileOpenAs(uid, gid, false, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, needUnlink); -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d3daa0fe17..efbcdc6d2d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5832,10 +5832,13 @@ void qemuDomainObjCheckTaint(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivate *priv = obj->privateData; bool custom_hypervisor_feat = false; + uid_t uid; + gid_t gid; + qemuDomainGetImageIds(cfg, obj->def, NULL, NULL, &uid, &gid); if (driver->privileged && - (cfg->user == 0 || - cfg->group == 0)) + (uid == 0 || + gid == 0)) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, logCtxt); if (priv->hookRun) -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_migration.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 33cc0f0ffe..9b172f47a0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -7251,6 +7251,8 @@ qemuMigrationSrcToSparseFile(virDomainObj *vm, int directFlag = 0; bool needUnlink = false; int ret; + uid_t uid; + gid_t gid; /* When using directio with mapped-ram, qemu needs two fds. One with * O_DIRECT set writing the memory, and another without it set for @@ -7262,7 +7264,8 @@ qemuMigrationSrcToSparseFile(virDomainObj *vm, _("bypass cache unsupported by this system")); return -1; } - directFd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, + qemuDomainGetImageIds(cfg, vm->def, NULL, NULL, &uid, &gid); + directFd = virQEMUFileOpenAs(uid, gid, false, path, O_WRONLY | directFlag, &needUnlink); if (directFd < 0) -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 861795724a..80e276b7df 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3103,6 +3103,8 @@ doCoreDump(virQEMUDriver *driver, const char *memory_dump_format = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) compressor = NULL; + uid_t uid; + gid_t gid; if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, &compressor, "dump") < 0) goto cleanup; @@ -3117,7 +3119,8 @@ doCoreDump(virQEMUDriver *driver, goto cleanup; } } - if ((fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, + qemuDomainGetImageIds(cfg, vm->def, NULL, NULL, &uid, &gid); + if ((fd = virQEMUFileOpenAs(uid, gid, false, path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, &needUnlink)) < 0) goto cleanup; -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index efbcdc6d2d..30c4c596df 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11599,6 +11599,9 @@ qemuDomainStorageOpenStat(virQEMUDriverConfig *cfg, struct stat *ret_sb, bool skipInaccessible) { + uid_t uid; + gid_t gid; + if (virStorageSourceIsLocalStorage(src)) { if (skipInaccessible && !virFileExists(src->path)) return 0; @@ -11616,7 +11619,8 @@ qemuDomainStorageOpenStat(virQEMUDriverConfig *cfg, if (skipInaccessible && virStorageSourceSupportsBackingChainTraversal(src) <= 0) return 0; - if (virStorageSourceInitAs(src, cfg->user, cfg->group) < 0) + qemuDomainGetImageIds(cfg, vm->def, src, NULL, &uid, &gid); + if (virStorageSourceInitAs(src, uid, gid) < 0) return -1; if (virStorageSourceStat(src, ret_sb) < 0) { -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_tpm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 660410bcba..dacceac678 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -1316,16 +1316,20 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *shortName = virDomainDefGetShortName(def); + uid_t uid; + gid_t gid; if (!shortName) return -1; + qemuDomainGetImageIds(cfg, def, NULL, NULL, &uid, &gid); + return qemuTPMEmulatorPrepareHost(tpm, cfg->swtpmLogDir, cfg->swtpm_user, cfg->swtpm_group, cfg->swtpmStateDir, - cfg->user, + uid, shortName); } -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80e276b7df..59b9601360 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14310,6 +14310,8 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, bool syncWrites = !!(flags & VIR_DOMAIN_BLOCK_COPY_SYNCHRONOUS_WRITES); bool targetIsZero = !!(flags & VIR_DOMAIN_BLOCK_COPY_TARGET_ZEROED); int rc = 0; + uid_t uid; + gid_t gid; /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -14414,10 +14416,11 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, * can also pass the RAW flag or use XML to tell us the format. * So if we get here, we assume it is safe for us to probe the * format from the file that we will be using. */ + qemuDomainGetImageIds(cfg, vm->def, NULL, NULL, &uid, &gid); if (!supports_detect || !virStorageSourceIsLocalStorage(mirror) || - (mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, - cfg->group)) < 0) { + (mirror->format = virStorageFileProbeFormat(mirror->path, uid, + gid)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reused mirror destination format must be specified")); goto endjob; -- 2.53.0
This does not change behavior, but it eliminates direct usage of `cfg->user` which makes for easier auditing Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59b9601360..0cfd42b0e1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11756,6 +11756,8 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, g_autoptr(virCPUDef) cpu = NULL; virArch arch; virDomainVirtType virttype; + uid_t uid; + gid_t gid; virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE | VIR_CONNECT_COMPARE_CPU_VALIDATE_XML, @@ -11810,8 +11812,10 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, return VIR_CPU_COMPARE_ERROR; } } + + qemuDomainGetImageIds(cfg, NULL, NULL, NULL, &uid, &gid); return qemuConnectCPUModelComparison(qemuCaps, cfg->libDir, - cfg->user, cfg->group, + uid, gid, hvCPU, cpu, failIncompatible); } @@ -11996,6 +12000,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, g_auto(GStrv) features = NULL; unsigned int physAddrSize = 0; size_t i; + uid_t uid; + gid_t gid; virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE | @@ -12057,8 +12063,9 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) { bool expand_features = (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES); + qemuDomainGetImageIds(cfg, NULL, NULL, NULL, &uid, &gid); if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, - cfg->user, cfg->group, + uid, gid, expand_features, cpus, ncpus, cpuModels))) goto cleanup; -- 2.53.0
This does not change behavior, but it eliminates direct usage of 'cfg->user' which makes for easier auditing Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_process.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7ebc038e54..a921bbcea8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4926,6 +4926,8 @@ qemuPrepareNVRAMFileCommon(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE srcFD = -1; struct qemuPrepareNVRAMHelperData data; + uid_t uid; + gid_t gid; if (!path) return 0; @@ -4951,9 +4953,10 @@ qemuPrepareNVRAMFileCommon(virQEMUDriver *driver, data.srcFD = srcFD; data.srcPath = template; + qemuDomainGetImageIds(cfg, NULL, NULL, NULL, &uid, &gid); if (virFileRewrite(path, S_IRUSR | S_IWUSR, - cfg->user, cfg->group, + uid, gid, qemuPrepareNVRAMHelper, &data) < 0) { return -1; -- 2.53.0
Variously 'cfg', 'driver', and 'priv' data are passed around the callstack, but we can get all of those as needed by passing around a single virDomainObj. Upcoming patches need vm->def as well, so let's clean this up first Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_backup.c | 12 ++--- src/qemu/qemu_domain.c | 97 +++++++++++++++++++++------------------- src/qemu/qemu_domain.h | 12 ++--- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 6 +-- src/qemu/qemu_process.c | 15 +++---- src/qemu/qemu_snapshot.c | 6 +-- 7 files changed, 69 insertions(+), 81 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 65a083ea74..a953d1879b 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -272,11 +272,8 @@ qemuBackupDiskPrepareDataOne(virDomainObj *vm, struct qemuBackupDiskData *dd, virJSONValue *actions, bool pull, - GHashTable *blockNamedNodeData, - virQEMUDriverConfig *cfg) + GHashTable *blockNamedNodeData) { - qemuDomainObjPrivate *priv = vm->privateData; - /* set data structure */ dd->backupdisk = backupdisk; dd->store = dd->backupdisk->store; @@ -315,7 +312,7 @@ qemuBackupDiskPrepareDataOne(virDomainObj *vm, dd->backingStore = dd->terminator = virStorageSourceNew(); } - if (qemuDomainPrepareStorageSourceBlockdev(NULL, dd->store, priv, cfg) < 0) + if (qemuDomainPrepareStorageSourceBlockdev(NULL, dd->store, vm) < 0) return -1; if (dd->backupdisk->incremental) { @@ -398,7 +395,6 @@ qemuBackupDiskPrepareData(virDomainObj *vm, virDomainBackupDef *def, GHashTable *blockNamedNodeData, virJSONValue *actions, - virQEMUDriverConfig *cfg, struct qemuBackupDiskData **rdd) { struct qemuBackupDiskData *disks = NULL; @@ -418,7 +414,7 @@ qemuBackupDiskPrepareData(virDomainObj *vm, ndisks++; if (qemuBackupDiskPrepareDataOne(vm, backupdisk, dd, actions, pull, - blockNamedNodeData, cfg) < 0) + blockNamedNodeData) < 0) goto error; if (pull) { @@ -883,7 +879,7 @@ qemuBackupBegin(virDomainObj *vm, goto endjob; if ((ndd = qemuBackupDiskPrepareData(vm, def, blockNamedNodeData, actions, - cfg, &dd)) <= 0) { + &dd)) <= 0) { if (ndd == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("no disks selected for backup")); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 30c4c596df..bed8c558d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6124,10 +6124,11 @@ qemuDomainSetFakeReboot(virDomainObj *vm, } static void -qemuDomainCheckRemoveOptionalDisk(virQEMUDriver *driver, - virDomainObj *vm, +qemuDomainCheckRemoveOptionalDisk(virDomainObj *vm, size_t diskIndex) { + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; char uuid[VIR_UUID_STRING_BUFLEN]; virObjectEvent *event = NULL; virDomainDiskDef *disk = vm->def->disks[diskIndex]; @@ -6176,8 +6177,7 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriver *driver, * clears any reported error if 0 is returned. */ int -qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, - virDomainObj *vm, +qemuDomainCheckDiskStartupPolicy(virDomainObj *vm, size_t diskIndex, bool cold_boot) { @@ -6209,7 +6209,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, break; } - qemuDomainCheckRemoveOptionalDisk(driver, vm, diskIndex); + qemuDomainCheckRemoveOptionalDisk(vm, diskIndex); virResetLastError(); return 0; } @@ -6416,10 +6416,10 @@ qemuDomainPrepareStorageSourceConfig(virStorageSource *src, static int qemuDomainPrepareStorageSource(virStorageSource *src, virDomainObj *vm, - virDomainDiskDef *disk, - virQEMUDriverConfig *cfg) + virDomainDiskDef *disk) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); /* convert detected ISO format to 'raw' as qemu would not understand it */ if (src->format == VIR_STORAGE_FILE_ISO) @@ -6432,7 +6432,7 @@ qemuDomainPrepareStorageSource(virStorageSource *src, qemuDomainPrepareDiskSourceData(disk, src); if (!qemuDiskBusIsSD(disk->bus) && - qemuDomainPrepareStorageSourceBlockdev(disk, src, priv, cfg) < 0) + qemuDomainPrepareStorageSourceBlockdev(disk, src, vm) < 0) return -1; return 0; @@ -6548,15 +6548,15 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, * backing chain we need to also consider the data store part of the current * image */ if (src->dataFileStore && !hadDataStore && - qemuDomainPrepareStorageSource(src->dataFileStore, vm, disk, cfg) < 0) + qemuDomainPrepareStorageSource(src->dataFileStore, vm, disk) < 0) return -1; for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuDomainPrepareStorageSource(n, vm, disk, cfg) < 0) + if (qemuDomainPrepareStorageSource(n, vm, disk) < 0) return -1; if (n->dataFileStore && - qemuDomainPrepareStorageSource(n->dataFileStore, vm, disk, cfg) < 0) + qemuDomainPrepareStorageSource(n->dataFileStore, vm, disk) < 0) return -1; } @@ -9101,11 +9101,12 @@ qemuDomainPrepareStorageSourceNFS(virStorageSource *src) */ static bool qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, - virQEMUDriverConfig *cfg, const char *alias, - qemuDomainObjPrivate *priv) + virDomainObj *vm) { g_autoptr(qemuNbdkitCaps) nbdkit = NULL; + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); if (!cfg->storageUseNbdkit) return false; @@ -9135,10 +9136,12 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, */ static int qemuDomainPrepareStorageSourceTLS(virStorageSource *src, - virQEMUDriverConfig *cfg, - const char *parentAlias, - qemuDomainObjPrivate *priv) + virDomainObj *vm, + const char *parentAlias) { + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) return 0; @@ -9781,9 +9784,11 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDef *disk) static int qemuDomainPrepareStorageSourcePR(virStorageSource *src, - qemuDomainObjPrivate *priv, + virDomainObj *vm, const char *parentalias) { + qemuDomainObjPrivate *priv = vm->privateData; + if (!src->pr) return 0; @@ -9804,16 +9809,17 @@ qemuDomainPrepareStorageSourcePR(virStorageSource *src, /** * qemuDomainPrepareDiskSourceLegacy: * @disk: disk to prepare - * @priv: VM private data - * @cfg: qemu driver config + * @vm: domain object * * Prepare any disk source relevant data for use with the -drive command line. */ static int qemuDomainPrepareDiskSourceLegacy(virDomainDiskDef *disk, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg) + virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + if (qemuDomainValidateStorageSource(disk->src, priv->qemuCaps) < 0) return -1; @@ -9828,11 +9834,10 @@ qemuDomainPrepareDiskSourceLegacy(virDomainDiskDef *disk, disk->info.alias) < 0) return -1; - if (qemuDomainPrepareStorageSourcePR(disk->src, priv, disk->info.alias) < 0) + if (qemuDomainPrepareStorageSourcePR(disk->src, vm, disk->info.alias) < 0) return -1; - if (qemuDomainPrepareStorageSourceTLS(disk->src, cfg, disk->info.alias, - priv) < 0) + if (qemuDomainPrepareStorageSourceTLS(disk->src, vm, disk->info.alias) < 0) return -1; return 0; @@ -9841,8 +9846,9 @@ qemuDomainPrepareDiskSourceLegacy(virDomainDiskDef *disk, static int qemuDomainPrepareStorageSourceFDs(virStorageSource *src, - qemuDomainObjPrivate *priv) + virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; qemuDomainStorageSourcePrivate *srcpriv = NULL; virStorageType actualType = virStorageSourceGetActualType(src); virDomainFDTuple *fdt = NULL; @@ -9898,16 +9904,17 @@ static int qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, virStorageSource *src, const char *nodenameprefix, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg) + virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); char *nodestorage = g_strdup_printf("%s-storage", nodenameprefix); const char *encryptionAlias = nodestorage; /* qemuBlockStorageSourceSetStorageNodename steals 'nodestorage' */ qemuBlockStorageSourceSetStorageNodename(src, nodestorage); - if (qemuDomainPrepareStorageSourceFDs(src, priv) < 0) + if (qemuDomainPrepareStorageSourceFDs(src, vm) < 0) return -1; if (qemuBlockStorageSourceNeedsFormatLayer(src, priv->qemuCaps)) { @@ -9933,18 +9940,17 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, qemuDomainPrepareStorageSourceConfig(src, cfg); qemuDomainPrepareDiskSourceData(disk, src); - if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, nodestorage, priv)) { + if (!qemuDomainPrepareStorageSourceNbdkit(src, nodestorage, vm)) { /* If we're using nbdkit to serve the storage source, we don't pass * authentication secrets to qemu, but will pass them to nbdkit instead */ if (qemuDomainSecretStorageSourcePrepareAuth(priv, src, nodestorage) < 0) return -1; } - if (qemuDomainPrepareStorageSourcePR(src, priv, nodestorage) < 0) + if (qemuDomainPrepareStorageSourcePR(src, vm, nodestorage) < 0) return -1; - if (qemuDomainPrepareStorageSourceTLS(src, cfg, nodestorage, - priv) < 0) + if (qemuDomainPrepareStorageSourceTLS(src, vm, nodestorage) < 0) return -1; if (qemuDomainPrepareStorageSourceNFS(src) < 0) @@ -9973,24 +9979,24 @@ qemuDomainPrepareThrottleFilterBlockdev(virDomainThrottleFilterDef *filter, int qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDef *disk, virStorageSource *src, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg) + virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; g_autofree char *nodenameprefix = NULL; src->id = qemuDomainStorageIDNew(priv); nodenameprefix = g_strdup_printf("libvirt-%u", src->id); - return qemuDomainPrepareStorageSourceBlockdevNodename(disk, src, nodenameprefix, priv, cfg); + return qemuDomainPrepareStorageSourceBlockdevNodename(disk, src, nodenameprefix, vm); } static int qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg) + virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); virStorageSource *n; size_t i; @@ -10000,11 +10006,11 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk, diskPriv->nodeCopyOnRead = g_strdup_printf("libvirt-CoR-%s", disk->dst); for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuDomainPrepareStorageSourceBlockdev(disk, n, priv, cfg) < 0) + if (qemuDomainPrepareStorageSourceBlockdev(disk, n, vm) < 0) return -1; if (n->dataFileStore && - qemuDomainPrepareStorageSourceBlockdev(disk, n->dataFileStore, priv, cfg) < 0) + qemuDomainPrepareStorageSourceBlockdev(disk, n->dataFileStore, vm) < 0) return -1; } @@ -10018,8 +10024,7 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk, int qemuDomainPrepareDiskSource(virDomainDiskDef *disk, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg) + virDomainObj *vm) { /* Nothing to prepare as it will use -chardev instead * of -blockdev/-drive option. */ @@ -10040,10 +10045,10 @@ qemuDomainPrepareDiskSource(virDomainDiskDef *disk, } if (!qemuDiskBusIsSD(disk->bus)) { - if (qemuDomainPrepareDiskSourceBlockdev(disk, priv, cfg) < 0) + if (qemuDomainPrepareDiskSourceBlockdev(disk, vm) < 0) return -1; } else { - if (qemuDomainPrepareDiskSourceLegacy(disk, priv, cfg) < 0) + if (qemuDomainPrepareDiskSourceLegacy(disk, vm) < 0) return -1; } @@ -10451,8 +10456,7 @@ qemuDomainDefHasManagedPR(virDomainObj *vm) * 'libvirt-pflash1-format' for pflash1. */ int -qemuDomainInitializePflashStorageSource(virDomainObj *vm, - virQEMUDriverConfig *cfg) +qemuDomainInitializePflashStorageSource(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; virDomainDef *def = vm->def; @@ -10476,8 +10480,7 @@ qemuDomainInitializePflashStorageSource(virDomainObj *vm, if (qemuDomainPrepareStorageSourceBlockdevNodename(NULL, def->os.loader->nvram, "libvirt-pflash1", - priv, - cfg) < 0) + vm) < 0) return -1; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c8f0d2326c..a3ce85bc8e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -709,8 +709,7 @@ qemuDomainRemoveInactiveLocked(virDomainObj *vm); void qemuDomainSetFakeReboot(virDomainObj *vm, bool value); -int qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, - virDomainObj *vm, +int qemuDomainCheckDiskStartupPolicy(virDomainObj *vm, size_t diskIndex, bool cold_boot); @@ -767,8 +766,7 @@ int qemuDomainStorageSourceAccessAllow(virQEMUDriver *driver, int qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDef *disk, virStorageSource *src, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg); + virDomainObj *vm); void qemuDomainCleanupAdd(virDomainObj *vm, qemuDomainCleanupCallback cb); @@ -1031,8 +1029,7 @@ qemuDomainValidateStorageSource(virStorageSource *src, int qemuDomainPrepareDiskSource(virDomainDiskDef *disk, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg); + virDomainObj *vm); bool qemuDomainDiskCachemodeFlags(virDomainDiskCache cachemode, @@ -1070,8 +1067,7 @@ qemuDomainMakeCPUMigratable(virArch arch, virCPUDef *origCPU); int -qemuDomainInitializePflashStorageSource(virDomainObj *vm, - virQEMUDriverConfig *cfg); +qemuDomainInitializePflashStorageSource(virDomainObj *vm); bool qemuDomainDiskHasLatencyHistogram(virDomainDiskDef *disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0cfd42b0e1..95c843451e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14481,7 +14481,7 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, /* we must initialize XML-provided chain prior to detecting to keep semantics * with VM startup */ for (n = mirror; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuDomainPrepareStorageSourceBlockdev(disk, n, priv, cfg) < 0) + if (qemuDomainPrepareStorageSourceBlockdev(disk, n, vm) < 0) goto endjob; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9439948089..458b1d1eca 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -608,8 +608,6 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, virStorageSource *newsrc, bool force) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - qemuDomainObjPrivate *priv = vm->privateData; virStorageSource *oldsrc = disk->src; qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); int rc; @@ -628,7 +626,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto rollback; - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + if (qemuDomainPrepareDiskSource(disk, vm) < 0) goto rollback; if (qemuDomainStorageSourceChainAccessAllow(driver, vm, newsrc) < 0) @@ -1072,7 +1070,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, goto cleanup; if (!virStorageSourceIsEmpty(disk->src)) { - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + if (qemuDomainPrepareDiskSource(disk, vm) < 0) goto cleanup; if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a921bbcea8..612698463d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6840,10 +6840,7 @@ qemuProcessPrepareDeviceBootorder(virDomainDef *def) static int -qemuProcessPrepareDomainStorage(virQEMUDriver *driver, - virDomainObj *vm, - qemuDomainObjPrivate *priv, - virQEMUDriverConfig *cfg, +qemuProcessPrepareDomainStorage(virDomainObj *vm, unsigned int flags) { size_t i; @@ -6854,14 +6851,14 @@ qemuProcessPrepareDomainStorage(virQEMUDriver *driver, virDomainDiskDef *disk = vm->def->disks[idx]; if (virDomainDiskTranslateSourcePool(disk) < 0) { - if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0) + if (qemuDomainCheckDiskStartupPolicy(vm, idx, cold_boot) < 0) return -1; /* disk source was dropped */ continue; } - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + if (qemuDomainPrepareDiskSource(disk, vm) < 0) return -1; } @@ -7122,7 +7119,7 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, return -1; VIR_DEBUG("Setting up storage"); - if (qemuProcessPrepareDomainStorage(driver, vm, priv, cfg, flags) < 0) + if (qemuProcessPrepareDomainStorage(vm, flags) < 0) return -1; VIR_DEBUG("Setting up host devices"); @@ -7144,7 +7141,7 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, VIR_DEBUG("Prepare bios/uefi paths"); if (qemuFirmwareFillDomain(driver, vm->def, false) < 0) return -1; - if (qemuDomainInitializePflashStorageSource(vm, cfg) < 0) + if (qemuDomainInitializePflashStorageSource(vm) < 0) return -1; VIR_DEBUG("Preparing external devices"); @@ -7436,7 +7433,7 @@ qemuProcessPrepareHostStorage(virQEMUDriver *driver, else if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) >= 0) continue; - if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) + if (qemuDomainCheckDiskStartupPolicy(vm, idx, cold_boot) >= 0) continue; return -1; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 82ae38ca29..e71282d559 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1257,12 +1257,10 @@ qemuSnapshotDiskBitmapsPropagate(qemuSnapshotDiskData *dd, static int qemuSnapshotDiskPrepareOneBlockdev(virDomainObj *vm, qemuSnapshotDiskData *dd, - virQEMUDriverConfig *cfg, bool reuse, GHashTable *blockNamedNodeData, virDomainAsyncJob asyncJob) { - qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(virStorageSource) terminator = NULL; int rc; @@ -1271,7 +1269,7 @@ qemuSnapshotDiskPrepareOneBlockdev(virDomainObj *vm, terminator = virStorageSourceNew(); if (qemuDomainPrepareStorageSourceBlockdev(dd->disk, dd->src, - priv, cfg) < 0) + vm) < 0) return -1; if (!(dd->crdata = qemuBuildStorageSourceChainAttachPrepareBlockdevTop(dd->src, @@ -1371,7 +1369,7 @@ qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt, dd->prepared = true; - if (qemuSnapshotDiskPrepareOneBlockdev(vm, dd, snapctxt->cfg, reuse, + if (qemuSnapshotDiskPrepareOneBlockdev(vm, dd, reuse, blockNamedNodeData, snapctxt->asyncJob) < 0) return -1; -- 2.53.0
Fixed to abide domain seclabel model='dac' override Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bed8c558d0..c89030b60c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9107,6 +9107,8 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, g_autoptr(qemuNbdkitCaps) nbdkit = NULL; qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + uid_t uid; + gid_t gid; if (!cfg->storageUseNbdkit) return false; @@ -9118,8 +9120,9 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, if (!nbdkit) return false; + qemuDomainGetImageIds(cfg, vm->def, NULL, NULL, &uid, &gid); return qemuNbdkitInitStorageSource(nbdkit, src, priv->libDir, - alias, cfg->user, cfg->group); + alias, uid, gid); } -- 2.53.0
qemuDomainOpenFile() only acts on the 'dac' driver, where 'label' and 'imagelabel' are always identical (see virSecurityDACGenLabel()). So there's nothing TODO here Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c89030b60c..6695f32c01 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10701,7 +10701,6 @@ qemuDomainOpenFile(virQEMUDriverConfig *cfg, bool dynamicOwnership = cfg->dynamicOwnership; virSecurityLabelDef *seclabel; - /* TODO: Take imagelabel into account? */ if (def && (seclabel = virDomainDefGetSecurityLabelDef(def, "dac")) != NULL && seclabel->label != NULL && -- 2.53.0
This matches the behavior of qemuDomainGetImageIds() which we are about to convert to. I think it's safe to depend on the security driver to have already validated this for us. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6695f32c01..a0f41c436e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10703,9 +10703,8 @@ qemuDomainOpenFile(virQEMUDriverConfig *cfg, if (def && (seclabel = virDomainDefGetSecurityLabelDef(def, "dac")) != NULL && - seclabel->label != NULL && - (virParseOwnershipIds(seclabel->label, &user, &group) < 0)) - return -EINVAL; + seclabel->label != NULL) + virParseOwnershipIds(seclabel->label, &user, &group); return virQEMUFileOpenAs(user, group, dynamicOwnership, path, oflags, needUnlink); -- 2.53.0
This does not change behavior, but it eliminates direct usage of 'cfg->user' which makes for easier auditing Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_domain.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a0f41c436e..83d4bb124e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10696,17 +10696,12 @@ qemuDomainOpenFile(virQEMUDriverConfig *cfg, int oflags, bool *needUnlink) { - uid_t user = cfg->user; - gid_t group = cfg->group; bool dynamicOwnership = cfg->dynamicOwnership; - virSecurityLabelDef *seclabel; + uid_t uid; + gid_t gid; - if (def && - (seclabel = virDomainDefGetSecurityLabelDef(def, "dac")) != NULL && - seclabel->label != NULL) - virParseOwnershipIds(seclabel->label, &user, &group); - - return virQEMUFileOpenAs(user, group, dynamicOwnership, + qemuDomainGetImageIds(cfg, def, NULL, NULL, &uid, &gid); + return virQEMUFileOpenAs(uid, gid, dynamicOwnership, path, oflags, needUnlink); } -- 2.53.0
participants (1)
-
Cole Robinson