Re: [libvirt] [PATCH v2 2/3] qemu: Handle device mapper targets properly

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(a)redhat.com&gt; > --- > 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.