On Tue, Apr 03, 2018 at 11:03:26 +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1557769 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@xxxxxxxxxx> > --- > 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
