3:31 a.m.
These were sent to the libvirt-security list, where they were reviewed.
And before that, I've sent them to the public list:
https://www.redhat.com/archives/libvir-list/2020-July/msg01500.html
Anyway, I'm resending here for future reference. Patches are merged so
no need to review.
We are still using libdevmapper after these in
src/storage/storage_backend_mpath.c and thus I'm not removing configure
check. But in time for the next release I will look into it.
Michal Prívozník (4):
virdevmapper.c: Join two WITH_DEVMAPPER sections together
virDevMapperGetTargetsImpl: Use VIR_AUTOSTRINGLIST
virdevmapper: Don't use libdevmapper to obtain dependencies
virDevMapperGetTargets: Don't ignore EBADF
po/POTFILES.in | 1 +
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/qemu_domain.c | 4 +-
src/util/virdevmapper.c | 337 +++++++++++++++++++++++++++-------------
4 files changed, 232 insertions(+), 112 deletions(-)
--
2.26.2
3:31 a.m.
New subject: [PATCH 1/4] virdevmapper.c: Join two WITH_DEVMAPPER sections together
There are two distinct WITH_DEVMAPPER sections in the file, for
different functions each. Rearrange the code to make some of
future commits smaller.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/util/virdevmapper.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index 40a82285f9..118dc82e94 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -195,19 +195,7 @@ virDevMapperGetTargets(const char *path,
return virDevMapperGetTargetsImpl(path, devPaths, ttl);
}
-#else /* ! WITH_DEVMAPPER */
-int
-virDevMapperGetTargets(const char *path G_GNUC_UNUSED,
- char ***devPaths G_GNUC_UNUSED)
-{
- errno = ENOSYS;
- return -1;
-}
-#endif /* ! WITH_DEVMAPPER */
-
-
-#if WITH_DEVMAPPER
bool
virIsDevMapperDevice(const char *dev_name)
{
@@ -223,6 +211,15 @@ virIsDevMapperDevice(const char *dev_name)
#else /* ! WITH_DEVMAPPER */
+int
+virDevMapperGetTargets(const char *path G_GNUC_UNUSED,
+ char ***devPaths G_GNUC_UNUSED)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
+
bool
virIsDevMapperDevice(const char *dev_name G_GNUC_UNUSED)
{
--
2.26.2
3:31 a.m.
New subject: [PATCH 2/4] virDevMapperGetTargetsImpl: Use VIR_AUTOSTRINGLIST
Since we have VIR_AUTOSTRINGLIST we can use it to free string
lists used in the function automatically.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/util/virdevmapper.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index 118dc82e94..44c4731fb4 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -67,8 +67,7 @@ virDevMapperGetTargetsImpl(const char *path,
struct dm_task *dmt = NULL;
struct dm_deps *deps;
struct dm_info info;
- char **devPaths = NULL;
- char **recursiveDevPaths = NULL;
+ VIR_AUTOSTRINGLIST devPaths = NULL;
size_t i;
int ret = -1;
@@ -133,28 +132,19 @@ virDevMapperGetTargetsImpl(const char *path,
minor(deps->device[i]));
}
- recursiveDevPaths = NULL;
for (i = 0; i < deps->count; i++) {
- char **tmpPaths;
+ VIR_AUTOSTRINGLIST tmpPaths = NULL;
if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0)
goto cleanup;
- if (tmpPaths &&
- virStringListMerge(&recursiveDevPaths, &tmpPaths) < 0) {
- virStringListFree(tmpPaths);
+ if (virStringListMerge(&devPaths, &tmpPaths) < 0)
goto cleanup;
- }
}
- if (virStringListMerge(&devPaths, &recursiveDevPaths) < 0)
- goto cleanup;
-
*devPaths_ret = g_steal_pointer(&devPaths);
ret = 0;
cleanup:
- virStringListFree(recursiveDevPaths);
- virStringListFree(devPaths);
dm_task_destroy(dmt);
return ret;
}
--
2.26.2
3:31 a.m.
New subject: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
CVE-2020-14339
When building domain's private /dev in a namespace, libdevmapper
is consulted for getting full dependency tree of domain's disks.
The reason is that for a multipath devices all dependent devices
must be created in the namespace and allowed in CGroups.
However, this approach is very fragile as building of namespace
happens in the forked off child process, after mass close of FDs
and just before dropping privileges and execing QEMU. And it so
happens that when calling libdevmapper APIs, one of them opens
/dev/mapper/control and saves the FD into a global variable. The
FD is kept open until the lib is unlinked or dm_lib_release() is
called explicitly. We are doing neither.
However, the virDevMapperGetTargets() function is called also
from libvirtd (when setting up CGroups) and thus has to be thread
safe. Unfortunately, libdevmapper APIs are not thread safe (nor
async signal safe) and thus we can't use them. Reimplement what
libdevmapper would do using plain C (ioctl()-s, /proc/devices
parsing, /dev/mapper dirwalking, and so on).
Fixes: a30078cb832646177defd256e77c632905f1e6d0
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
po/POTFILES.in | 1 +
src/util/virdevmapper.c | 304 ++++++++++++++++++++++++++++------------
2 files changed, 219 insertions(+), 86 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index b10008ae3d..0498a0e819 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -240,6 +240,7 @@
@SRCDIR(a)src/util/vircrypto.c
@SRCDIR(a)src/util/virdaemon.c
@SRCDIR(a)src/util/virdbus.c
+@SRCDIR(a)src/util/virdevmapper.c
@SRCDIR(a)src/util/virdnsmasq.c
@SRCDIR(a)src/util/virerror.c
@SRCDIR(a)src/util/virerror.h
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index 44c4731fb4..a471504176 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -20,38 +20,67 @@
#include <config.h>
-#ifdef __linux__
-# include <sys/sysmacros.h>
-#endif
-
-#ifdef WITH_DEVMAPPER
-# include <libdevmapper.h>
-#endif
-
#include "virdevmapper.h"
#include "internal.h"
-#include "virthread.h"
-#include "viralloc.h"
-#include "virstring.h"
-
-#ifdef WITH_DEVMAPPER
-static void
-virDevMapperDummyLogger(int level G_GNUC_UNUSED,
- const char *file G_GNUC_UNUSED,
- int line G_GNUC_UNUSED,
- int dm_errno G_GNUC_UNUSED,
- const char *fmt G_GNUC_UNUSED,
- ...)
-{
- return;
-}
+
+#ifdef __linux__
+# include <sys/sysmacros.h>
+# include <linux/dm-ioctl.h>
+# include <sys/ioctl.h>
+# include <sys/types.h>
+# include <sys/stat.h>
+# include <fcntl.h>
+
+# include "virthread.h"
+# include "viralloc.h"
+# include "virstring.h"
+# include "virfile.h"
+
+# define VIR_FROM_THIS VIR_FROM_STORAGE
+
+# define PROC_DEVICES "/proc/devices"
+# define DM_NAME "device-mapper"
+# define DEV_DM_DIR "/dev/" DM_DIR
+# define CONTROL_PATH DEV_DM_DIR "/" DM_CONTROL_NODE
+# define BUF_SIZE (16 * 1024)
+
+G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl));
+
+static unsigned int virDMMajor;
+
static int
virDevMapperOnceInit(void)
{
- /* Ideally, we would not need this. But libdevmapper prints
- * error messages to stderr by default. Sad but true. */
- dm_log_with_errno_init(virDevMapperDummyLogger);
+ g_autofree char *buf = NULL;
+ VIR_AUTOSTRINGLIST lines = NULL;
+ size_t i;
+
+ if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0)
+ return -1;
+
+ lines = virStringSplit(buf, "\n", 0);
+ if (!lines)
+ return -1;
+
+ for (i = 0; lines[i]; i++) {
+ g_autofree char *dev = NULL;
+ unsigned int maj;
+
+ if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
+ STREQ(dev, DM_NAME)) {
+ virDMMajor = maj;
+ break;
+ }
+ }
+
+ if (!lines[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find major for %s"),
+ DM_NAME);
+ return -1;
+ }
+
return 0;
}
@@ -59,94 +88,190 @@ virDevMapperOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virDevMapper);
+static void *
+virDMIoctl(int controlFD, int cmd, struct dm_ioctl *dm, char **buf)
+{
+ size_t bufsize = BUF_SIZE;
+
+ reread:
+ *buf = g_new0(char, bufsize);
+
+ dm->version[0] = DM_VERSION_MAJOR;
+ dm->version[1] = 0;
+ dm->version[2] = 0;
+ dm->data_size = bufsize;
+ dm->data_start = sizeof(struct dm_ioctl);
+
+ memcpy(*buf, dm, sizeof(struct dm_ioctl));
+
+ if (ioctl(controlFD, cmd, *buf) < 0) {
+ VIR_FREE(*buf);
+ return NULL;
+ }
+
+ memcpy(dm, *buf, sizeof(struct dm_ioctl));
+
+ if (dm->flags & DM_BUFFER_FULL_FLAG) {
+ bufsize += BUF_SIZE;
+ VIR_FREE(*buf);
+ goto reread;
+ }
+
+ return *buf + dm->data_start;
+}
+
+
static int
-virDevMapperGetTargetsImpl(const char *path,
+virDMOpen(void)
+{
+ VIR_AUTOCLOSE controlFD = -1;
+ struct dm_ioctl dm;
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ memset(&dm, 0, sizeof(dm));
+
+ if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0)
+ return -1;
+
+ if (!virDMIoctl(controlFD, DM_VERSION, &dm, &tmp)) {
+ virReportSystemError(errno, "%s",
+ _("Unable to get device-mapper version"));
+ return -1;
+ }
+
+ if (dm.version[0] != DM_VERSION_MAJOR) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("Unsupported device-mapper version. Expected %d got
%d"),
+ DM_VERSION_MAJOR, dm.version[0]);
+ return -1;
+ }
+
+ ret = controlFD;
+ controlFD = -1;
+ return ret;
+}
+
+
+static char *
+virDMSanitizepath(const char *path)
+{
+ g_autofree char *dmDirPath = NULL;
+ struct dirent *ent = NULL;
+ struct stat sb[2];
+ DIR *dh = NULL;
+ const char *p;
+ char *ret = NULL;
+ int rc;
+
+ /* If a path is NOT provided then assume it's DM name */
+ p = strrchr(path, '/');
+
+ if (!p)
+ return g_strdup(path);
+ else
+ p++;
+
+ /* It's a path. Check if the last component is DM name */
+ if (stat(path, &sb[0]) < 0) {
+ virReportError(errno,
+ _("Unable to stat %p"),
+ path);
+ return NULL;
+ }
+
+ dmDirPath = g_strdup_printf(DEV_DM_DIR "/%s", p);
+
+ if (stat(dmDirPath, &sb[1]) == 0 &&
+ sb[0].st_rdev == sb[1].st_rdev) {
+ return g_strdup(p);
+ }
+
+ /* The last component of @path wasn't DM name. Let's check if
+ * there's a device under /dev/mapper/ with the same rdev. */
+ if (virDirOpen(&dh, DEV_DM_DIR) < 0)
+ return NULL;
+
+ while ((rc = virDirRead(dh, &ent, DEV_DM_DIR)) > 0) {
+ g_autofree char *tmp = g_strdup_printf(DEV_DM_DIR "/%s",
ent->d_name);
+
+ if (stat(tmp, &sb[1]) == 0 &&
+ sb[0].st_rdev == sb[0].st_rdev) {
+ ret = g_steal_pointer(&tmp);
+ break;
+ }
+ }
+
+ virDirClose(&dh);
+ return ret;
+}
+
+
+static int
+virDevMapperGetTargetsImpl(int controlFD,
+ const char *path,
char ***devPaths_ret,
unsigned int ttl)
{
- struct dm_task *dmt = NULL;
- struct dm_deps *deps;
- struct dm_info info;
+ g_autofree char *sanitizedPath = NULL;
+ g_autofree char *buf = NULL;
+ struct dm_ioctl dm;
+ struct dm_target_deps *deps = NULL;
VIR_AUTOSTRINGLIST devPaths = NULL;
size_t i;
- int ret = -1;
+ memset(&dm, 0, sizeof(dm));
*devPaths_ret = NULL;
- if (virDevMapperInitialize() < 0)
- return ret;
-
if (ttl == 0) {
errno = ELOOP;
- return ret;
+ return -1;
}
if (!virIsDevMapperDevice(path))
return 0;
- if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) {
- if (errno == ENOENT || errno == ENODEV) {
- /* It's okay. Kernel is probably built without
- * devmapper support. */
- ret = 0;
- }
- return ret;
- }
+ if (!(sanitizedPath = virDMSanitizepath(path)))
+ return 0;
- if (!dm_task_set_name(dmt, path)) {
- if (errno == ENOENT) {
- /* It's okay, @path is not managed by devmapper =>
- * not a devmapper device. */
- ret = 0;
- }
- goto cleanup;
+ if (virStrncpy(dm.name, sanitizedPath, -1, DM_TABLE_DEPS) < 0) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Resolved device mapper name too long"));
+ return -1;
}
- dm_task_no_open_count(dmt);
+ deps = virDMIoctl(controlFD, DM_TABLE_DEPS, &dm, &buf);
+ if (!deps) {
+ if (errno == ENXIO)
+ return 0;
- if (!dm_task_run(dmt)) {
- if (errno == ENXIO) {
- /* If @path = "/dev/mapper/control" ENXIO is returned. */
- ret = 0;
- }
- goto cleanup;
+ virReportSystemError(errno,
+ _("Unable to query dependencies for %s"),
+ path);
+ return -1;
}
- if (!dm_task_get_info(dmt, &info))
- goto cleanup;
-
- if (!info.exists) {
- ret = 0;
- goto cleanup;
- }
-
- if (!(deps = dm_task_get_deps(dmt)))
- goto cleanup;
-
if (VIR_ALLOC_N_QUIET(devPaths, deps->count + 1) < 0)
- goto cleanup;
+ return -1;
for (i = 0; i < deps->count; i++) {
devPaths[i] = g_strdup_printf("/dev/block/%u:%u",
- major(deps->device[i]),
- minor(deps->device[i]));
+ major(deps->dev[i]),
+ minor(deps->dev[i]));
}
for (i = 0; i < deps->count; i++) {
VIR_AUTOSTRINGLIST tmpPaths = NULL;
- if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0)
- goto cleanup;
+ if (virDevMapperGetTargetsImpl(controlFD, devPaths[i], &tmpPaths, ttl - 1)
< 0)
+ return -1;
if (virStringListMerge(&devPaths, &tmpPaths) < 0)
- goto cleanup;
+ return -1;
}
*devPaths_ret = g_steal_pointer(&devPaths);
- ret = 0;
- cleanup:
- dm_task_destroy(dmt);
- return ret;
+ return 0;
}
@@ -165,9 +290,6 @@ virDevMapperGetTargetsImpl(const char *path,
* If @path consists of yet another devmapper targets these are
* consulted recursively.
*
- * If we don't have permissions to talk to kernel, -1 is returned
- * and errno is set to EBADF.
- *
* Returns 0 on success,
* -1 otherwise (with errno set, no libvirt error is
* reported)
@@ -176,13 +298,20 @@ int
virDevMapperGetTargets(const char *path,
char ***devPaths)
{
+ VIR_AUTOCLOSE controlFD = -1;
const unsigned int ttl = 32;
/* Arbitrary limit on recursion level. A devmapper target can
* consist of devices or yet another targets. If that's the
* case, we have to stop recursion somewhere. */
- return virDevMapperGetTargetsImpl(path, devPaths, ttl);
+ if (virDevMapperInitialize() < 0)
+ return -1;
+
+ if ((controlFD = virDMOpen()) < 0)
+ return -1;
+
+ return virDevMapperGetTargetsImpl(controlFD, path, devPaths, ttl);
}
@@ -191,15 +320,18 @@ virIsDevMapperDevice(const char *dev_name)
{
struct stat buf;
+ if (virDevMapperInitialize() < 0)
+ return false;
+
if (!stat(dev_name, &buf) &&
S_ISBLK(buf.st_mode) &&
- dm_is_dm_major(major(buf.st_rdev)))
- return true;
+ major(buf.st_rdev) == virDMMajor)
+ return true;
return false;
}
-#else /* ! WITH_DEVMAPPER */
+#else /* !defined(__linux__) */
int
virDevMapperGetTargets(const char *path G_GNUC_UNUSED,
@@ -215,4 +347,4 @@ virIsDevMapperDevice(const char *dev_name G_GNUC_UNUSED)
{
return false;
}
-#endif /* ! WITH_DEVMAPPER */
+#endif /* ! defined(__linux__) */
--
2.26.2
4:39 p.m.
New subject: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Mon, 2020-07-27 at 10:31 +0200, Michal Privoznik wrote:
CVE-2020-14339
When building domain's private /dev in a namespace, libdevmapper
is consulted for getting full dependency tree of domain's disks.
The reason is that for a multipath devices all dependent devices
must be created in the namespace and allowed in CGroups.
However, this approach is very fragile as building of namespace
happens in the forked off child process, after mass close of FDs
and just before dropping privileges and execing QEMU. And it so
happens that when calling libdevmapper APIs, one of them opens
/dev/mapper/control and saves the FD into a global variable. The
FD is kept open until the lib is unlinked or dm_lib_release() is
called explicitly. We are doing neither.
However, the virDevMapperGetTargets() function is called also
from libvirtd (when setting up CGroups) and thus has to be thread
safe. Unfortunately, libdevmapper APIs are not thread safe (nor
async signal safe) and thus we can't use them. Reimplement what
libdevmapper would do using plain C (ioctl()-s, /proc/devices
parsing, /dev/mapper dirwalking, and so on).
Fixes: a30078cb832646177defd256e77c632905f1e6d0
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
po/POTFILES.in | 1 +
src/util/virdevmapper.c | 304 ++++++++++++++++++++++++++++------------
2 files changed, 219 insertions(+), 86 deletions(-)
This patch broke libvirt in Debian for certain setups.
With AppArmor enabled (the default), the error is
$ virsh start cirros
error: Failed to start domain cirros
error: internal error: Process exited prior to exec: libvirt:
error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
Device or resource busy
If I disable AppArmor by passing security='' on the kernel command
line, the error message changes to
$ virsh start cirros
error: Failed to start domain cirros
error: internal error: Process exited prior to exec: libvirt:
QEMU Driver error : Unable to get devmapper targets for
/var/lib/libvirt/images/cirros.qcow2: Success
An effective workaround is to set namespaces=[] in qemu.conf, but
that's of course not something that we want users doing :)
The underlying issue seems to be caused by the fact that, on a Debian
installation that uses plain partitions instead of LVM, /proc/devices
doesn't contain an entry for device-mapper right after boot, which...
static int
virDevMapperOnceInit(void)
{
- /* Ideally, we would not need this. But libdevmapper prints
- * error messages to stderr by default. Sad but true. */
- dm_log_with_errno_init(virDevMapperDummyLogger);
+ g_autofree char *buf = NULL;
+ VIR_AUTOSTRINGLIST lines = NULL;
+ size_t i;
+
+ if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0)
+ return -1;
+
+ lines = virStringSplit(buf, "\n", 0);
+ if (!lines)
+ return -1;
+
+ for (i = 0; lines[i]; i++) {
+ g_autofree char *dev = NULL;
+ unsigned int maj;
+
+ if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
+ STREQ(dev, DM_NAME)) {
+ virDMMajor = maj;
+ break;
+ }
+ }
+
+ if (!lines[i]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find major for %s"),
+ DM_NAME);
+ return -1;
+ }
... this code expects.
Running
$ sudo dmsetup info
No devices found
causes the entry to appear, and from that moment on guest startup
will work as expected regardless of whether or not AppArmor is
enabled.
I hope the information above can help someone who's familiar with the
code figure out a fix. I'll provide more if needed, just ask! I can
also provide prebuilt .deb files for 6.6.0 that consistently trigger
the issue when added to a bog standard Debian testing installation.
--
Andrea Bolognani / Red Hat / Virtualization
8:45 a.m.
New subject: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani <abologna(a)redhat.com>
wrote:
On Mon, 2020-07-27 at 10:31 +0200, Michal Privoznik wrote:
> CVE-2020-14339
>
> When building domain's private /dev in a namespace, libdevmapper
> is consulted for getting full dependency tree of domain's disks.
> The reason is that for a multipath devices all dependent devices
> must be created in the namespace and allowed in CGroups.
>
> However, this approach is very fragile as building of namespace
> happens in the forked off child process, after mass close of FDs
> and just before dropping privileges and execing QEMU. And it so
> happens that when calling libdevmapper APIs, one of them opens
> /dev/mapper/control and saves the FD into a global variable. The
> FD is kept open until the lib is unlinked or dm_lib_release() is
> called explicitly. We are doing neither.
>
> However, the virDevMapperGetTargets() function is called also
> from libvirtd (when setting up CGroups) and thus has to be thread
> safe. Unfortunately, libdevmapper APIs are not thread safe (nor
> async signal safe) and thus we can't use them. Reimplement what
> libdevmapper would do using plain C (ioctl()-s, /proc/devices
> parsing, /dev/mapper dirwalking, and so on).
>
> Fixes: a30078cb832646177defd256e77c632905f1e6d0
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
> ---
> po/POTFILES.in | 1 +
> src/util/virdevmapper.c | 304 ++++++++++++++++++++++++++++------------
> 2 files changed, 219 insertions(+), 86 deletions(-)
This patch broke libvirt in Debian for certain setups.
With AppArmor enabled (the default), the error is
$ virsh start cirros
error: Failed to start domain cirros
error: internal error: Process exited prior to exec: libvirt:
error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
Device or resource busy
If I disable AppArmor by passing security='' on the kernel command
line, the error message changes to
$ virsh start cirros
error: Failed to start domain cirros
error: internal error: Process exited prior to exec: libvirt:
QEMU Driver error : Unable to get devmapper targets for
/var/lib/libvirt/images/cirros.qcow2: Success
An effective workaround is to set namespaces=[] in qemu.conf, but
that's of course not something that we want users doing :)
The underlying issue seems to be caused by the fact that, on a Debian
installation that uses plain partitions instead of LVM, /proc/devices
doesn't contain an entry for device-mapper right after boot, which...
> static int
> virDevMapperOnceInit(void)
> {
> - /* Ideally, we would not need this. But libdevmapper prints
> - * error messages to stderr by default. Sad but true. */
> - dm_log_with_errno_init(virDevMapperDummyLogger);
> + g_autofree char *buf = NULL;
> + VIR_AUTOSTRINGLIST lines = NULL;
> + size_t i;
> +
> + if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0)
> + return -1;
> +
> + lines = virStringSplit(buf, "\n", 0);
> + if (!lines)
> + return -1;
> +
> + for (i = 0; lines[i]; i++) {
> + g_autofree char *dev = NULL;
> + unsigned int maj;
> +
> + if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2
&&
> + STREQ(dev, DM_NAME)) {
> + virDMMajor = maj;
> + break;
> + }
> + }
> +
> + if (!lines[i]) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find major for %s"),
> + DM_NAME);
> + return -1;
> + }
... this code expects.
Running
$ sudo dmsetup info
No devices found
We see the same problem as mentioned by Andrea. The host kernel
configuration used:
…
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=m
…
As soon as we load the kernel module ‘dm-mod‘ everything works because
then ‘device-mapper‘ is listed in /proc/devices.
causes the entry to appear, and from that moment on guest startup
will work as expected regardless of whether or not AppArmor is
enabled.
I hope the information above can help someone who's familiar with the
code figure out a fix. I'll provide more if needed, just ask! I can
also provide prebuilt .deb files for 6.6.0 that consistently trigger
the issue when added to a bog standard Debian testing installation.
--
Andrea Bolognani / Red Hat / Virtualization
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
9:17 a.m.
New subject: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Thu, 2020-08-06 at 15:45 +0200, Marc Hartmayer wrote:
On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani
<abologna(a)redhat.com> wrote:
> This patch broke libvirt in Debian for certain setups.
>
> With AppArmor enabled (the default), the error is
>
> $ virsh start cirros
> error: Failed to start domain cirros
> error: internal error: Process exited prior to exec: libvirt:
> error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
> Device or resource busy
>
> If I disable AppArmor by passing security='' on the kernel command
> line, the error message changes to
>
> $ virsh start cirros
> error: Failed to start domain cirros
> error: internal error: Process exited prior to exec: libvirt:
> QEMU Driver error : Unable to get devmapper targets for
> /var/lib/libvirt/images/cirros.qcow2: Success
>
> An effective workaround is to set namespaces=[] in qemu.conf, but
> that's of course not something that we want users doing :)
>
> The underlying issue seems to be caused by the fact that, on a Debian
> installation that uses plain partitions instead of LVM, /proc/devices
> doesn't contain an entry for device-mapper right after boot, which...
> ... this code expects.
>
> Running
>
> $ sudo dmsetup info
> No devices found
We see the same problem as mentioned by Andrea. The host kernel
configuration used:
…
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=m
…
As soon as we load the kernel module ‘dm-mod‘ everything works because
then ‘device-mapper‘ is listed in /proc/devices.
Thanks Marc! I have confirmed that Debian also uses the same kernel
configuration as the one you have reported above, and that running
dmsetup(8) causes the dm-mod kernel module to be loaded.
For comparison Fedora, where everything works fine, uses
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=y
--
Andrea Bolognani / Red Hat / Virtualization
5:05 a.m.
New subject: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Thu, Aug 6, 2020 at 4:21 PM Andrea Bolognani <abologna(a)redhat.com> wrote:
On Thu, 2020-08-06 at 15:45 +0200, Marc Hartmayer wrote:
> On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani <
abologna(a)redhat.com> wrote:
> > This patch broke libvirt in Debian for certain setups.
> >
> > With AppArmor enabled (the default), the error is
> >
> > $ virsh start cirros
> > error: Failed to start domain cirros
> > error: internal error: Process exited prior to exec: libvirt:
> > error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
> > Device or resource busy
> >
> > If I disable AppArmor by passing security='' on the kernel command
> > line, the error message changes to
> >
> > $ virsh start cirros
> > error: Failed to start domain cirros
> > error: internal error: Process exited prior to exec: libvirt:
> > QEMU Driver error : Unable to get devmapper targets for
> > /var/lib/libvirt/images/cirros.qcow2: Success
> >
> > An effective workaround is to set namespaces=[] in qemu.conf, but
> > that's of course not something that we want users doing :)
> >
> > The underlying issue seems to be caused by the fact that, on a Debian
> > installation that uses plain partitions instead of LVM, /proc/devices
> > doesn't contain an entry for device-mapper right after boot, which...
> > ... this code expects.
> >
> > Running
> >
> > $ sudo dmsetup info
> > No devices found
>
> We see the same problem as mentioned by Andrea. The host kernel
> configuration used:
>
> …
> CONFIG_BLK_DEV_DM_BUILTIN=y
> CONFIG_BLK_DEV_DM=m
> …
>
> As soon as we load the kernel module ‘dm-mod‘ everything works because
> then ‘device-mapper‘ is listed in /proc/devices.
Thanks Marc! I have confirmed that Debian also uses the same kernel
configuration as the one you have reported above, and that running
dmsetup(8) causes the dm-mod kernel module to be loaded.
For comparison Fedora, where everything works fine, uses
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=y
FYI even having the module loaded isn't the ultimate workaround as
there is a further twist of the same issue.
Ubuntu has:
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=y
And there is an entry in devices (in host and in the container)
$ cat /proc/devices | grep map
253 device-mapper
But libvirt 6.6 in this case running in a LXD system container
(working before) now fails related to this with what seems to be the
same high level symptom.
# virsh start kvmguest-groovy-normal3
error: Failed to start domain kvmguest-groovy-normal3
error: internal error: Process exited prior to exec: libvirt: QEMU
Driver error : Unable to get devmapper targets for
/var/lib/uvtool/libvirt/images/kvmguest-groovy-normal3.qcow: No such
file or directory
--
Andrea Bolognani / Red Hat / Virtualization
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
3:31 a.m.
New subject: [PATCH 4/4] virDevMapperGetTargets: Don't ignore EBADF
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/qemu_domain.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 914bf640ca..e88da02341 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -87,7 +87,7 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
}
if (virDevMapperGetTargets(path, &targetPaths) < 0 &&
- errno != ENOSYS && errno != EBADF) {
+ errno != ENOSYS) {
virReportSystemError(errno,
_("Unable to get devmapper targets for %s"),
path);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5b22eb2eaa..2058290870 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10264,7 +10264,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
return -1;
if (virDevMapperGetTargets(next->path, &targetPaths) < 0
&&
- errno != ENOSYS && errno != EBADF) {
+ errno != ENOSYS) {
virReportSystemError(errno,
_("Unable to get devmapper targets for
%s"),
next->path);
@@ -11328,7 +11328,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
tmpPath = g_strdup(next->path);
if (virDevMapperGetTargets(next->path, &targetPaths) < 0
&&
- errno != ENOSYS && errno != EBADF) {
+ errno != ENOSYS) {
virReportSystemError(errno,
_("Unable to get devmapper targets for
%s"),
next->path);
--
2.26.2
1564
days inactive
1578
days old
8 comments
4 participants
participants (4)
-
Andrea Bolognani -
Christian Ehrhardt -
Marc Hartmayer -
Michal Privoznik