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

[Michal Privoznik]

On 12.12.2016 12:47, Daniel P. Berrange wrote:
> 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

Btw: my compiler does not warn me. Even with -Wextra :-(

>>
>> @@ -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.

Okay, I'm sticking with:

-    return virBitmapSetBit(priv->namespaces, ns);
+    if (virBitmapSetBit(priv->namespaces, ns) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to enable namespace: %s"),
+                       qemuDomainNamespaceTypeToString(ns));
+        return -1;
+    }
+
+    return 0;

Which makes sense from another POV: if virBitmapNew() fails, it also
reports error, however virBitmapSetBit() does not. So upon failure
qemuDomainEnableNamespace() might and might not reported error.

Michal