[PATCH 0/6] qemu: sync initial dir perms with RPM spec
The qemu driver creates a bunch of directories at startup, example /var/lib/libvirt/qemu/*. The initial mode mask on these directories is almost always specified as 0777, which leaves it up to umask to lock things down. As a result, if running from git, directories are usually created 0755 which seems overly permissive for a system daemon. This doesn't have much effect from RPM users at least, since the spec file pre-creates most of these directories with more limited permissions. This series syncs the code to match what we already specify in the spec file. Code is simplified first, and some missing dirs are added to the spec at the end. Cole Robinson (6): qemu: driver: split out qemuStateInitializeDirs qemu: driver: streamline dir creation qemu: driver: don't chown() dirname(cfg->channelTargetDir) qemu: driver: sync dir creation permissions with RPM spec qemu: driver: adjust mode mask for rdpStateDir qemu: driver: adjust mode mask for channelTargetDir libvirt.spec.in | 2 + src/qemu/qemu_driver.c | 249 ++++++++++++----------------------------- 2 files changed, 74 insertions(+), 177 deletions(-) -- 2.53.0
Move all all driver directory creation and permission handling from qemuStateInitialize to its own function. This is just code movement Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 299 ++++++++++++++++++++++------------------- 1 file changed, 158 insertions(+), 141 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 861795724a..b9f5e976b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -518,68 +518,11 @@ qemuDomainFindMaxID(virDomainObj *vm, } -/** - * qemuStateInitialize: - * - * Initialization function for the QEMU daemon - */ -static virDrvStateInitResult -qemuStateInitialize(bool privileged, - const char *root, - bool monolithic G_GNUC_UNUSED, - virStateInhibitCallback callback, - void *opaque) +static int +qemuStateInitializeDirs(bool privileged, + virQEMUDriverConfig *cfg) { - g_autofree char *driverConf = NULL; - virQEMUDriverConfig *cfg; - uid_t run_uid = -1; - gid_t run_gid = -1; - size_t i; - const char *defsecmodel = NULL; - g_autoptr(virIdentity) identity = virIdentityGetCurrent(); - virDomainDriverAutoStartConfig autostartCfg; - - qemu_driver = g_new0(virQEMUDriver, 1); - - qemu_driver->lockFD = -1; - - if (virMutexInit(&qemu_driver->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize mutex")); - VIR_FREE(qemu_driver); - return VIR_DRV_STATE_INIT_ERROR; - } - - qemu_driver->privileged = privileged; - qemu_driver->hostarch = virArchFromHost(); - if (root != NULL) - qemu_driver->embeddedRoot = g_strdup(root); - - if (!(qemu_driver->domains = virDomainObjListNew())) - goto error; - - /* Init domain events */ - qemu_driver->domainEventState = virObjectEventStateNew(); - if (!qemu_driver->domainEventState) - goto error; - - /* read the host sysinfo */ - if (privileged) - qemu_driver->hostsysinfo = virSysinfoRead(); - - if (!(qemu_driver->config = cfg = virQEMUDriverConfigNew(privileged, root))) - goto error; - - driverConf = g_strdup_printf("%s/qemu.conf", cfg->configBaseDir); - - if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0) - goto error; - - if (virQEMUDriverConfigValidate(cfg) < 0) - goto error; - - if (virQEMUDriverConfigSetDefaults(cfg) < 0) - goto error; + int ret = -1; if (g_mkdir_with_parents(cfg->stateDir, 0777) < 0) { virReportSystemError(errno, _("Failed to create state dir %1$s"), @@ -659,81 +602,6 @@ qemuStateInitialize(bool privileged, goto error; } - qemu_driver->inhibitor = virInhibitorNew( - VIR_INHIBITOR_WHAT_SHUTDOWN, - _("Libvirt QEMU"), - _("QEMU/KVM virtual machines are running"), - VIR_INHIBITOR_MODE_DELAY, - callback, - opaque); - - if ((qemu_driver->lockFD = - virPidFileAcquire(cfg->stateDir, "driver", getpid())) < 0) - goto error; - - if (!(qemu_driver->lockManager = - virLockManagerPluginNew(cfg->lockManagerName ? - cfg->lockManagerName : "nop", - "qemu", - cfg->configBaseDir, - 0))) - goto error; - - if (cfg->macFilter) { - if (!(qemu_driver->ebtables = ebtablesContextNew("qemu"))) { - virReportSystemError(errno, - _("failed to enable mac filter in '%1$s'"), - __FILE__); - goto error; - } - - if (ebtablesAddForwardPolicyReject(qemu_driver->ebtables) < 0) - goto error; - } - - /* Allocate bitmap for remote display port reservations. We cannot - * do this before the config is loaded properly, since the port - * numbers are configurable now */ - if ((qemu_driver->remotePorts = - virPortAllocatorRangeNew(_("display"), - cfg->remotePortMin, - cfg->remotePortMax)) == NULL) - goto error; - - if ((qemu_driver->webSocketPorts = - virPortAllocatorRangeNew(_("webSocket"), - cfg->webSocketPortMin, - cfg->webSocketPortMax)) == NULL) - goto error; - - if ((qemu_driver->rdpPorts = - virPortAllocatorRangeNew(_("rdp"), - cfg->rdpPortMin, - cfg->rdpPortMax)) == NULL) - goto error; - - - if ((qemu_driver->migrationPorts = - virPortAllocatorRangeNew(_("migration"), - cfg->migrationPortMin, - cfg->migrationPortMax)) == NULL) - goto error; - - if ((qemu_driver->backupPorts = - virPortAllocatorRangeNew(_("backup"), - cfg->backupPortMin, - cfg->backupPortMax)) == NULL) - goto error; - - if (qemuSecurityInit(qemu_driver) < 0) - goto error; - - if (!(qemu_driver->hostdevMgr = virHostdevManagerGetDefault())) - goto error; - - if (qemuMigrationDstErrorInit(qemu_driver) < 0) - goto error; - if (privileged) { g_autofree char *channeldir = NULL; @@ -830,7 +698,161 @@ qemuStateInitialize(bool privileged, (int)cfg->group); goto error; } + } + if (privileged && + virFileUpdatePerm(cfg->memoryBackingDir, + 0, S_IXGRP | S_IXOTH) < 0) + goto error; + + ret = 0; +error: + return ret; +} + + +/** + * qemuStateInitialize: + * + * Initialization function for the QEMU daemon + */ +static virDrvStateInitResult +qemuStateInitialize(bool privileged, + const char *root, + bool monolithic G_GNUC_UNUSED, + virStateInhibitCallback callback, + void *opaque) +{ + g_autofree char *driverConf = NULL; + virQEMUDriverConfig *cfg; + uid_t run_uid = -1; + gid_t run_gid = -1; + size_t i; + const char *defsecmodel = NULL; + g_autoptr(virIdentity) identity = virIdentityGetCurrent(); + virDomainDriverAutoStartConfig autostartCfg; + + qemu_driver = g_new0(virQEMUDriver, 1); + + qemu_driver->lockFD = -1; + + if (virMutexInit(&qemu_driver->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize mutex")); + VIR_FREE(qemu_driver); + return VIR_DRV_STATE_INIT_ERROR; + } + + qemu_driver->privileged = privileged; + qemu_driver->hostarch = virArchFromHost(); + if (root != NULL) + qemu_driver->embeddedRoot = g_strdup(root); + + if (!(qemu_driver->domains = virDomainObjListNew())) + goto error; + + /* Init domain events */ + qemu_driver->domainEventState = virObjectEventStateNew(); + if (!qemu_driver->domainEventState) + goto error; + + /* read the host sysinfo */ + if (privileged) + qemu_driver->hostsysinfo = virSysinfoRead(); + + if (!(qemu_driver->config = cfg = virQEMUDriverConfigNew(privileged, root))) + goto error; + + driverConf = g_strdup_printf("%s/qemu.conf", cfg->configBaseDir); + + if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0) + goto error; + + if (virQEMUDriverConfigValidate(cfg) < 0) + goto error; + + if (virQEMUDriverConfigSetDefaults(cfg) < 0) + goto error; + + if (qemuStateInitializeDirs(privileged, cfg) < 0) + goto error; + + qemu_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_SHUTDOWN, + _("Libvirt QEMU"), + _("QEMU/KVM virtual machines are running"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + + if ((qemu_driver->lockFD = + virPidFileAcquire(cfg->stateDir, "driver", getpid())) < 0) + goto error; + + if (!(qemu_driver->lockManager = + virLockManagerPluginNew(cfg->lockManagerName ? + cfg->lockManagerName : "nop", + "qemu", + cfg->configBaseDir, + 0))) + goto error; + + if (cfg->macFilter) { + if (!(qemu_driver->ebtables = ebtablesContextNew("qemu"))) { + virReportSystemError(errno, + _("failed to enable mac filter in '%1$s'"), + __FILE__); + goto error; + } + + if (ebtablesAddForwardPolicyReject(qemu_driver->ebtables) < 0) + goto error; + } + + /* Allocate bitmap for remote display port reservations. We cannot + * do this before the config is loaded properly, since the port + * numbers are configurable now */ + if ((qemu_driver->remotePorts = + virPortAllocatorRangeNew(_("display"), + cfg->remotePortMin, + cfg->remotePortMax)) == NULL) + goto error; + + if ((qemu_driver->webSocketPorts = + virPortAllocatorRangeNew(_("webSocket"), + cfg->webSocketPortMin, + cfg->webSocketPortMax)) == NULL) + goto error; + + if ((qemu_driver->rdpPorts = + virPortAllocatorRangeNew(_("rdp"), + cfg->rdpPortMin, + cfg->rdpPortMax)) == NULL) + goto error; + + + if ((qemu_driver->migrationPorts = + virPortAllocatorRangeNew(_("migration"), + cfg->migrationPortMin, + cfg->migrationPortMax)) == NULL) + goto error; + + if ((qemu_driver->backupPorts = + virPortAllocatorRangeNew(_("backup"), + cfg->backupPortMin, + cfg->backupPortMax)) == NULL) + goto error; + + if (qemuSecurityInit(qemu_driver) < 0) + goto error; + + if (!(qemu_driver->hostdevMgr = virHostdevManagerGetDefault())) + goto error; + + if (qemuMigrationDstErrorInit(qemu_driver) < 0) + goto error; + + if (privileged) { run_uid = cfg->user; run_gid = cfg->group; } @@ -859,11 +881,6 @@ qemuStateInitialize(bool privileged, goto error; } - if (privileged && - virFileUpdatePerm(cfg->memoryBackingDir, - 0, S_IXGRP | S_IXOTH) < 0) - goto error; - /* Get all the running persistent or transient configs first */ if (virDomainObjListLoadAllConfigs(qemu_driver->domains, cfg->stateDir, -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:53 -0400, Cole Robinson via Devel wrote:
Move all all driver directory creation and permission handling from qemuStateInitialize to its own function. This is just code movement
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 299 ++++++++++++++++++++++------------------- 1 file changed, 158 insertions(+), 141 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 861795724a..b9f5e976b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
[...]
@@ -830,7 +698,161 @@ qemuStateInitialize(bool privileged, (int)cfg->group); goto error; } + }
+ if (privileged && + virFileUpdatePerm(cfg->memoryBackingDir, + 0, S_IXGRP | S_IXOTH) < 0) + goto error; + + ret = 0; +error:
235/314 libvirt:syntax-check / require_space_before_label FAIL 0.09s exit status 2
ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MESON_TEST_ITERATION=1 MALLOC_PERTURB_=82 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 /bin/make -C /home/pipo/build/libvirt/gcc/build-aux sc_require_space_before_label ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― stdout: make: Entering directory '/home/pipo/build/libvirt/gcc/build-aux' /home/pipo/libvirt/src/qemu/qemu_driver.c:709:error: make: Leaving directory '/home/pipo/build/libvirt/gcc/build-aux' stderr: Top-level labels should be indented by one space make: *** [/home/pipo/libvirt/build-aux/syntax-check.mk:720: sc_require_space_before_label] Error 1
+ return ret;
With syntax-check fixed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Make it more data driven. This reduces code and makes it easier to see dir mode requests at a glance. Semantics of virDirCreate are subtly different, so keep dbusStateDir separate for now Besides some operation reordering this should behave the same as before Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 224 ++++++++++------------------------------- 1 file changed, 51 insertions(+), 173 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b9f5e976b2..0cf88b8be9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -522,192 +522,70 @@ static int qemuStateInitializeDirs(bool privileged, virQEMUDriverConfig *cfg) { - int ret = -1; + size_t i; + g_autofree char *channeldir = g_path_get_dirname(cfg->channelTargetDir); - if (g_mkdir_with_parents(cfg->stateDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create state dir %1$s"), - cfg->stateDir); - goto error; - } - if (g_mkdir_with_parents(cfg->libDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create lib dir %1$s"), - cfg->libDir); - goto error; - } - if (g_mkdir_with_parents(cfg->cacheDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create cache dir %1$s"), - cfg->cacheDir); - goto error; - } - if (g_mkdir_with_parents(cfg->saveDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create save dir %1$s"), - cfg->saveDir); - goto error; - } - if (g_mkdir_with_parents(cfg->snapshotDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create snapshot dir %1$s"), - cfg->snapshotDir); - goto error; - } - if (g_mkdir_with_parents(cfg->checkpointDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create checkpoint dir %1$s"), - cfg->checkpointDir); - goto error; - } - if (g_mkdir_with_parents(cfg->autoDumpPath, 0777) < 0) { - virReportSystemError(errno, _("Failed to create dump dir %1$s"), - cfg->autoDumpPath); - goto error; - } - if (g_mkdir_with_parents(cfg->channelTargetDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create channel target dir %1$s"), - cfg->channelTargetDir); - goto error; - } - if (g_mkdir_with_parents(cfg->nvramDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create nvram dir %1$s"), - cfg->nvramDir); - goto error; - } - if (g_mkdir_with_parents(cfg->varstoreDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create varstore dir %1$s"), - cfg->varstoreDir); - goto error; - } - if (g_mkdir_with_parents(cfg->memoryBackingDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create memory backing dir %1$s"), - cfg->memoryBackingDir); - goto error; - } - if (g_mkdir_with_parents(cfg->slirpStateDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create slirp state dir %1$s"), - cfg->slirpStateDir); - goto error; - } - if (g_mkdir_with_parents(cfg->passtStateDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create passt state dir %1$s"), - cfg->passtStateDir); - goto error; + struct dirperms { + const char *dir; + int mode; + uid_t user; + gid_t group; + }; + struct dirperms dirs[] = { + /* example: /var/cache/libvirt/qemu */ + { cfg->cacheDir, 0777, -1, -1 }, + + /* example: /run/libvirt/qemu */ + { cfg->stateDir, 0777, -1, -1 }, + { cfg->slirpStateDir, 0777, cfg->user, cfg->group }, + { cfg->passtStateDir, 0777, cfg->user, cfg->group }, + { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, + { channeldir, 0777, cfg->user, cfg->group }, + { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, + + /* example: /var/lib/libvirt/qemu */ + { cfg->libDir, 0777, cfg->user, cfg->group }, + { cfg->saveDir, 0777, cfg->user, cfg->group }, + { cfg->snapshotDir, 0777, cfg->user, cfg->group }, + { cfg->checkpointDir, 0777, cfg->user, cfg->group }, + { cfg->autoDumpPath, 0777, cfg->user, cfg->group }, + { cfg->nvramDir, 0777, cfg->user, cfg->group }, + { cfg->varstoreDir, 0777, cfg->user, cfg->group }, + { cfg->memoryBackingDir, 0777, cfg->user, cfg->group }, + }; + + for (i = 0; i < G_N_ELEMENTS(dirs); i++) { + if (g_mkdir_with_parents(dirs[i].dir, dirs[i].mode) < 0) { + virReportSystemError(errno, _("Failed to create directory %1$s"), + dirs[i].dir); + return -1; + } + + if (privileged && + dirs[i].user != -1) { + if (chown(dirs[i].dir, dirs[i].user, dirs[i].group) < 0) { + virReportSystemError(errno, + _("unable to set ownership of '%1$s' to %2$d:%3$d"), + dirs[i].dir, (int)dirs[i].user, + (int)dirs[i].group); + return -1; + } + } } if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, VIR_DIR_CREATE_ALLOW_EXIST) < 0) { virReportSystemError(errno, _("Failed to create dbus state dir %1$s"), cfg->dbusStateDir); - goto error; - } - if (g_mkdir_with_parents(cfg->rdpStateDir, 0777) < 0) { - virReportSystemError(errno, _("Failed to create rdp state dir %1$s"), - cfg->rdpStateDir); - goto error; - } - - if (privileged) { - g_autofree char *channeldir = NULL; - - if (chown(cfg->libDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to user %2$d:%3$d"), - cfg->libDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->saveDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->saveDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->snapshotDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->snapshotDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->checkpointDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->checkpointDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->autoDumpPath, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->autoDumpPath, (int)cfg->user, - (int)cfg->group); - goto error; - } - channeldir = g_path_get_dirname(cfg->channelTargetDir); - - if (chown(channeldir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - channeldir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->channelTargetDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->channelTargetDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->nvramDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->nvramDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->varstoreDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->varstoreDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->memoryBackingDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->memoryBackingDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->slirpStateDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->slirpStateDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->passtStateDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->passtStateDir, (int)cfg->user, - (int)cfg->group); - goto error; - } - if (chown(cfg->rdpStateDir, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership of '%1$s' to %2$d:%3$d"), - cfg->rdpStateDir, (int)cfg->user, - (int)cfg->group); - goto error; - } + return -1; } if (privileged && virFileUpdatePerm(cfg->memoryBackingDir, 0, S_IXGRP | S_IXOTH) < 0) - goto error; + return -1; - ret = 0; -error: - return ret; + return 0; } -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:54 -0400, Cole Robinson via Devel wrote:
Make it more data driven. This reduces code and makes it easier to see dir mode requests at a glance.
Semantics of virDirCreate are subtly different, so keep dbusStateDir separate for now
[1]
Besides some operation reordering this should behave the same as before
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 224 ++++++++++------------------------------- 1 file changed, 51 insertions(+), 173 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b9f5e976b2..0cf88b8be9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -522,192 +522,70 @@ static int qemuStateInitializeDirs(bool privileged, virQEMUDriverConfig *cfg) { - int ret = -1; + size_t i; + g_autofree char *channeldir = g_path_get_dirname(cfg->channelTargetDir);
Since you're keeping dbusStateDir separate, keep this one too. Especially since it's being removed later on. [...]
+ struct dirperms { + const char *dir; + int mode; + uid_t user; + gid_t group; + }; + struct dirperms dirs[] = { + /* example: /var/cache/libvirt/qemu */ + { cfg->cacheDir, 0777, -1, -1 }, @ + + /* example: /run/libvirt/qemu */ + { cfg->stateDir, 0777, -1, -1 }, @ + { cfg->slirpStateDir, 0777, cfg->user, cfg->group }, @# + { cfg->passtStateDir, 0777, cfg->user, cfg->group }, @# + { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, @# + { channeldir, 0777, cfg->user, cfg->group }, + { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, @ + + /* example: /var/lib/libvirt/qemu */ + { cfg->libDir, 0777, cfg->user, cfg->group }, @ + { cfg->saveDir, 0777, cfg->user, cfg->group }, @ + { cfg->snapshotDir, 0777, cfg->user, cfg->group }, @ + { cfg->checkpointDir, 0777, cfg->user, cfg->group }, @ + { cfg->autoDumpPath, 0777, cfg->user, cfg->group }, @ + { cfg->nvramDir, 0777, cfg->user, cfg->group }, @# + { cfg->varstoreDir, 0777, cfg->user, cfg->group }, @# + { cfg->memoryBackingDir, 0777, cfg->user, cfg->group }, @# + }; + + for (i = 0; i < G_N_ELEMENTS(dirs); i++) { + if (g_mkdir_with_parents(dirs[i].dir, dirs[i].mode) < 0) { + virReportSystemError(errno, _("Failed to create directory %1$s"), + dirs[i].dir); + return -1; + } + + if (privileged && + dirs[i].user != -1) {
Fails syntax-check: make -C /home/pipo/build/libvirt/gcc/build-aux sc_prohibit_risky_id_promotion ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― stdout: make: Entering directory '/home/pipo/build/libvirt/gcc/build-aux' /home/pipo/libvirt/src/qemu/qemu_driver.c:565: dirs[i].user != -1) { make: Leaving directory '/home/pipo/build/libvirt/gcc/build-aux' stderr: cast -1 to ([ug]id_t) before comparing against id make: *** [/home/pipo/libvirt/build-aux/syntax-check.mk:183: sc_prohibit_risky_id_promotion] Error 1 You need to typecast -1 to uid_t. With syntax-check fixed and the channelDir thing separate: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Once upon a time dirname(cfg->channelTargetDir) was a unique dir, but nowadays it is the same as cfg->stateDir, so this is redundant. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0cf88b8be9..5dff049d85 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -523,7 +523,6 @@ qemuStateInitializeDirs(bool privileged, virQEMUDriverConfig *cfg) { size_t i; - g_autofree char *channeldir = g_path_get_dirname(cfg->channelTargetDir); struct dirperms { const char *dir; @@ -540,7 +539,6 @@ qemuStateInitializeDirs(bool privileged, { cfg->slirpStateDir, 0777, cfg->user, cfg->group }, { cfg->passtStateDir, 0777, cfg->user, cfg->group }, { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, - { channeldir, 0777, cfg->user, cfg->group }, { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, /* example: /var/lib/libvirt/qemu */ -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:55 -0400, Cole Robinson via Devel wrote:
Once upon a time dirname(cfg->channelTargetDir) was a unique dir, but nowadays it is the same as cfg->stateDir, so this is redundant.
The commit message is a bit light on the justification for this change. historically this was in /var/lib/libvirt/qemu/channel/target/ thus it was supposed to also chown the 'channel' directory to the appropriate mode. Now commit d3759d3674ab9453e5fb5a27ab6c28b8ff8d5569 moved it to: /var/lib/libvirt/qemu/channel/ and subsequently commit 8abc979bb09ca4b93123e8f75f3d28cc421a0bb6 moved it to: /var/run/libvirt/qemu/channel/ thus currently it chowns /var/run/libvirt/qemu. I'd say this change would make sense to happen before the commit which refactors it to ...
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0cf88b8be9..5dff049d85 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -523,7 +523,6 @@ qemuStateInitializeDirs(bool privileged, virQEMUDriverConfig *cfg) { size_t i; - g_autofree char *channeldir = g_path_get_dirname(cfg->channelTargetDir);
struct dirperms { const char *dir; @@ -540,7 +539,6 @@ qemuStateInitializeDirs(bool privileged, { cfg->slirpStateDir, 0777, cfg->user, cfg->group }, { cfg->passtStateDir, 0777, cfg->user, cfg->group }, { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, - { channeldir, 0777, cfg->user, cfg->group },
.. the array syntax.
{ cfg->channelTargetDir, 0777, cfg->user, cfg->group },
I'd even say that this'd warant a: Fixes: d3759d3674ab9453e5fb5a27ab6c28b8ff8d5569 although it doesn't have a functional difference so it's not needed. If you move this before the refactor: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The 0777 permission mask we use when creating most 'cfg' dirs does not match what we put on disk via the RPM spec file. Generally those dirs are more locked down. Match driver startup permissions and owners with what we encode in the RPM spec. Presumably this is safe because this has been tested with real world usage. Some dirs are created here but not in the RPM spec. Leave their permission mask as is, we will deal with them in future patches. The 2 runtime changes for an RPM installed libvirt is that stateDir is now chown(qemu, qemu) and runDir is now chown(0, 0) where previously there was no chown() calls for these dirs. I don't think that should cause problems Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5dff049d85..f351aab009 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -524,6 +524,9 @@ qemuStateInitializeDirs(bool privileged, { size_t i; + uid_t daemon_uid = geteuid(); + gid_t daemon_gid = getegid(); + struct dirperms { const char *dir; int mode; @@ -532,24 +535,24 @@ qemuStateInitializeDirs(bool privileged, }; struct dirperms dirs[] = { /* example: /var/cache/libvirt/qemu */ - { cfg->cacheDir, 0777, -1, -1 }, + { cfg->cacheDir, 0750, daemon_uid, daemon_gid }, /* example: /run/libvirt/qemu */ - { cfg->stateDir, 0777, -1, -1 }, - { cfg->slirpStateDir, 0777, cfg->user, cfg->group }, - { cfg->passtStateDir, 0777, cfg->user, cfg->group }, + { cfg->stateDir, 0755, cfg->user, cfg->group }, + { cfg->slirpStateDir, 0755, cfg->user, cfg->group }, + { cfg->passtStateDir, 0755, cfg->user, cfg->group }, { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, /* example: /var/lib/libvirt/qemu */ - { cfg->libDir, 0777, cfg->user, cfg->group }, - { cfg->saveDir, 0777, cfg->user, cfg->group }, - { cfg->snapshotDir, 0777, cfg->user, cfg->group }, - { cfg->checkpointDir, 0777, cfg->user, cfg->group }, - { cfg->autoDumpPath, 0777, cfg->user, cfg->group }, - { cfg->nvramDir, 0777, cfg->user, cfg->group }, - { cfg->varstoreDir, 0777, cfg->user, cfg->group }, - { cfg->memoryBackingDir, 0777, cfg->user, cfg->group }, + { cfg->libDir, 0751, cfg->user, cfg->group }, + { cfg->saveDir, 0751, cfg->user, cfg->group }, + { cfg->snapshotDir, 0751, cfg->user, cfg->group }, + { cfg->checkpointDir, 0751, cfg->user, cfg->group }, + { cfg->autoDumpPath, 0751, cfg->user, cfg->group }, + { cfg->nvramDir, 0751, cfg->user, cfg->group }, + { cfg->varstoreDir, 0751, cfg->user, cfg->group }, + { cfg->memoryBackingDir, 0751, cfg->user, cfg->group }, }; for (i = 0; i < G_N_ELEMENTS(dirs); i++) { @@ -559,8 +562,7 @@ qemuStateInitializeDirs(bool privileged, return -1; } - if (privileged && - dirs[i].user != -1) { + if (privileged) { if (chown(dirs[i].dir, dirs[i].user, dirs[i].group) < 0) { virReportSystemError(errno, _("unable to set ownership of '%1$s' to %2$d:%3$d"), -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:56 -0400, Cole Robinson via Devel wrote:
The 0777 permission mask we use when creating most 'cfg' dirs does not match what we put on disk via the RPM spec file. Generally those dirs are more locked down.
Match driver startup permissions and owners with what we encode in the RPM spec. Presumably this is safe because this has been tested with real world usage.
The question may be whether packagers from other distros agree. Although I agree that the permissions are a bit saner the way you've changed them.
Some dirs are created here but not in the RPM spec. Leave their permission mask as is, we will deal with them in future patches.
The 2 runtime changes for an RPM installed libvirt is that stateDir is now chown(qemu, qemu) and runDir is now chown(0, 0) where previously there was no chown() calls for these dirs. I don't think that should cause problems
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5dff049d85..f351aab009 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -524,6 +524,9 @@ qemuStateInitializeDirs(bool privileged, { size_t i;
+ uid_t daemon_uid = geteuid(); + gid_t daemon_gid = getegid(); + struct dirperms { const char *dir; int mode; @@ -532,24 +535,24 @@ qemuStateInitializeDirs(bool privileged, }; struct dirperms dirs[] = { /* example: /var/cache/libvirt/qemu */ - { cfg->cacheDir, 0777, -1, -1 }, + { cfg->cacheDir, 0750, daemon_uid, daemon_gid },
/var/cache/libvirt is declared as 711 in libvirt.spec.in [...] To me this looks good, but maybe leave some time for packagers of other distros to have opportunity to chime in. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
rdpStateDir serves a similar purpose to slirpStateDir, just tracking the external process pid. Use the same mask as slirpStateDir Add rdpStateDir to the rpm spec similarly Signed-off-by: Cole Robinson <crobinso@redhat.com> --- libvirt.spec.in | 1 + src/qemu/qemu_driver.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 00316a03f2..258c59e7c5 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2340,6 +2340,7 @@ exit 0 %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/ %ghost %dir %attr(0770, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/dbus/ %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/passt/ +%ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/rdp/ %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/slirp/ %ghost %dir %attr(0770, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/swtpm/ %dir %attr(0751, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f351aab009..cf4f97d104 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -541,7 +541,7 @@ qemuStateInitializeDirs(bool privileged, { cfg->stateDir, 0755, cfg->user, cfg->group }, { cfg->slirpStateDir, 0755, cfg->user, cfg->group }, { cfg->passtStateDir, 0755, cfg->user, cfg->group }, - { cfg->rdpStateDir, 0777, cfg->user, cfg->group }, + { cfg->rdpStateDir, 0755, cfg->user, cfg->group }, { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, /* example: /var/lib/libvirt/qemu */ -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:57 -0400, Cole Robinson via Devel wrote:
rdpStateDir serves a similar purpose to slirpStateDir, just tracking the external process pid. Use the same mask as slirpStateDir Add rdpStateDir to the rpm spec similarly
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- libvirt.spec.in | 1 + src/qemu/qemu_driver.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The only thing created directly in channelTargetDir is domain specific directories (see priv->channelTargetDir). 0755 seems fine here, like other state dirs. Add channelTargetDir to the rpm spec similarly Signed-off-by: Cole Robinson <crobinso@redhat.com> --- libvirt.spec.in | 1 + src/qemu/qemu_driver.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 258c59e7c5..e001a1ac64 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2338,6 +2338,7 @@ exit 0 %config(noreplace) %{_sysconfdir}/libvirt/qemu-lockd.conf %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd.qemu %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/ +%ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/channel/ %ghost %dir %attr(0770, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/dbus/ %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/passt/ %ghost %dir %attr(0755, %{qemu_user}, %{qemu_group}) %{_rundir}/libvirt/qemu/rdp/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cf4f97d104..99b752f7e8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -542,7 +542,7 @@ qemuStateInitializeDirs(bool privileged, { cfg->slirpStateDir, 0755, cfg->user, cfg->group }, { cfg->passtStateDir, 0755, cfg->user, cfg->group }, { cfg->rdpStateDir, 0755, cfg->user, cfg->group }, - { cfg->channelTargetDir, 0777, cfg->user, cfg->group }, + { cfg->channelTargetDir, 0755, cfg->user, cfg->group }, /* example: /var/lib/libvirt/qemu */ { cfg->libDir, 0751, cfg->user, cfg->group }, -- 2.53.0
On Mon, Apr 06, 2026 at 18:16:58 -0400, Cole Robinson via Devel wrote:
The only thing created directly in channelTargetDir is domain specific directories (see priv->channelTargetDir). 0755 seems fine here, like other state dirs.
Add channelTargetDir to the rpm spec similarly
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- libvirt.spec.in | 1 + src/qemu/qemu_driver.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Cole Robinson -
Peter Krempa