Re: [PATCH v1 20/34] qemuDomainBuildNamespace: Populate chardevs from daemon's namespace

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2020, Michal Privoznik wrote:
> As mentioned in one of previous commits, populating domain's
> namespace from pre-exec() hook is dangerous. This commit moves
> population of the namespace with domain chardevs into daemon's
> namespace.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain_namespace.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/src/qemu/qemu_domain_namespace.c b/src/qemu/qemu_domain_namespace.c
> index bafb08fac8..36d22b42f2 100644
> --- a/src/qemu/qemu_domain_namespace.c
> +++ b/src/qemu/qemu_domain_namespace.c
> @@ -629,7 +629,7 @@ qemuDomainSetupChardev(virDomainDefPtr def G_GNUC_UNUSED,
>                        virDomainChrDefPtr dev,
>                        void *opaque)
> {
> -    const struct qemuDomainCreateDeviceData *data = opaque;
> +    char ***paths = opaque;
>     const char *path = NULL;
>
>     if (!(path = virDomainChrSourceDefGetPath(dev->source)))
> @@ -640,20 +640,20 @@ qemuDomainSetupChardev(virDomainDefPtr def G_GNUC_UNUSED,
>         dev->source->data.nix.listen)
>         return 0;
>
> -    return qemuDomainCreateDevice(path, data, true);
> +    return virStringListAdd(paths, path);
> }
>
>
> static int
> qemuDomainSetupAllChardevs(virDomainObjPtr vm,
> -                           const struct qemuDomainCreateDeviceData *data)
> +                           char ***paths)
> {
>     VIR_DEBUG("Setting up chardevs");
>
>     if (virDomainChrDefForeach(vm->def,
>                                true,
>                                qemuDomainSetupChardev,
> -                               (void *)data) < 0)
> +                               paths) < 0)
>         return -1;
>
>     VIR_DEBUG("Setup all chardevs");
> @@ -877,6 +877,9 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>     if (qemuDomainSetupAllMemories(vm, &paths) < 0)
>         return -1;
>
> +    if (qemuDomainSetupAllChardevs(vm, &paths) < 0)
> +        return -1;
> +
>     if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths) < 0)
>         return -1;
>
> @@ -928,9 +931,6 @@ qemuDomainUnshareNamespace(virQEMUDriverConfigPtr cfg,
>     if (qemuDomainSetupDev(mgr, vm, devPath) < 0)
>         goto cleanup;
>
> -    if (qemuDomainSetupAllChardevs(vm, &data) < 0)
> -        goto cleanup;
> -
>     if (qemuDomainSetupAllTPMs(vm, &data) < 0)
>         goto cleanup;
>
> @@ -1779,20 +1779,15 @@ int
> qemuDomainNamespaceSetupChardev(virDomainObjPtr vm,
>                                 virDomainChrDefPtr chr)
> {
> -    const char *path;
> +    VIR_AUTOSTRINGLIST paths = NULL;
>
>     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>         return 0;
>
> -    if (!(path = virDomainChrSourceDefGetPath(chr->source)))
> -        return 0;
> +    if (qemuDomainSetupChardev(vm->def, chr, &paths) < 0)
> +        return -1;
>
> -    /* Socket created by qemu. It doesn't exist upfront. */
> -    if (chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
> -        chr->source->data.nix.listen)
> -        return 0;
> -

Hmm, this is not necessarily true. qemuBuildChrChardevStr opens listen
type sockets if QEMU supports FD passing for them.

> -    if (qemuDomainNamespaceMknodPath(vm, path) < 0)
> +    if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths) < 0)
>         return -1;

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature