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 <mpriv...@redhat.com> > --- > 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.
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
