[PATCH v1 16/34] qemuDomainBuildNamespace: Populate basic /dev from daemon's namespace

[Daniel P. Berrangé]

On Wed, Jul 22, 2020 at 11:40:10AM +0200, Michal Privoznik wrote:
> As mentioned in previous commit, populating domain's namespace
> from pre-exec() hook is dangerous. This commit moves population
> of the namespace with basic /dev nodes (e.g. /dev/null, /dev/kvm,
> etc.) into daemon's namespace.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain_namespace.c | 23 +++++++++++------------
>  src/qemu/qemu_domain_namespace.h |  3 ++-
>  src/qemu/qemu_process.c          |  2 +-
>  3 files changed, 14 insertions(+), 14 deletions(-)

I don't understand why, but this commit has broken QEMU startup on
hosts without KVM. It now always dies with

error : qemuNamespaceMknodItemInit:1341 : Unable to access /dev/kvm: No such file or directory


This was git bisect identified, but since theres no mention of kvm in
this patch, I'm going to assume the actual bug is hiding dormant in
a previous patch until this patch activates the bug.


> 
> diff --git a/src/qemu/qemu_domain_namespace.c b/src/qemu/qemu_domain_namespace.c
> index 38abed56c8..17c804dfca 100644
> --- a/src/qemu/qemu_domain_namespace.c
> +++ b/src/qemu/qemu_domain_namespace.c
> @@ -435,8 +435,7 @@ qemuDomainCreateDevice(const char *device,
>  
>  static int
>  qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,
> -                          virDomainObjPtr vm G_GNUC_UNUSED,
> -                          const struct qemuDomainCreateDeviceData *data)
> +                          char ***paths)
>  {
>      const char *const *devices = (const char *const *) cfg->cgroupDeviceACL;
>      size_t i;
> @@ -445,7 +444,7 @@ qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,
>          devices = defaultDeviceACL;
>  
>      for (i = 0; devices[i]; i++) {
> -        if (qemuDomainCreateDevice(devices[i], data, true) < 0)
> +        if (virStringListAdd(paths, devices[i]) < 0)
>              return -1;
>      }
>  
> @@ -454,10 +453,9 @@ qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,
>  
>  
>  static int
> -qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,
> -                   virSecurityManagerPtr mgr,
> +qemuDomainSetupDev(virSecurityManagerPtr mgr,
>                     virDomainObjPtr vm,
> -                   const struct qemuDomainCreateDeviceData *data)
> +                   const char *path)
>  {
>      g_autofree char *mount_options = NULL;
>      g_autofree char *opts = NULL;
> @@ -475,10 +473,7 @@ qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,
>       */
>      opts = g_strdup_printf("mode=755,size=65536%s", mount_options);
>  
> -    if (virFileSetupDev(data->path, opts) < 0)
> -        return -1;
> -
> -    if (qemuDomainPopulateDevices(cfg, vm, data) < 0)
> +    if (virFileSetupDev(path, opts) < 0)
>          return -1;
>  
>      return 0;
> @@ -862,10 +857,14 @@ qemuDomainNamespaceMknodPaths(virDomainObjPtr vm,
>  
>  
>  int
> -qemuDomainBuildNamespace(virDomainObjPtr vm)
> +qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
> +                         virDomainObjPtr vm)
>  {
>      VIR_AUTOSTRINGLIST paths = NULL;
>  
> +    if (qemuDomainPopulateDevices(cfg, &paths) < 0)
> +        return -1;
> +
>      if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths) < 0)
>          return -1;
>  
> @@ -914,7 +913,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfigPtr cfg,
>      if (virProcessSetupPrivateMountNS() < 0)
>          goto cleanup;
>  
> -    if (qemuDomainSetupDev(cfg, mgr, vm, &data) < 0)
> +    if (qemuDomainSetupDev(mgr, vm, devPath) < 0)
>          goto cleanup;
>  
>      if (qemuDomainSetupAllDisks(vm, &data) < 0)
> diff --git a/src/qemu/qemu_domain_namespace.h b/src/qemu/qemu_domain_namespace.h
> index 70eebf4dc4..644f2adef3 100644
> --- a/src/qemu/qemu_domain_namespace.h
> +++ b/src/qemu/qemu_domain_namespace.h
> @@ -41,7 +41,8 @@ int qemuDomainUnshareNamespace(virQEMUDriverConfigPtr cfg,
>                                 virSecurityManagerPtr mgr,
>                                 virDomainObjPtr vm);
>  
> -int qemuDomainBuildNamespace(virDomainObjPtr vm);
> +int qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
> +                             virDomainObjPtr vm);
>  
>  void qemuDomainDestroyNamespace(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bee0fd031b..e2f32dc25a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6832,7 +6832,7 @@ qemuProcessLaunch(virConnectPtr conn,
>      }
>  
>      VIR_DEBUG("Building domain mount namespace (if required)");
> -    if (qemuDomainBuildNamespace(vm) < 0)
> +    if (qemuDomainBuildNamespace(cfg, vm) < 0)
>          goto cleanup;
>  
>      VIR_DEBUG("Setting up domain cgroup (if required)");
> -- 
> 2.26.2
> 

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 :|