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

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(a)redhat.com&gt; > --- > 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.