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