On Tue, Apr 03, 2018 at 11:03:26 +0200, Michal Privoznik wrote:
This BZ is private. Please don't use links which can't be viewed by the public.
Problem with device mapper targets is that there can be several other devices 'hidden' behind them. For instance, /dev/dm-1 can consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when setting up devices CGroup and namespaces we have to take this into account.
Also I'd appreciate if you could justify this here. Mention that the kernel was fixed and this was supposed to be happening from the beggining.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 ++ src/qemu/qemu_cgroup.c | 69 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 9 deletions(-)
[...]
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b604edb31c..42502e1b03 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c
[...]
@@ -71,12 +75,35 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", path, virCgroupGetDevicePermsString(perms));
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); + rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virCgroupGetDevicePermsString(perms), - ret); + rv); + if (rv < 0) + goto cleanup;
+ if (virDevMapperGetTargets(path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupAllowDevicePath(priv->cgroup, targetPaths[i], perms, false); + + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", targetPaths[i], + virCgroupGetDevicePermsString(perms), + rv); + if (rv < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(targetPaths); return ret; }
This looks okay. I did not check that the 'errnos' returned by libdevmapper are sane given your checks though. [...]
@@ -126,11 +154,34 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
VIR_DEBUG("Deny path %s", src->path);
- ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + rv = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, - virCgroupGetDevicePermsString(perms), ret); + virCgroupGetDevicePermsString(perms), rv); + if (rv < 0) + goto cleanup;
+ if (virDevMapperGetTargets(src->path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + src->path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupDenyDevicePath(priv->cgroup, targetPaths[i], perms, false);
This isn't a good idea. If a backing device of a device mapper volume is shared between two disks, this would rip it out of cgroups even if it would be still used. I think a simple reproducer for this should be if you use two LVs in the same VG for backing two disks and hot-unplug one of them.