[libvirt] [PATCH 0/2] qemu: Update CGroups on some devices hotplug
We are already doing that for some devices (e.g. disks, hostdevs), but completely forgot about some others (chardevs and RNGs). So far the hotplug worked by pure chance as /dev/random and /dev/urandom are enabled in the device CGroups by default. Michal Privoznik (2): qemu: Update cgroup on RNG hotplug qemu: Update cgroup on chardev hotplug src/qemu/qemu_cgroup.c | 108 +++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_cgroup.h | 8 ++++ src/qemu/qemu_hotplug.c | 37 ++++++++++++++--- 3 files changed, 131 insertions(+), 22 deletions(-) -- 2.8.4
If users try to hotplug RNG device with a backend different to /dev/random or /dev/urandom the whole operation fails as qemu is unable to access the device. The problem is we don't update device CGroups during the operation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 62 +++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_cgroup.h | 4 ++++ src/qemu/qemu_hotplug.c | 17 ++++++++++++-- 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1443f7e..e7ce032 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -569,6 +569,54 @@ qemuSetupFirmwareCgroup(virDomainObjPtr vm) } +int +qemuSetupRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { + VIR_DEBUG("Setting Cgroup ACL for RNG device"); + rv = virCgroupAllowDevicePath(priv->cgroup, + rng->source.file, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", + rng->source.file, + "rw", rv == 0); + if (rv < 0 && + !virLastErrorIsSystemErrno(ENOENT)) + return -1; + } + + return 0; +} + + +int +qemuTeardownRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { + VIR_DEBUG("Setting Cgroup ACL for RNG device"); + rv = virCgroupDenyDevicePath(priv->cgroup, + rng->source.file, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + rng->source.file, + "rw", rv == 0); + if (rv < 0 && + !virLastErrorIsSystemErrno(ENOENT)) + return -1; + } + + return 0; +} + + static int qemuSetupDevicesCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm) @@ -663,18 +711,8 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, } for (i = 0; i < vm->def->nrngs; i++) { - if (vm->def->rngs[i]->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { - VIR_DEBUG("Setting Cgroup ACL for RNG device"); - rv = virCgroupAllowDevicePath(priv->cgroup, - vm->def->rngs[i]->source.file, - VIR_CGROUP_DEVICE_RW, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", - vm->def->rngs[i]->source.file, - "rw", rv == 0); - if (rv < 0 && - !virLastErrorIsSystemErrno(ENOENT)) - goto cleanup; - } + if (qemuSetupRNGCgroup(vm, vm->def->rngs[i]) < 0) + goto cleanup; } ret = 0; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 623823e..1c3b7ff 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -43,6 +43,10 @@ int qemuSetupHostdevCgroup(virDomainObjPtr vm, int qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuSetupRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng); +int qemuTeardownRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng); int qemuConnectCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm); int qemuSetupCgroup(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6b3a068..b43d9dd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1951,6 +1951,7 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, char *tlsAlias = NULL; char *secAlias = NULL; bool releaseaddr = false; + bool teardowncgroup = false; bool chardevAdded = false; bool objAdded = false; bool tlsobjAdded = false; @@ -1996,6 +1997,10 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, goto cleanup; } + if (qemuSetupRNGCgroup(vm, rng) < 0) + goto cleanup; + teardowncgroup = true; + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) qemuDomainPrepareChardevSourceTLS(rng->source.chardev, cfg); @@ -2073,8 +2078,13 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, virJSONValueFree(tlsProps); virJSONValueFree(secProps); virJSONValueFree(props); - if (ret < 0 && releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); + if (ret < 0) { + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); + if (teardowncgroup && qemuTeardownRNGCgroup(vm, rng) < 0) + VIR_WARN("Unable to remove RNG device cgroup ACL on hotplug fail"); + } + VIR_FREE(tlsAlias); VIR_FREE(secAlias); VIR_FREE(charAlias); @@ -3912,6 +3922,9 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, if (rc < 0) goto cleanup; + if (qemuTeardownRNGCgroup(vm, rng) < 0) + VIR_WARN("Failed to remove RNG device cgroup ACL"); + event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias); qemuDomainEventQueue(driver, event); -- 2.8.4
[...]
+int +qemuTeardownRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { + VIR_DEBUG("Setting Cgroup ACL for RNG device");
s/Setting/Tearing down/ (or something similar - Unsetting, Removing) Looks reasonable otherwise, ACK John
+ rv = virCgroupDenyDevicePath(priv->cgroup, + rng->source.file, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + rng->source.file, + "rw", rv == 0); + if (rv < 0 && + !virLastErrorIsSystemErrno(ENOENT)) + return -1; + } + + return 0; +} + + [...]
On 23.11.2016 15:20, John Ferlan wrote:
[...]
+int +qemuTeardownRNGCgroup(virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { + VIR_DEBUG("Setting Cgroup ACL for RNG device");
s/Setting/Tearing down/
(or something similar - Unsetting, Removing)
Looks reasonable otherwise,
Ah, right.
ACK
Thank you. I've just pushed both. Michal
Just like in the previous commit, we are not updating CGroups on chardev hot(un-)plug and thus leaving qemu unable to access any non-default device users are trying to hotplug. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_cgroup.h | 4 ++++ src/qemu/qemu_hotplug.c | 20 ++++++++++++++++---- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e7ce032..50e3d35 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -189,10 +189,32 @@ qemuSetupChrSourceCgroup(virDomainObjPtr vm, return ret; } + static int -qemuSetupChardevCgroup(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainChrDefPtr dev, - void *opaque) +qemuTeardownChrSourceCgroup(virDomainObjPtr vm, + virDomainChrSourceDefPtr source) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (source->type != VIR_DOMAIN_CHR_TYPE_DEV) + return 0; + + VIR_DEBUG("Process path '%s' for device", source->data.file.path); + + ret = virCgroupDenyDevicePath(priv->cgroup, source->data.file.path, + VIR_CGROUP_DEVICE_RW, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + source->data.file.path, "rw", ret == 0); + + return ret; +} + + +static int +qemuSetupChardevCgroupCB(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque) { virDomainObjPtr vm = opaque; @@ -617,6 +639,22 @@ qemuTeardownRNGCgroup(virDomainObjPtr vm, } +int +qemuSetupChardevCgroup(virDomainObjPtr vm, + virDomainChrDefPtr dev) +{ + return qemuSetupChrSourceCgroup(vm, dev->source); +} + + +int +qemuTeardownChardevCgroup(virDomainObjPtr vm, + virDomainChrDefPtr dev) +{ + return qemuTeardownChrSourceCgroup(vm, dev->source); +} + + static int qemuSetupDevicesCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm) @@ -693,7 +731,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, if (virDomainChrDefForeach(vm->def, true, - qemuSetupChardevCgroup, + qemuSetupChardevCgroupCB, vm) < 0) goto cleanup; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 1c3b7ff..6e2c742 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -47,6 +47,10 @@ int qemuSetupRNGCgroup(virDomainObjPtr vm, virDomainRNGDefPtr rng); int qemuTeardownRNGCgroup(virDomainObjPtr vm, virDomainRNGDefPtr rng); +int qemuSetupChardevCgroup(virDomainObjPtr vm, + virDomainChrDefPtr dev); +int qemuTeardownChardevCgroup(virDomainObjPtr vm, + virDomainChrDefPtr dev); int qemuConnectCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm); int qemuSetupCgroup(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b43d9dd..5038ce1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1830,6 +1830,7 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, char *charAlias = NULL; bool chardevAttached = false; bool tlsobjAdded = false; + bool teardowncgroup = false; bool secobjAdded = false; virJSONValuePtr tlsProps = NULL; char *tlsAlias = NULL; @@ -1851,6 +1852,10 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, if (rc == 1) need_release = true; + if (qemuSetupChardevCgroup(vm, chr) < 0) + goto cleanup; + teardowncgroup = true; + if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; @@ -1903,10 +1908,14 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, audit: virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: - if (ret < 0 && virDomainObjIsActive(vm)) - qemuDomainChrInsertPreAllocCleanup(vmdef, chr); - if (ret < 0 && need_release) - qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); + if (ret < 0) { + if (virDomainObjIsActive(vm)) + qemuDomainChrInsertPreAllocCleanup(vmdef, chr); + if (need_release) + qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); + if (teardowncgroup && qemuTeardownChardevCgroup(vm, chr) < 0) + VIR_WARN("Unable to remove chr device cgroup ACL on hotplug fail"); + } VIR_FREE(tlsAlias); virJSONValueFree(tlsProps); VIR_FREE(secAlias); @@ -3847,6 +3856,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (rc < 0) goto cleanup; + if (qemuTeardownChardevCgroup(vm, chr) < 0) + VIR_WARN("Failed to remove chr device cgroup ACL"); + event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); qemuDomainEventQueue(driver, event); -- 2.8.4
On 11/18/2016 06:30 AM, Michal Privoznik wrote:
Just like in the previous commit, we are not updating CGroups on chardev hot(un-)plug and thus leaving qemu unable to access any non-default device users are trying to hotplug.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_cgroup.h | 4 ++++ src/qemu/qemu_hotplug.c | 20 ++++++++++++++++---- 3 files changed, 62 insertions(+), 8 deletions(-)
ACK, John
On 18.11.2016 12:30, Michal Privoznik wrote:
We are already doing that for some devices (e.g. disks, hostdevs), but completely forgot about some others (chardevs and RNGs). So far the hotplug worked by pure chance as /dev/random and /dev/urandom are enabled in the device CGroups by default.
Michal Privoznik (2): qemu: Update cgroup on RNG hotplug qemu: Update cgroup on chardev hotplug
src/qemu/qemu_cgroup.c | 108 +++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_cgroup.h | 8 ++++ src/qemu/qemu_hotplug.c | 37 ++++++++++++++--- 3 files changed, 131 insertions(+), 22 deletions(-)
ping? It would be nice to have these in before the freeze. Michal
participants (2)
-
John Ferlan -
Michal Privoznik