[libvirt] [PATCH] qemu_conf: Check for namespaces availability more wisely

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/93903ba1d9e3419a4965979fb5a2216e2d91da05.1487150427.git.mprivozn@redhat.com
src/qemu/qemu_conf.c   | 20 ++++++++++++++++----
src/qemu/qemu_conf.h   |  3 ++-
src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++---------------
src/qemu/qemu_domain.h |  2 ++
src/qemu/qemu_driver.c |  2 +-
5 files changed, 49 insertions(+), 21 deletions(-)
[libvirt] [PATCH] qemu_conf: Check for namespaces availability more wisely
Posted by Michal Privoznik 7 years, 1 month ago
The bare fact that mnt namespace is available is not enough for
us to allow/enable qemu namespaces feature. There are other
requirements: we must copy all the ACL & SELinux labels otherwise
we might grant access that is administratively forbidden or vice
versa.
At the same time, the check for namespace prerequisites is moved
from domain startup time to qemu.conf parser as it doesn't make
much sense to allow users to start misconfigured libvirt just to
find out they can't start a single domain.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_conf.c   | 20 ++++++++++++++++----
 src/qemu/qemu_conf.h   |  3 ++-
 src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++---------------
 src/qemu/qemu_domain.h |  2 ++
 src/qemu/qemu_driver.c |  2 +-
 5 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0223a95d2..ad482d0ee 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -321,12 +321,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
     if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
         goto error;
 
-#if defined(__linux__)
     if (privileged &&
-        virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) == 0 &&
+        qemuDomainNamespaceAvailable(QEMU_DOMAIN_NS_MOUNT) &&
         virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0)
         goto error;
-#endif /* defined(__linux__) */
 
 #ifdef DEFAULT_LOADER_NVRAM
     if (virFirmwareParseList(DEFAULT_LOADER_NVRAM,
@@ -438,7 +436,8 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
 
 
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
-                                const char *filename)
+                                const char *filename,
+                                bool privileged)
 {
     virConfPtr conf = NULL;
     int ret = -1;
@@ -832,6 +831,19 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
                 goto cleanup;
             }
 
+            if (!privileged) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("cannot use namespaces in session mode"));
+                goto cleanup;
+            }
+
+            if (qemuDomainNamespaceAvailable(ns) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("%s namespace is not available"),
+                               namespaces[i]);
+                goto cleanup;
+            }
+
             if (virBitmapSetBit(cfg->namespaces, ns) < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Unable to enable namespace: %s"),
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 91904ed4f..e585f81af 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -294,7 +294,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def);
 virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
 
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
-                                const char *filename);
+                                const char *filename,
+                                bool privileged);
 
 virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver);
 bool virQEMUDriverIsPrivileged(virQEMUDriverPtr driver);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3adec5c14..c3dcea0c4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7643,21 +7643,8 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     int ret = -1;
 
-    if (!virBitmapIsBitSet(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT)) {
-        ret = 0;
-        goto cleanup;
-    }
-
-    if (!virQEMUDriverIsPrivileged(driver)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("cannot use namespaces in session mode"));
-        goto cleanup;
-    }
-
-    if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
-        goto cleanup;
-
-    if (qemuDomainEnableNamespace(vm, QEMU_DOMAIN_NS_MOUNT) < 0)
+    if (virBitmapIsBitSet(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) &&
+        qemuDomainEnableNamespace(vm, QEMU_DOMAIN_NS_MOUNT) < 0)
         goto cleanup;
 
     ret = 0;
@@ -7667,6 +7654,32 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
 }
 
 
+bool
+qemuDomainNamespaceAvailable(qemuDomainNamespace ns)
+{
+
+    switch (ns) {
+    case QEMU_DOMAIN_NS_MOUNT:
+#if !defined(__linux__)
+        /* Namespaces are Linux specific. */
+        return false;
+#endif
+#if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
+        /* We can't create the exact copy of paths if either of
+         * these is not available. */
+        return false;
+#endif
+        if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
+            return false;
+        break;
+    case QEMU_DOMAIN_NS_LAST:
+        break;
+    }
+
+    return true;
+}
+
+
 struct qemuDomainAttachDeviceMknodData {
     virQEMUDriverPtr driver;
     virDomainObjPtr vm;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5cfa3e114..524a6729c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -808,6 +808,8 @@ int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
 int qemuDomainCreateNamespace(virQEMUDriverPtr driver,
                               virDomainObjPtr vm);
 
+bool qemuDomainNamespaceAvailable(qemuDomainNamespace ns);
+
 int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
                                  virDomainObjPtr vm,
                                  virStorageSourcePtr src);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89bc833de..afbcded93 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -676,7 +676,7 @@ qemuStateInitialize(bool privileged,
     if (virAsprintf(&driverConf, "%s/qemu.conf", cfg->configBaseDir) < 0)
         goto error;
 
-    if (virQEMUDriverConfigLoadFile(cfg, driverConf) < 0)
+    if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0)
         goto error;
     VIR_FREE(driverConf);
 
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_conf: Check for namespaces availability more wisely
Posted by Daniel P. Berrange 7 years, 1 month ago
On Wed, Feb 15, 2017 at 10:20:27AM +0100, Michal Privoznik wrote:
> The bare fact that mnt namespace is available is not enough for
> us to allow/enable qemu namespaces feature. There are other
> requirements: we must copy all the ACL & SELinux labels otherwise
> we might grant access that is administratively forbidden or vice
> versa.
> At the same time, the check for namespace prerequisites is moved
> from domain startup time to qemu.conf parser as it doesn't make
> much sense to allow users to start misconfigured libvirt just to
> find out they can't start a single domain.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_conf.c   | 20 ++++++++++++++++----
>  src/qemu/qemu_conf.h   |  3 ++-
>  src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++---------------
>  src/qemu/qemu_domain.h |  2 ++
>  src/qemu/qemu_driver.c |  2 +-
>  5 files changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 0223a95d2..ad482d0ee 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -321,12 +321,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>      if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
>          goto error;
>  
> -#if defined(__linux__)
>      if (privileged &&
> -        virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) == 0 &&
> +        qemuDomainNamespaceAvailable(QEMU_DOMAIN_NS_MOUNT) &&
>          virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0)
>          goto error;
> -#endif /* defined(__linux__) */
>  
>  #ifdef DEFAULT_LOADER_NVRAM
>      if (virFirmwareParseList(DEFAULT_LOADER_NVRAM,
> @@ -438,7 +436,8 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
>  
>  
>  int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> -                                const char *filename)
> +                                const char *filename,
> +                                bool privileged)
>  {
>      virConfPtr conf = NULL;
>      int ret = -1;
> @@ -832,6 +831,19 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>                  goto cleanup;
>              }
>  
> +            if (!privileged) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("cannot use namespaces in session mode"));
> +                goto cleanup;
> +            }
> +
> +            if (qemuDomainNamespaceAvailable(ns) < 0) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("%s namespace is not available"),
> +                               namespaces[i]);
> +                goto cleanup;
> +            }
> +
>              if (virBitmapSetBit(cfg->namespaces, ns) < 0) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Unable to enable namespace: %s"),
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 91904ed4f..e585f81af 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -294,7 +294,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def);
>  virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
>  
>  int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> -                                const char *filename);
> +                                const char *filename,
> +                                bool privileged);
>  
>  virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver);
>  bool virQEMUDriverIsPrivileged(virQEMUDriverPtr driver);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3adec5c14..c3dcea0c4 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7643,21 +7643,8 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      int ret = -1;
>  
> -    if (!virBitmapIsBitSet(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT)) {
> -        ret = 0;
> -        goto cleanup;
> -    }
> -
> -    if (!virQEMUDriverIsPrivileged(driver)) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("cannot use namespaces in session mode"));
> -        goto cleanup;
> -    }
> -
> -    if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
> -        goto cleanup;
> -
> -    if (qemuDomainEnableNamespace(vm, QEMU_DOMAIN_NS_MOUNT) < 0)
> +    if (virBitmapIsBitSet(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) &&
> +        qemuDomainEnableNamespace(vm, QEMU_DOMAIN_NS_MOUNT) < 0)
>          goto cleanup;
>  
>      ret = 0;
> @@ -7667,6 +7654,32 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
>  }
>  
>  
> +bool
> +qemuDomainNamespaceAvailable(qemuDomainNamespace ns)
> +{
> +
> +    switch (ns) {
> +    case QEMU_DOMAIN_NS_MOUNT:
> +#if !defined(__linux__)
> +        /* Namespaces are Linux specific. */
> +        return false;
> +#endif
> +#if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
> +        /* We can't create the exact copy of paths if either of
> +         * these is not available. */
> +        return false;
> +#endif

Pretty sure this will cause the compiler to complain about
unreachable code paths because you'll get

    return false;
    return false;
    if (virProcessNamespaceAvailable(....)

You need to use #elif for the acl/selnux check, and then
use an #else for this last bit:

> +        if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
> +            return false;
> +        break;
> +    case QEMU_DOMAIN_NS_LAST:
> +        break;
> +    }

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_conf: Check for namespaces availability more wisely
Posted by Michal Privoznik 7 years, 1 month ago
On 02/15/2017 10:43 AM, Daniel P. Berrange wrote:
> On Wed, Feb 15, 2017 at 10:20:27AM +0100, Michal Privoznik wrote:
>> The bare fact that mnt namespace is available is not enough for
>> us to allow/enable qemu namespaces feature. There are other
>> requirements: we must copy all the ACL & SELinux labels otherwise
>> we might grant access that is administratively forbidden or vice
>> versa.
>> At the same time, the check for namespace prerequisites is moved
>> from domain startup time to qemu.conf parser as it doesn't make
>> much sense to allow users to start misconfigured libvirt just to
>> find out they can't start a single domain.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_conf.c   | 20 ++++++++++++++++----
>>  src/qemu/qemu_conf.h   |  3 ++-
>>  src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++---------------
>>  src/qemu/qemu_domain.h |  2 ++
>>  src/qemu/qemu_driver.c |  2 +-
>>  5 files changed, 49 insertions(+), 21 deletions(-)
>>


>> +bool
>> +qemuDomainNamespaceAvailable(qemuDomainNamespace ns)
>> +{
>> +
>> +    switch (ns) {
>> +    case QEMU_DOMAIN_NS_MOUNT:
>> +#if !defined(__linux__)
>> +        /* Namespaces are Linux specific. */
>> +        return false;
>> +#endif
>> +#if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
>> +        /* We can't create the exact copy of paths if either of
>> +         * these is not available. */
>> +        return false;
>> +#endif
> 
> Pretty sure this will cause the compiler to complain about
> unreachable code paths because you'll get
> 
>     return false;
>     return false;
>     if (virProcessNamespaceAvailable(....)

Ah. Obviously. What about this?

+bool
+qemuDomainNamespaceAvailable(qemuDomainNamespace ns ATTRIBUTE_UNUSED)
+{
+#if !defined(__linux__)
+    /* Namespaces are Linux specific. */
+    return false;
+
+#else /* defined(__linux__) */
+
+    switch (ns) {
+    case QEMU_DOMAIN_NS_MOUNT:
+# if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
+        /* We can't create the exact copy of paths if either of
+         * these is not available. */
+        return false;
+# else
+        if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
+            return false;
+# endif
+        break;
+    case QEMU_DOMAIN_NS_LAST:
+        break;
+    }
+
+    return true;
+#endif /* defined(__linux__) */
+}
+
+

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_conf: Check for namespaces availability more wisely
Posted by Daniel P. Berrange 7 years, 1 month ago
On Wed, Feb 15, 2017 at 11:19:24AM +0100, Michal Privoznik wrote:
> On 02/15/2017 10:43 AM, Daniel P. Berrange wrote:
> > On Wed, Feb 15, 2017 at 10:20:27AM +0100, Michal Privoznik wrote:
> >> The bare fact that mnt namespace is available is not enough for
> >> us to allow/enable qemu namespaces feature. There are other
> >> requirements: we must copy all the ACL & SELinux labels otherwise
> >> we might grant access that is administratively forbidden or vice
> >> versa.
> >> At the same time, the check for namespace prerequisites is moved
> >> from domain startup time to qemu.conf parser as it doesn't make
> >> much sense to allow users to start misconfigured libvirt just to
> >> find out they can't start a single domain.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_conf.c   | 20 ++++++++++++++++----
> >>  src/qemu/qemu_conf.h   |  3 ++-
> >>  src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++---------------
> >>  src/qemu/qemu_domain.h |  2 ++
> >>  src/qemu/qemu_driver.c |  2 +-
> >>  5 files changed, 49 insertions(+), 21 deletions(-)
> >>
> 
> 
> >> +bool
> >> +qemuDomainNamespaceAvailable(qemuDomainNamespace ns)
> >> +{
> >> +
> >> +    switch (ns) {
> >> +    case QEMU_DOMAIN_NS_MOUNT:
> >> +#if !defined(__linux__)
> >> +        /* Namespaces are Linux specific. */
> >> +        return false;
> >> +#endif
> >> +#if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
> >> +        /* We can't create the exact copy of paths if either of
> >> +         * these is not available. */
> >> +        return false;
> >> +#endif
> > 
> > Pretty sure this will cause the compiler to complain about
> > unreachable code paths because you'll get
> > 
> >     return false;
> >     return false;
> >     if (virProcessNamespaceAvailable(....)
> 
> Ah. Obviously. What about this?
> 
> +bool
> +qemuDomainNamespaceAvailable(qemuDomainNamespace ns ATTRIBUTE_UNUSED)
> +{
> +#if !defined(__linux__)
> +    /* Namespaces are Linux specific. */
> +    return false;
> +
> +#else /* defined(__linux__) */
> +
> +    switch (ns) {
> +    case QEMU_DOMAIN_NS_MOUNT:
> +# if !defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)
> +        /* We can't create the exact copy of paths if either of
> +         * these is not available. */
> +        return false;
> +# else
> +        if (virProcessNamespaceAvailable(VIR_PROCESS_NAMESPACE_MNT) < 0)
> +            return false;
> +# endif
> +        break;
> +    case QEMU_DOMAIN_NS_LAST:
> +        break;
> +    }
> +
> +    return true;
> +#endif /* defined(__linux__) */
> +}
> +

ACK that looks fine.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list