[libvirt] [PATCH v2 0/3] Fix case with non-root domain and hugepages

v2 of: https://www.redhat.com/archives/libvir-list/2016-November/msg01060.html diff to v1: - use virDomainObjGetShortName to construct hugepages path - instead of implementing virSecurityManagerSetHugepages drop it Michal Privoznik (3): virDomainObjGetShortName: take virDomainDef qemu: Create hugepage path on per domain basis security: Drop virSecurityManagerSetHugepages src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 4 +- src/qemu/qemu_conf.c | 44 +++++++++---- src/qemu/qemu_conf.h | 16 +++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 21 ++---- src/qemu/qemu_process.c | 74 ++++++++++++++++------ src/security/security_driver.h | 1 - src/security/security_manager.c | 17 ----- src/security/security_manager.h | 3 - src/security/security_stack.c | 19 ------ .../qemuxml2argv-hugepages-numa.args | 6 +- .../qemuxml2argv-hugepages-pages.args | 16 ++--- .../qemuxml2argv-hugepages-pages2.args | 2 +- .../qemuxml2argv-hugepages-pages3.args | 2 +- .../qemuxml2argv-hugepages-pages5.args | 2 +- .../qemuxml2argv-hugepages-shared.args | 14 ++-- tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- 22 files changed, 136 insertions(+), 124 deletions(-) -- 2.8.4

So far this function takes virDomainObjPtr which: 1) is an overkill, 2) might be not available in all the places we will use it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5d2bc8d..4361abf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25713,13 +25713,13 @@ virDomainDefHasMemballoon(const virDomainDef *def) * Shorten domain name to avoid possible path length limitations. */ char * -virDomainObjGetShortName(virDomainObjPtr vm) +virDomainObjGetShortName(const virDomainDef *def) { const int dommaxlen = 20; char *ret = NULL; ignore_value(virAsprintf(&ret, "%d-%.*s", - vm->def->id, dommaxlen, vm->def->name)); + def->id, dommaxlen, def->name)); return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3dfd780..1d094ae 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3202,7 +3202,7 @@ int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1); -char *virDomainObjGetShortName(virDomainObjPtr vm); +char *virDomainObjGetShortName(const virDomainDef *def) ATTRIBUTE_NONNULL(1); int virDomainGetBlkioParametersAssignFromDef(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 47332a8..35c6cb0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1475,7 +1475,7 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; - char *domname = virDomainObjGetShortName(vm); + char *domname = virDomainObjGetShortName(vm->def); int ret = -1; if (!domname) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..29ed16e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3880,7 +3880,7 @@ getAutoDumpPath(virQEMUDriverPtr driver, virDomainObjPtr vm) { char *dumpfile = NULL; - char *domname = virDomainObjGetShortName(vm); + char *domname = virDomainObjGetShortName(vm->def); char timestr[100]; struct tm time_info; time_t curtime = time(NULL); -- 2.8.4

On Tue, Nov 29, 2016 at 10:31:11AM +0100, Michal Privoznik wrote:
So far this function takes virDomainObjPtr which: 1) is an overkill, 2) might be not available in all the places we will use it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
ACK Jan

If you've ever tried running a huge page backed guest under different user than root, you probably failed. Problem is even though we have corresponding APIs in the security drivers, there's no implementation and thus we don't relabel the huge page path. But even if we did, so far all of the domains share the same path: /hugepageMount/libvirt/qemu Our only option there would be to set 0777 mode on the qemu dir which is totally unsafe. Therefore, we can create dir on per-domain basis, i.e.: /hugepageMount/libvirt/qemu/domainName and chown domainName dir to the user that domain is configured to run under. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 4 +- src/qemu/qemu_conf.c | 44 +++++++++---- src/qemu/qemu_conf.h | 16 +++-- src/qemu/qemu_driver.c | 19 ++---- src/qemu/qemu_process.c | 74 ++++++++++++++++------ .../qemuxml2argv-hugepages-numa.args | 6 +- .../qemuxml2argv-hugepages-pages.args | 16 ++--- .../qemuxml2argv-hugepages-pages2.args | 2 +- .../qemuxml2argv-hugepages-pages3.args | 2 +- .../qemuxml2argv-hugepages-pages5.args | 2 +- .../qemuxml2argv-hugepages-shared.args | 14 ++-- tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- 14 files changed, 131 insertions(+), 78 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6c457dd..10b6d45 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3303,7 +3303,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, return -1; if (pagesize) { - if (qemuGetHupageMemPath(cfg, pagesize, &mem_path) < 0) + if (qemuGetDomainHupageMemPath(def, cfg, pagesize, &mem_path) < 0) goto cleanup; *backendType = "memory-backend-file"; @@ -7257,7 +7257,7 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; } - if (qemuGetHupageMemPath(cfg, def->mem.hugepages[0].size, &mem_path) < 0) + if (qemuGetDomainHupageMemPath(def, cfg, def->mem.hugepages[0].size, &mem_path) < 0) return -1; virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ccefbe8..ad85551 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1456,7 +1456,7 @@ qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn ATTRIBUTE_UNUSED, } char * -qemuGetHugepagePath(virHugeTLBFSPtr hugepage) +qemuGetBaseHugepagePath(virHugeTLBFSPtr hugepage) { char *ret; @@ -1467,8 +1467,25 @@ qemuGetHugepagePath(virHugeTLBFSPtr hugepage) } +char * +qemuGetDomainHugepagePath(const virDomainDef *def, + virHugeTLBFSPtr hugepage) +{ + char *base = qemuGetBaseHugepagePath(hugepage); + char *domPath = virDomainObjGetShortName(def); + char *ret; + + if (base && domPath) + ignore_value(virAsprintf(&ret, "%s/%s", base, domPath)); + VIR_FREE(domPath); + VIR_FREE(base); + return ret; +} + + /** - * qemuGetDefaultHugepath: + * qemuGetDomainDefaultHugepath: + * @def: domain definition * @hugetlbfs: array of configured hugepages * @nhugetlbfs: number of item in the array * @@ -1477,8 +1494,9 @@ qemuGetHugepagePath(virHugeTLBFSPtr hugepage) * Returns 0 on success, -1 otherwise. * */ char * -qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, - size_t nhugetlbfs) +qemuGetDomainDefaultHugepath(const virDomainDef *def, + virHugeTLBFSPtr hugetlbfs, + size_t nhugetlbfs) { size_t i; @@ -1489,12 +1507,12 @@ qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, if (i == nhugetlbfs) i = 0; - return qemuGetHugepagePath(&hugetlbfs[i]); + return qemuGetDomainHugepagePath(def, &hugetlbfs[i]); } /** - * qemuGetHupageMemPath: Construct HP enabled memory backend path + * qemuGetDomainHupageMemPath: Construct HP enabled memory backend path * * If no specific hugepage size is requested (@pagesize is zero) * the default hugepage size is used). @@ -1504,9 +1522,10 @@ qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, * -1 otherwise. */ int -qemuGetHupageMemPath(virQEMUDriverConfigPtr cfg, - unsigned long long pagesize, - char **memPath) +qemuGetDomainHupageMemPath(const virDomainDef *def, + virQEMUDriverConfigPtr cfg, + unsigned long long pagesize, + char **memPath) { size_t i = 0; @@ -1518,8 +1537,9 @@ qemuGetHupageMemPath(virQEMUDriverConfigPtr cfg, } if (!pagesize) { - if (!(*memPath = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) + if (!(*memPath = qemuGetDomainDefaultHugepath(def, + cfg->hugetlbfs, + cfg->nhugetlbfs))) return -1; } else { for (i = 0; i < cfg->nhugetlbfs; i++) { @@ -1535,7 +1555,7 @@ qemuGetHupageMemPath(virQEMUDriverConfigPtr cfg, return -1; } - if (!(*memPath = qemuGetHugepagePath(&cfg->hugetlbfs[i]))) + if (!(*memPath = qemuGetDomainHugepagePath(def, &cfg->hugetlbfs[i]))) return -1; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index f6e3257..d191e10 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -335,11 +335,15 @@ virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver); int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn, virDomainSnapshotDiskDefPtr def); -char * qemuGetHugepagePath(virHugeTLBFSPtr hugepage); -char * qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, - size_t nhugetlbfs); +char * qemuGetBaseHugepagePath(virHugeTLBFSPtr hugepage); +char * qemuGetDomainHugepagePath(const virDomainDef *def, + virHugeTLBFSPtr hugepage); +char * qemuGetDomainDefaultHugepath(const virDomainDef *def, + virHugeTLBFSPtr hugetlbfs, + size_t nhugetlbfs); -int qemuGetHupageMemPath(virQEMUDriverConfigPtr cfg, - unsigned long long pagesize, - char **memPath); +int qemuGetDomainHupageMemPath(const virDomainDef *def, + virQEMUDriverConfigPtr cfg, + unsigned long long pagesize, + char **memPath); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 29ed16e..b0164e8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -852,7 +852,7 @@ qemuStateInitialize(bool privileged, * it, since we can't assume the root mount point has permissions that * will let our spawned QEMU instances use it. */ for (i = 0; i < cfg->nhugetlbfs; i++) { - hugepagePath = qemuGetHugepagePath(&cfg->hugetlbfs[i]); + hugepagePath = qemuGetBaseHugepagePath(&cfg->hugetlbfs[i]); if (!hugepagePath) goto error; @@ -863,19 +863,10 @@ qemuStateInitialize(bool privileged, hugepagePath); goto error; } - if (privileged) { - if (virFileUpdatePerm(cfg->hugetlbfs[i].mnt_dir, - 0, S_IXGRP | S_IXOTH) < 0) - goto error; - if (chown(hugepagePath, cfg->user, cfg->group) < 0) { - virReportSystemError(errno, - _("unable to set ownership on %s to %d:%d"), - hugepagePath, - (int) cfg->user, - (int) cfg->group); - goto error; - } - } + if (privileged && + virFileUpdatePerm(cfg->hugetlbfs[i].mnt_dir, + 0, S_IXGRP | S_IXOTH) < 0) + goto error; VIR_FREE(hugepagePath); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4b82a97..5e179b9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3221,6 +3221,55 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr vm) } +static int +qemuProcessBuildDestroydHugepagesPath(virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool build) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *hugepagePath = NULL; + size_t i; + int ret = -1; + + if (vm->def->mem.nhugepages) { + for (i = 0; i < cfg->nhugetlbfs; i++) { + VIR_FREE(hugepagePath); + hugepagePath = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]); + + if (!hugepagePath) + goto cleanup; + + if (build) { + if (virFileMakePathWithMode(hugepagePath, 0700) < 0) { + virReportSystemError(errno, + _("Unable to create %s"), + hugepagePath); + goto cleanup; + } + + if (virSecurityManagerSetHugepages(driver->securityManager, + vm->def, hugepagePath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unable to set huge path in security driver")); + VIR_FREE(hugepagePath); + goto cleanup; + } + } else { + if (rmdir(hugepagePath) < 0) + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)", + hugepagePath, errno); + } + } + } + + ret = 0; + cleanup: + VIR_FREE(hugepagePath); + virObjectUnref(cfg); + return ret; +} + + struct qemuProcessReconnectData { virConnectPtr conn; virQEMUDriverPtr driver; @@ -3365,6 +3414,9 @@ qemuProcessReconnect(void *opaque) goto cleanup; } + if (qemuProcessBuildDestroydHugepagesPath(driver, obj, true) < 0) + goto error; + if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj, false)) < 0) goto error; @@ -5227,7 +5279,6 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, { int ret = -1; unsigned int hostdev_flags = 0; - size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -5259,23 +5310,8 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (vm->def->mem.nhugepages) { - for (i = 0; i < cfg->nhugetlbfs; i++) { - char *hugepagePath = qemuGetHugepagePath(&cfg->hugetlbfs[i]); - - if (!hugepagePath) - goto cleanup; - - if (virSecurityManagerSetHugepages(driver->securityManager, - vm->def, hugepagePath) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to set huge path in security driver")); - VIR_FREE(hugepagePath); - goto cleanup; - } - VIR_FREE(hugepagePath); - } - } + if (qemuProcessBuildDestroydHugepagesPath(driver, vm, true) < 0) + goto cleanup; /* Ensure no historical cgroup for this VM is lying around bogus * settings */ @@ -5953,6 +5989,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, goto endjob; } + qemuProcessBuildDestroydHugepagesPath(driver, vm, false); + vm->def->id = -1; if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 7b90784..26e9496 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -11,11 +11,11 @@ QEMU_AUDIO_DRV=spice \ -m size=1048576k,slots=16,maxmem=1099511627776k \ -smp 2,sockets=2,cores=1,threads=1 \ -mem-prealloc \ --mem-path /dev/hugepages2M/libvirt/qemu \ +-mem-path /dev/hugepages2M/libvirt/qemu/-1-fedora \ -numa node,nodeid=0,cpus=0-1,mem=1024 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-fedora,size=1073741824,\ +host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \ -uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ -nodefaults \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args index 9f0e696..4c11bb7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args @@ -11,20 +11,20 @@ QEMU_AUDIO_DRV=none \ -m 4096 \ -smp 4,sockets=4,cores=1,threads=1 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ -object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args index a37f03d..72a1d8c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args @@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \ -m 1024 \ -smp 2,sockets=2,cores=1,threads=1 \ -mem-prealloc \ --mem-path /dev/hugepages2M/libvirt/qemu \ +-mem-path /dev/hugepages2M/libvirt/qemu/-1-SomeDummyHugepagesGu \ -numa node,nodeid=0,cpus=0,mem=256 \ -numa node,nodeid=1,cpus=1,mem=768 \ -uuid ef1bdff4-27f3-4e85-a807-5fb4d58463cc \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args index 57dd3fa..75cb1de 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -13,7 +13,7 @@ QEMU_AUDIO_DRV=none \ -object memory-backend-ram,id=ram-node0,size=268435456 \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=805306368 \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-SomeDummyHugepagesGu,size=805306368 \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -uuid ef1bdff4-27f3-4e85-a807-5fb4d58463cc \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args index a826149..fb9fa8a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args @@ -10,7 +10,7 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 1024 \ -mem-prealloc \ --mem-path /dev/hugepages2M/libvirt/qemu \ +-mem-path /dev/hugepages2M/libvirt/qemu/-1-SomeDummyHugepagesGu \ -smp 2,sockets=2,cores=1,threads=1 \ -uuid ef1bdff4-27f3-4e85-a807-5fb4d58463cc \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args index f9fc218..27a6d84 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-shared.args @@ -11,20 +11,20 @@ QEMU_AUDIO_DRV=none \ -m 4096 \ -smp 4,sockets=4,cores=1,threads=1 \ -object memory-backend-file,id=ram-node0,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=0,cpus=0,memdev=ram-node0 \ -object memory-backend-file,id=ram-node1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,share=yes,size=1073741824,\ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,share=yes,size=1073741824,\ host-nodes=0-3,policy=bind \ -numa node,nodeid=1,cpus=1,memdev=ram-node1 \ -object memory-backend-file,id=ram-node2,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,share=no,size=1073741824,host-nodes=0-3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,share=no,size=1073741824,\ +host-nodes=0-3,policy=bind \ -numa node,nodeid=2,cpus=2,memdev=ram-node2 \ -object memory-backend-file,id=ram-node3,prealloc=yes,\ -mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=3,\ -policy=bind \ +mem-path=/dev/hugepages1G/libvirt/qemu/-1-QEMUGuest1,size=1073741824,\ +host-nodes=3,policy=bind \ -numa node,nodeid=3,cpus=3,memdev=ram-node3 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages.args index 361c8e5..52d04cd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages.args @@ -10,7 +10,7 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 214 \ -mem-prealloc \ --mem-path /dev/hugepages2M/libvirt/qemu \ +-mem-path /dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1 \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args index fdbb4c3..6825fa5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args @@ -12,8 +12,8 @@ QEMU_AUDIO_DRV=none \ -smp 2,sockets=2,cores=1,threads=1 \ -numa node,nodeid=0,cpus=0-1,mem=214 \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\ -policy=bind \ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,size=536870912,\ +host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0,addr=4294967296 \ -object memory-backend-ram,id=memdimm2,size=536870912 \ -device pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args index 1587aba..321e64e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args @@ -14,8 +14,8 @@ QEMU_AUDIO_DRV=none \ -object memory-backend-ram,id=memdimm0,size=536870912 \ -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \ -object memory-backend-file,id=memdimm1,prealloc=yes,\ -mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\ -policy=bind \ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,size=536870912,\ +host-nodes=1-3,policy=bind \ -device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -- 2.8.4

On Tue, Nov 29, 2016 at 10:31:12AM +0100, Michal Privoznik wrote:
If you've ever tried running a huge page backed guest under different user than root, you probably failed. Problem is even
Should this be: different than the user in qemu.conf?
though we have corresponding APIs in the security drivers, there's no implementation and thus we don't relabel the huge page path. But even if we did, so far all of the domains share the same path:
/hugepageMount/libvirt/qemu
Our only option there would be to set 0777 mode on the qemu dir which is totally unsafe. Therefore, we can create dir on per-domain basis, i.e.:
/hugepageMount/libvirt/qemu/domainName
and chown domainName dir to the user that domain is configured to run under.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 4 +- src/qemu/qemu_conf.c | 44 +++++++++---- src/qemu/qemu_conf.h | 16 +++-- src/qemu/qemu_driver.c | 19 ++---- src/qemu/qemu_process.c | 74 ++++++++++++++++------ .../qemuxml2argv-hugepages-numa.args | 6 +- .../qemuxml2argv-hugepages-pages.args | 16 ++--- .../qemuxml2argv-hugepages-pages2.args | 2 +- .../qemuxml2argv-hugepages-pages3.args | 2 +- .../qemuxml2argv-hugepages-pages5.args | 2 +- .../qemuxml2argv-hugepages-shared.args | 14 ++-- tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- 14 files changed, 131 insertions(+), 78 deletions(-)
@@ -3221,6 +3221,55 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr vm) }
+static int +qemuProcessBuildDestroydHugepagesPath(virQEMUDriverPtr driver,
Extra 'd' after Destroy.
+ virDomainObjPtr vm,
Only vm->def is needed.
+ bool build)
This would look nicer split in two functions. And the 'Destroy' one can be void and best-effort.
+{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *hugepagePath = NULL; + size_t i; + int ret = -1; + + if (vm->def->mem.nhugepages) { + for (i = 0; i < cfg->nhugetlbfs; i++) { + VIR_FREE(hugepagePath); + hugepagePath = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]); + + if (!hugepagePath) + goto cleanup;
In theory, this does not need to be an error for the Destroy path, we can just continue to the next mount. In practice, this will never fail.
+ + if (build) { + if (virFileMakePathWithMode(hugepagePath, 0700) < 0) { + virReportSystemError(errno, + _("Unable to create %s"), + hugepagePath); + goto cleanup; + } + + if (virSecurityManagerSetHugepages(driver->securityManager, + vm->def, hugepagePath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unable to set huge path in security driver"));
Pre-existing error message oddness.
+ VIR_FREE(hugepagePath);
Already done in 'cleanup'.
+ goto cleanup; + } + } else { + if (rmdir(hugepagePath) < 0) + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)", + hugepagePath, errno);
Can be VIR_ERROR, this will never happen(TM).
+ } + } + } + + ret = 0; + cleanup: + VIR_FREE(hugepagePath); + virObjectUnref(cfg); + return ret; +} + +
ACK Jan

Since its introduction in 2012 this internal API did nothing. Moreover we have the same API that does exactly the same: virSecurityManagerDomainSetPathLabel. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/qemu/qemu_process.c | 4 ++-- src/security/security_driver.h | 1 - src/security/security_manager.c | 17 ----------------- src/security/security_manager.h | 3 --- src/security/security_stack.c | 19 ------------------- 6 files changed, 2 insertions(+), 43 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cc7bf8..04c18bf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1164,7 +1164,6 @@ virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; virSecurityManagerSetDiskLabel; virSecurityManagerSetHostdevLabel; -virSecurityManagerSetHugepages; virSecurityManagerSetImageFDLabel; virSecurityManagerSetImageLabel; virSecurityManagerSetProcessLabel; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5e179b9..47e1fe2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3247,8 +3247,8 @@ qemuProcessBuildDestroydHugepagesPath(virQEMUDriverPtr driver, goto cleanup; } - if (virSecurityManagerSetHugepages(driver->securityManager, - vm->def, hugepagePath) < 0) { + if (virSecurityManagerDomainSetPathLabel(driver->securityManager, + vm->def, hugepagePath) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to set huge path in security driver")); VIR_FREE(hugepagePath); diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 7cb62f0..b48f2aa 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -168,7 +168,6 @@ struct _virSecurityDriver { virSecurityDomainSetTapFDLabel domainSetSecurityTapFDLabel; virSecurityDomainGetMountOptions domainGetSecurityMountOptions; - virSecurityDomainSetHugepages domainSetSecurityHugepages; virSecurityDriverGetBaseLabel getBaseLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ecb4a40..f98c7d2 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -970,23 +970,6 @@ virSecurityManagerGetNested(virSecurityManagerPtr mgr) int -virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *path) -{ - if (mgr->drv->domainSetSecurityHugepages) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityHugepages(mgr, vm, path); - virObjectUnlock(mgr); - return ret; - } - - return 0; -} - - -int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *path) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 4cbc2d8..0d22cb1 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -149,9 +149,6 @@ int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, virDomainDefPtr vm); virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr); -int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr sec, - const char *hugepages_path); int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 3ea2751..c123931 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -511,23 +511,6 @@ virSecurityStackSetTapFDLabel(virSecurityManagerPtr mgr, return rc; } -static int -virSecurityStackSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *path) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; - - for (; item; item = item->next) { - if (virSecurityManagerSetHugepages(item->securityManager, vm, path) < 0) - rc = -1; - } - - return rc; -} - static char * virSecurityStackGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED) @@ -663,8 +646,6 @@ virSecurityDriver virSecurityDriverStack = { .domainGetSecurityMountOptions = virSecurityStackGetMountOptions, - .domainSetSecurityHugepages = virSecurityStackSetHugepages, - .getBaseLabel = virSecurityStackGetBaseLabel, .domainSetPathLabel = virSecurityStackDomainSetPathLabel, -- 2.8.4

On Tue, Nov 29, 2016 at 10:31:13AM +0100, Michal Privoznik wrote:
Since its introduction in 2012 this internal API did nothing. Moreover we have the same API that does exactly the same: virSecurityManagerDomainSetPathLabel.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/qemu/qemu_process.c | 4 ++-- src/security/security_driver.h | 1 - src/security/security_manager.c | 17 ----------------- src/security/security_manager.h | 3 --- src/security/security_stack.c | 19 ------------------- 6 files changed, 2 insertions(+), 43 deletions(-)
ACK; beautiful. Jan
participants (2)
-
Ján Tomko
-
Michal Privoznik