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

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(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. Except that checking return value of virBitmapSetBit is also required.