[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
On Thu, Apr 02, 2026 at 11:12:25 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
+ 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
On Thu, Apr 02, 2026 at 11:12:26 -0400, Cole Robinson via Devel wrote:
+ 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,
Please document the arguments and the expectations. Especially the new ones you've introduced. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:27 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:28 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Thu, Apr 02, 2026 at 11:12:28 -0400, Cole Robinson via Devel wrote:
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);
I've noticed this a bit further down, but this is the first point where the qemuDomainGetImageIds name is starting to become misleading. The seclabel of the whole VM object is considered here and doesn't depend on any 'image' uid/gid. Renaming the function may be waranted before the whole refactor.
On 4/7/26 5:13 AM, Peter Krempa wrote:
On Thu, Apr 02, 2026 at 11:12:28 -0400, Cole Robinson via Devel wrote:
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);
Thanks for the reviews! I pushed 1 and 14.
I've noticed this a bit further down, but this is the first point where the qemuDomainGetImageIds name is starting to become misleading. The seclabel of the whole VM object is considered here and doesn't depend on any 'image' uid/gid.
Renaming the function may be waranted before the whole refactor.
Yeah that's fair. qemuDomainGetDACIds sound good? Or just qemuDomainGetIds? qemuDomainGetUidGid ? naming is not my forte Since you gave reviewed-by to 1-10 with comments, would you be fine with pushing those with function renamed and comments addressed, or rather I respin the whole series? I'm happy to do either Thanks, Cole
On Tue, Apr 07, 2026 at 19:35:41 -0400, Cole Robinson wrote:
On 4/7/26 5:13 AM, Peter Krempa wrote:
On Thu, Apr 02, 2026 at 11:12:28 -0400, Cole Robinson via Devel wrote:
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);
Thanks for the reviews! I pushed 1 and 14.
I've noticed this a bit further down, but this is the first point where the qemuDomainGetImageIds name is starting to become misleading. The seclabel of the whole VM object is considered here and doesn't depend on any 'image' uid/gid.
Renaming the function may be waranted before the whole refactor.
Yeah that's fair. qemuDomainGetDACIds sound good? Or just qemuDomainGetIds? qemuDomainGetUidGid ? naming is not my forte
I struggle with names too, that's why I didn't suggest any :D Anyways qemuDomainGetIds or qemuDomainGetUidGid would work for me, I guess I'd slightly prefer the latter.
Since you gave reviewed-by to 1-10 with comments, would you be fine with pushing those with function renamed and comments addressed, or rather I respin the whole series? I'm happy to do either
Once you rename qemuDomainGetImageIds and add error handling to it so that it can be used on code paths of inactive VMs you can use: Reviewed-by: Peter Krempa <pkrempa@redhat.com> on the rest of the series. IIRC Except for one comment all of my comments will be addressed by the rename and error handling.
On Wed, Apr 08, 2026 at 15:33:51 +0200, Peter Krempa via Devel wrote:
On Tue, Apr 07, 2026 at 19:35:41 -0400, Cole Robinson wrote:
On 4/7/26 5:13 AM, Peter Krempa wrote:
On Thu, Apr 02, 2026 at 11:12:28 -0400, Cole Robinson via Devel wrote:
[...]
Since you gave reviewed-by to 1-10 with comments, would you be fine with pushing those with function renamed and comments addressed, or rather I respin the whole series? I'm happy to do either
Once you rename qemuDomainGetImageIds and add error handling to it so that it can be used on code paths of inactive VMs you can use:
To be clear: - 11/16 should be reworked to pass in the definition and thus use proper seclabel; please post this one for review after you do that - 12/16 should not drop the 'cfg' argument from the leaf functions which operate on one image but rather pass it from the top. Also 'vm' argument ought to be first. With that and the rename+ error handling: Series: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:29 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:30 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:31 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:32 -0400, Cole Robinson via Devel wrote:
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;
'gid' is not used
if (!shortName) return -1;
+ qemuDomainGetImageIds(cfg, def, NULL, NULL, &uid, &gid);
and this tolerates any of the last two args being NULL.
+ return qemuTPMEmulatorPrepareHost(tpm, cfg->swtpmLogDir, cfg->swtpm_user, cfg->swtpm_group, cfg->swtpmStateDir, - cfg->user, + uid, shortName); }
With 'gid' removed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:33 -0400, Cole Robinson via Devel wrote:
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;
Declare these in the corresponding block rather than on top level.
/* 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);
In this case 'mirror' is the correct argument to be passed as 'src' here.
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;
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:34 -0400, Cole Robinson via Devel wrote:
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;
Declare these at the appropriate level.
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);
So using this name here, where we don't have a domain or image is a bit confusing. Renaming here will likely improve the situation to make this an acceptable change.
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;
same here
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;
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:35 -0400, Cole Robinson via Devel wrote:
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) {
While as you claim this doesn't change behaviour of the code, this function prepares the file to be used with a VM definition (which is available one level above in the call stack). IMO this patch should be replaced by a proper fix which passes in the definition and uses the seclabel from the definition. Otherwise it may seem that we deliberately don't want to use it, which is not the case (the correct seclabel will be applied anyways, albeit by the security driver when relabelling).
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
On Thu, Apr 02, 2026 at 11:12:36 -0400, Cole Robinson via Devel wrote:
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.
The idea to pass in 'cfg' from higher levels especially on code paths which prepare many virStorageSource elements at startup of a VM is to prevent the calls to 'virQEMUDriverGetConfig' which need the central lock - driver->lock - to be taken. I'd suggest you don't remove those at least on the code paths which are called back-to-back. Refactoring passing of 'driver' or 'priv' to pass the whole 'vm' is okay but note that in most cases our prevailing coding style is to pass 'vm' as first argument'.
Upcoming patches need vm->def as well, so let's clean this up first
Signed-off-by: Cole Robinson <crobinso@redhat.com> ---
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
On Thu, Apr 02, 2026 at 11:12:37 -0400, Cole Robinson via Devel wrote:
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);
So here the user/group is used to set the UID/GID of the nbdkit process. But this function also takes 'src' so it would seem to make sense on first glance to pass it to qemuDomainGetImageIds to apply the imagelabel as well. I don't think we want to do that, but the commit is not justifying in any way why qemuDomainGetImageIds is called as it is. Thus you'll have to add a comment expalining why the function is passed only the VM definition. I'd be even more critical to do so if you don't rename it as suggested earlier because qemuDomainGetImageIds really suggests that it ought to have 'src' passed in too.
return qemuNbdkitInitStorageSource(nbdkit, src, priv->libDir, - alias, cfg->user, cfg->group); + alias, uid, gid); }
-- 2.53.0
On Tue, Apr 07, 2026 at 14:06:46 +0200, Peter Krempa via Devel wrote:
On Thu, Apr 02, 2026 at 11:12:37 -0400, Cole Robinson via Devel wrote:
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);
So here the user/group is used to set the UID/GID of the nbdkit process. But this function also takes 'src' so it would seem to make sense on first glance to pass it to qemuDomainGetImageIds to apply the imagelabel as well. I don't think we want to do that, but the commit is not justifying in any way why qemuDomainGetImageIds is called as it is.
Thus you'll have to add a comment expalining why the function is passed only the VM definition.
I'd be even more critical to do so if you don't rename it as suggested earlier because qemuDomainGetImageIds really suggests that it ought to have 'src' passed in too.
With the rename done and the suggested comment added: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:38 -0400, Cole Robinson via Devel wrote:
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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
On Thu, Apr 02, 2026 at 11:12:39 -0400, Cole Robinson via Devel wrote:
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.
Prior to this series, 'qemuDomainGetImageIds()' was used only on code paths where the VM was already running (thus the user/group configured in the seclabel was validated). (only exception is the very much unused/useless API qemuDomainBlockPeek where it was used but thus had the bug) OTOH 'qemuDomainOpenFile' is used on code paths where the VM was not yet started and thus the value is not validated by the security driver at that point. User can thus have an invalid seclabel: <seclabel type='static' model='dac' relabel='yes'> <label>aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</label> </seclabel> configured in the definition; or for that matter e.g. user/group with unknown name and this would then silently ignore the error in the API call, but would spam the log with the error. qemuDomainGetImageIds or however you rename it thus should be fixed to report errors if you want to use it this way.
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
On Thu, Apr 02, 2026 at 11:12:40 -0400, Cole Robinson via Devel wrote:
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(-)
Once you address comments on previous patch this one will be okay: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Cole Robinson -
Peter Krempa