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@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;
+
+ rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
+ virCgroupGetDevicePermsString(perms),
+ rv);
+ 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;
+
+ VIR_FREE(devPath);
+ }
}
ret = 0;
cleanup:
+ VIR_FREE(devPath);
+ VIR_FREE(min);
+ VIR_FREE(maj);
VIR_FREE(dst);
return ret;
}
@@ -11131,6 +11158,12 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
virStorageSourcePtr next;
char **paths = NULL;
size_t npaths = 0;
+ unsigned int *maj = NULL;
+ unsigned int *min = NULL;
+ size_t nmaj = 0;
+ char **devmapperPaths = NULL;
+ size_t ndevmapperPaths = 0;
+ size_t i;
int ret = -1;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
@@ -11145,6 +11178,32 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
if (VIR_APPEND_ELEMENT_COPY(paths, npaths, next->path) < 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;
+ }
+
+ if (VIR_REALLOC_N(devmapperPaths, ndevmapperPaths + nmaj) < 0)
+ goto cleanup;
+
+ for (i = 0; i < nmaj; i++) {
+ if (virAsprintf(&devmapperPaths[ndevmapperPaths],
+ "/sys/block/%u:%u",
+ maj[i], min[i]) < 0)
+ goto cleanup;
+ ndevmapperPaths++;
+
+ if (VIR_APPEND_ELEMENT_COPY(paths, npaths,
+ devmapperPaths[ndevmapperPaths - 1]) < 0)
+ goto cleanup;
+ }
+
+ VIR_FREE(min);
+ VIR_FREE(maj);
}
if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0)
@@ -11152,6 +11211,11 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
ret = 0;
cleanup:
+ for (i = 0; i < ndevmapperPaths; i++)
+ VIR_FREE(devmapperPaths[i]);
+ VIR_FREE(devmapperPaths);
+ VIR_FREE(min);
+ VIR_FREE(maj);
VIR_FREE(paths);
return ret;
}
--
2.16.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 26, 2018 at 17:22:01 +0200, Michal Privoznik wrote:
> 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@redhat.com>
> >> ---
[...]
> >> + 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.
Okay. Apart from that I thought about this for a while and also tested
various configurations. Unfortunately I was not able to reproduce the
issue. I realized that this code is allowing the backing devices for all
device-mapper based storage. While I don't think that it is a very big
problem we still might have security implications (e.g. give access to
the whole device, while the device mapper makes the device accessible
only partially). This means that I'd like to have an explanation why it
is necessary in that case to allow the backend devices so that we can
asses whether it's worth doing it always or we can limit the amount of
devices allowed in cgroups.
Also yet another implication is that in the hotunplug code for disks
we'd remove the cgroup for the main device, but this implementation does
not remove the slave devices.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2018 08:56 PM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 17:22:01 +0200, Michal Privoznik wrote:
>> 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@redhat.com>
>>>> ---
>
> [...]
>
>>>> + 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.
>
> Okay. Apart from that I thought about this for a while and also tested
> various configurations. Unfortunately I was not able to reproduce the
> issue.
Looks like this is a kernel bug after all. I've also done some testing
myself and fount that:
a) on upstream kernel I'm unable to reproduce (4.15.11-gentoo),
b) also 7.4 kenel works,
c) it's only 7.5 where this bug presents itself.
https://bugzilla.redhat.com/show_bug.cgi?id=1557769#c32
So I think we should ignore these patches for now until we have some
input from kernel team.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.