In 6.7.0 release I've changed how domain namespace is built and
populated. Previously it used to be done from a pre-exec hook
(ran in the forked off child, just before dropping all privileges
and exec()-ing QEMU), which not only meant we had to have two
different code paths for creating a node in domain's namespace
(one for this pre-exec hook, the other for hotplug ran from the
daemon), it also proved problematic because it was leaking FDs
into QEMU process. To mitigate this problem, we've not only
ditched libdevmapper from the NS population process, I've also
dropped the pre-exec code and let the NS be populated from the
daemon (using the hotplug code). But, I was not careful when
doing so, because the pre-exec code was tolerant to files that
doesn't exist, while this new code isn't. For instance, the very
first thing that is done when the new NS is created is it's
populated with @defaultDeviceACL which contain files like
/dev/null, /dev/zero, /dev/random and /dev/kvm (and others).
While the rest will probably exist every time, /dev/kvm might not
and thus the new code I wrote has to be tolerant to that.

Of course, users can override the @defaultDeviceACL (by setting
cgroup_device_acl in qemu.conf) and remove /dev/kvm (which is
acceptable workaround), but we definitely want libvirt to work
out of the box even on hosts without KVM.

Fixes: 9048dc4e627ddf33996084167bece7b5fb83b0bc
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_namespace.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 454d6a7b4d..87f4fd8d58 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1094,6 +1094,9 @@ qemuNamespaceMknodItemInit(qemuNamespaceMknodItemPtr item,
     item->file = file;
 
     if (g_lstat(file, &item->sb) < 0) {
+        if (errno == ENOENT)
+            return -2;
+
         virReportSystemError(errno,
                              _("Unable to access %s"), file);
         return -1;
@@ -1168,9 +1171,16 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data,
 
     while (1) {
         qemuNamespaceMknodItem item = { 0 };
+        int rc;
 
-        if (qemuNamespaceMknodItemInit(&item, cfg, vm, next) < 0)
+        rc = qemuNamespaceMknodItemInit(&item, cfg, vm, next);
+        if (rc == -2) {
+            /* @file doesn't exist. We can break here. */
+            break;
+        } else if (rc < 0) {
+            /* Some other (critical) error. */
             return -1;
+        }
 
         if (STRPREFIX(next, QEMU_DEVPREFIX)) {
             for (i = 0; i < ndevMountsPath; i++) {
-- 
2.26.2

On Thu, Sep 03, 2020 at 06:22:00PM +0200, Michal Privoznik wrote:
> In 6.7.0 release I've changed how domain namespace is built and
> populated. Previously it used to be done from a pre-exec hook
> (ran in the forked off child, just before dropping all privileges
> and exec()-ing QEMU), which not only meant we had to have two
> different code paths for creating a node in domain's namespace
> (one for this pre-exec hook, the other for hotplug ran from the
> daemon), it also proved problematic because it was leaking FDs
> into QEMU process. To mitigate this problem, we've not only
> ditched libdevmapper from the NS population process, I've also
> dropped the pre-exec code and let the NS be populated from the
> daemon (using the hotplug code). But, I was not careful when
> doing so, because the pre-exec code was tolerant to files that
> doesn't exist, while this new code isn't. For instance, the very
> first thing that is done when the new NS is created is it's
> populated with @defaultDeviceACL which contain files like
> /dev/null, /dev/zero, /dev/random and /dev/kvm (and others).
> While the rest will probably exist every time, /dev/kvm might not
> and thus the new code I wrote has to be tolerant to that.
> 
> Of course, users can override the @defaultDeviceACL (by setting
> cgroup_device_acl in qemu.conf) and remove /dev/kvm (which is
> acceptable workaround), but we definitely want libvirt to work
> out of the box even on hosts without KVM.
> 
> Fixes: 9048dc4e627ddf33996084167bece7b5fb83b0bc
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_namespace.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On a Thursday in 2020, Michal Privoznik wrote:
>In 6.7.0 release I've changed how domain namespace is built and
>populated. Previously it used to be done from a pre-exec hook
>(ran in the forked off child, just before dropping all privileges
>and exec()-ing QEMU), which not only meant we had to have two
>different code paths for creating a node in domain's namespace
>(one for this pre-exec hook, the other for hotplug ran from the
>daemon), it also proved problematic because it was leaking FDs
>into QEMU process. To mitigate this problem, we've not only
>ditched libdevmapper from the NS population process, I've also
>dropped the pre-exec code and let the NS be populated from the
>daemon (using the hotplug code). But, I was not careful when
>doing so, because the pre-exec code was tolerant to files that
>doesn't exist, while this new code isn't. For instance, the very
>first thing that is done when the new NS is created is it's
>populated with @defaultDeviceACL which contain files like
>/dev/null, /dev/zero, /dev/random and /dev/kvm (and others).
>While the rest will probably exist every time, /dev/kvm might not
>and thus the new code I wrote has to be tolerant to that.

Please put a newline somewhere in this huge block of text.

>
>Of course, users can override the @defaultDeviceACL (by setting
>cgroup_device_acl in qemu.conf) and remove /dev/kvm (which is
>acceptable workaround), but we definitely want libvirt to work
>out of the box even on hosts without KVM.
>
>Fixes: 9048dc4e627ddf33996084167bece7b5fb83b0bc
>Reported-by: Daniel P. Berrangé <berrange@redhat.com>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_namespace.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano