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