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

Peter Krempa pkrempa at redhat.com
Mon Mar 26 15:17:53 UTC 2018


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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180326/eabf1fb4/attachment-0001.sig>


More information about the libvir-list mailing list