On 03/26/2018 05:17 PM, Peter Krempa wrote: > On Mon, Mar 26, 2018 at 16:43:02 +0200, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1557769 >> >> 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. >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> libvirt.spec.in | 2 ++ >> src/qemu/qemu_cgroup.c | 42 ++++++++++++++++++++++++++++++--- >> src/qemu/qemu_domain.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 105 insertions(+), 3 deletions(-) >> >> diff --git a/libvirt.spec.in b/libvirt.spec.in >> index b55a947ec9..ebfac10866 100644 >> --- a/libvirt.spec.in >> +++ b/libvirt.spec.in >> @@ -796,6 +796,8 @@ Requires: gzip >> Requires: bzip2 >> Requires: lzop >> Requires: xz >> +# For mpath devices >> +Requires: device-mapper >> %if 0%{?fedora} || 0%{?rhel} > 7 >> Requires: systemd-container >> %endif >> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c >> index b604edb31c..e17b3d21b5 100644 >> --- a/src/qemu/qemu_cgroup.c >> +++ b/src/qemu/qemu_cgroup.c >> @@ -37,6 +37,7 @@ >> #include "virtypedparam.h" >> #include "virnuma.h" >> #include "virsystemd.h" >> +#include "virdevmapper.h" >> >> #define VIR_FROM_THIS VIR_FROM_QEMU >> >> @@ -60,7 +61,13 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, >> { >> qemuDomainObjPrivatePtr priv = vm->privateData; >> int perms = VIR_CGROUP_DEVICE_READ; >> - int ret; >> + unsigned int *maj = NULL; >> + unsigned int *min = NULL; >> + size_t nmaj = 0; >> + size_t i; >> + char *devPath = NULL; >> + int rv; >> + int ret = -1; >> >> if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) >> return 0; >> @@ -71,12 +78,41 @@ 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, &maj, &min, &nmaj) < 0 && >> + errno != ENOSYS && errno != EBADF) { >> + virReportSystemError(errno, >> + _("Unable to get devmapper targets for %s"), >> + path); >> + goto cleanup; >> + } >> + >> + for (i = 0; i < nmaj; i++) { >> + if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0) >> + goto cleanup; > > So since this path will not help that much in the audit logs ... > >> + >> + rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true); > > ... you probably should use virCgroupAllowDevice ... > >> + >> + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath, >> + virCgroupGetDevicePermsString(perms), >> + rv); > > ... and add an equivalent of virDomainAuditCgroupMajor that takes both > major:minor similarly to virDomainAuditCgroupMajor. > >> + if (rv < 0) >> + goto cleanup; >> + VIR_FREE(devPath); >> + } >> + >> + ret = 0; >> + cleanup: >> + VIR_FREE(devPath); >> + VIR_FREE(min); >> + VIR_FREE(maj); >> return ret; >> } >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 580e0f830d..5f56324502 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -54,6 +54,7 @@ >> #include "secret_util.h" >> #include "logging/log_manager.h" >> #include "locking/domain_lock.h" >> +#include "virdevmapper.h" >> >> #ifdef MAJOR_IN_MKDEV >> # include <sys/mkdev.h> >> @@ -10108,6 +10109,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, >> { >> virStorageSourcePtr next; >> char *dst = NULL; >> + unsigned int *maj = NULL; >> + unsigned int *min = NULL; >> + size_t nmaj = 0; >> + char *devPath = NULL; >> + size_t i; >> int ret = -1; >> >> for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) { >> @@ -10118,10 +10124,31 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, >> >> if (qemuDomainCreateDevice(next->path, data, false) < 0) >> goto cleanup; >> + >> + if (virDevMapperGetTargets(next->path, &maj, &min, &nmaj) < 0 && >> + errno != ENOSYS && errno != EBADF) { >> + virReportSystemError(errno, >> + _("Unable to get mpath targets for %s"), >> + next->path); >> + goto cleanup; >> + } >> + >> + for (i = 0; i < nmaj; i++) { >> + if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0) >> + goto cleanup; >> + >> + if (qemuDomainCreateDevice(devPath, data, false) < 0) >> + goto cleanup; > > So now that I see this new version, this part starts looking suspicious > to me. Since this did not care much that the path changed, is it really > necessary to create the /dev/ entries in the container? > > Looks like even device mapper is returning them as the node > specificator, so I'd presume it really does not matter if they are > present. > > More specifically we can't really reverse engineer from the major:minor > numbers which actual path the user used so it should not really be > necessary for it to be present in the container. Yes, looks like I was too eager trying to fix this bug. I've rebuilt libvirt without qemu_domain.c change (so only CGroup code was modified) and the bug still did not reproduce. So I guess namespace changes are not necessary after all. I'll drop them. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
