[libvirt] [PATCH v2 08/21] qemu: Spawn qemu under mount namespace

[Daniel P. Berrange]

On Mon, Dec 12, 2016 at 10:55:55AM +0000, Daniel P. Berrange wrote:
> On Wed, Dec 07, 2016 at 09:36:15AM +0100, Michal Privoznik wrote:
> > Prime time. When it comes to spawning qemu process and
> > relabelling all the devices it's going to touch, there's inherent
> > race with other applications in the system (e.g. udev). Instead
> > of trying convincing udev to not touch libvirt managed devices,
> > we can create a separate mount namespace for the qemu, and mount
> > our own /dev there. Of course this puts more work onto us as we
> > have to maintain /dev files on each domain start and device
> > hot(un-)plug. On the other hand, this enhances security also.
> > 
> > >From technical POV, on domain startup process the parent
> > (libvirtd) creates:
> > 
> >   /var/lib/libvirt/qemu/$domain.dev
> >   /var/lib/libvirt/qemu/$domain.devpts
> > 
> > The child (which is going to be qemu eventually) calls unshare()
> > to create new mount namespace. From now on anything that child
> > does is invisible to the parent. Child then mounts tmpfs on
> > $domain.dev (so that it still sees original /dev from the host)
> > and creates some devices (as explained in one of the previous
> > patches). The devices have to be created exactly as they are in
> > the host (including perms, seclabels, ACLs, ...). After that it
> > moves $domain.dev mount to /dev.
> > 
> > What's the $domain.devpts mount there for then you ask? QEMU can
> > create PTYs for some chardevs. And historically we exposed the
> > host ends in our domain XML allowing users to connect to them.
> > Therefore we must preserve devpts mount to be shared with the
> > host's one.
> > 
> > To make this patch as small as possible, creating of devices
> > configured for domain in question is implemented in next patches.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > ---
> >  src/qemu/qemu_domain.c  | 377 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  src/qemu/qemu_domain.h  |  20 +++
> >  src/qemu/qemu_process.c |  13 ++
> >  3 files changed, 408 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 4aae14d9d..50b401f79 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -55,6 +55,9 @@
> >  
> >  #include <sys/time.h>
> >  #include <fcntl.h>
> > +#if defined(HAVE_SYS_MOUNT_H)
> > +# include <sys/mount.h>
> > +#endif
> >  
> >  #include <libxml/xpathInternals.h>
> >  
> > @@ -86,6 +89,10 @@ VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST,
> >                "start",
> >  );
> >  
> > +VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
> > +              "mount",
> > +);
> > +
> >  
> >  struct _qemuDomainLogContext {
> >      int refs;
> > @@ -146,6 +153,31 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
> >  }
> >  
> >  
> > +bool
> > +qemuDomainNamespaceEnabled(virDomainObjPtr vm,
> > +                           qemuDomainNamespace ns)
> > +{
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +
> > +    return priv->namespaces &&
> > +        virBitmapIsBitSet(priv->namespaces, ns);
> > +}
> > +
> > +
> > +static bool
> > +qemuDomainEnableNamespace(virDomainObjPtr vm,
> > +                          qemuDomainNamespace ns)
> > +{
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +
> > +    if (!priv->namespaces &&
> > +        !(priv->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
> > +        return -1;
> > +
> > +    return virBitmapSetBit(priv->namespaces, ns);
> 
> 
> Returning -1 from a method returning 'bool' is not what you want :-)
> Also causes a compile warning in the caller about comparing
> bool with '< 0'. Looking at the callers, I think you probably
> wanted
> 
> @@ -169,7 +169,7 @@ qemuDomainNamespaceEnabled(virDomainObjPtr vm,
>  }
>  
>  
> -static bool
> +static int
>  qemuDomainEnableNamespace(virDomainObjPtr vm,
>                            qemuDomainNamespace ns)
>  {
> @@ -179,7 +179,9 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
>          !(priv->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
>          return -1;
>  
> -    return virBitmapSetBit(priv->namespaces, ns);
> +    virBitmapSetBit(priv->namespaces, ns);
> +
> +    return 0;
>  }
>  
> 
> ACK with such a change made.

Except that checking return value of virBitmapSetBit is also required.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|