While the code allows devices to already be there (by some
miracle), we shouldn't try to create devices that don't belong to
us. For instance, we shouldn't try to create /dev/shm/file
because /dev/shm is a mount point that is preserved. Therefore if
a file is created there from an outside (e.g. by mgmt application
or some other daemon running on the system like vhostmd), it
exists in the qemu namespace too as the mount point is the same.
It's only /dev and /dev only that is different. The same
reasoning applies to all other preserved mount points.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9e18f7e..5840c57 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7339,6 +7339,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
 
 struct qemuDomainCreateDeviceData {
     const char *path;     /* Path to temp new /dev location */
+    char * const *devMountsPath;
+    size_t ndevMountsPath;
 };
 
 
@@ -7392,17 +7394,34 @@ qemuDomainCreateDeviceRecursive(const char *device,
      * For now, lets hope callers play nice.
      */
     if (STRPREFIX(device, DEVPREFIX)) {
-        if (virAsprintf(&devicePath, "%s/%s",
-                        data->path, device + strlen(DEVPREFIX)) < 0)
-            goto cleanup;
+        size_t i;
 
-        if (virFileMakeParentPath(devicePath) < 0) {
-            virReportSystemError(errno,
-                                 _("Unable to create %s"),
-                                 devicePath);
-            goto cleanup;
+        for (i = 0; i < data->ndevMountsPath; i++) {
+            if (STREQ(data->devMountsPath[i], "/dev"))
+                continue;
+            if (STRPREFIX(device, data->devMountsPath[i]))
+                break;
+        }
+
+        if (i == data->ndevMountsPath) {
+            /* Okay, @device is in /dev but not in any mount point under /dev.
+             * Create it. */
+            if (virAsprintf(&devicePath, "%s/%s",
+                            data->path, device + strlen(DEVPREFIX)) < 0)
+                goto cleanup;
+
+            if (virFileMakeParentPath(devicePath) < 0) {
+                virReportSystemError(errno,
+                                     _("Unable to create %s"),
+                                     devicePath);
+                goto cleanup;
+            }
+            VIR_DEBUG("Creating dev %s", device);
+            create = true;
+        } else {
+            VIR_DEBUG("Skipping dev %s because of %s mount point",
+                      device, data->devMountsPath[i]);
         }
-        create = true;
     }
 
     if (isLink) {
@@ -7951,6 +7970,8 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
     }
 
     data.path = devPath;
+    data.devMountsPath = devMountsPath;
+    data.ndevMountsPath = ndevMountsPath;
 
     if (virProcessSetupPrivateMountNS() < 0)
         goto cleanup;
-- 
2.10.2

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

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> While the code allows devices to already be there (by some
> miracle), we shouldn't try to create devices that don't belong to
> us. For instance, we shouldn't try to create /dev/shm/file
> because /dev/shm is a mount point that is preserved. Therefore if
> a file is created there from an outside (e.g. by mgmt application
> or some other daemon running on the system like vhostmd), it
> exists in the qemu namespace too as the mount point is the same.
> It's only /dev and /dev only that is different. The same

One 'only' should be dropped perhaps?

> reasoning applies to all other preserved mount points.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9e18f7e..5840c57 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7339,6 +7339,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
>  
>  struct qemuDomainCreateDeviceData {
>      const char *path;     /* Path to temp new /dev location */
> +    char * const *devMountsPath;
> +    size_t ndevMountsPath;
>  };
>  
>  
> @@ -7392,17 +7394,34 @@ qemuDomainCreateDeviceRecursive(const char *device,
>       * For now, lets hope callers play nice.
>       */
>      if (STRPREFIX(device, DEVPREFIX)) {
> -        if (virAsprintf(&devicePath, "%s/%s",
> -                        data->path, device + strlen(DEVPREFIX)) < 0)
> -            goto cleanup;
> +        size_t i;
>  
> -        if (virFileMakeParentPath(devicePath) < 0) {
> -            virReportSystemError(errno,
> -                                 _("Unable to create %s"),
> -                                 devicePath);
> -            goto cleanup;
> +        for (i = 0; i < data->ndevMountsPath; i++) {
> +            if (STREQ(data->devMountsPath[i], "/dev"))
> +                continue;
> +            if (STRPREFIX(device, data->devMountsPath[i]))
> +                break;
> +        }
> +
> +        if (i == data->ndevMountsPath) {
> +            /* Okay, @device is in /dev but not in any mount point under /dev.
> +             * Create it. */
> +            if (virAsprintf(&devicePath, "%s/%s",
> +                            data->path, device + strlen(DEVPREFIX)) < 0)
> +                goto cleanup;
> +
> +            if (virFileMakeParentPath(devicePath) < 0) {
> +                virReportSystemError(errno,
> +                                     _("Unable to create %s"),
> +                                     devicePath);
> +                goto cleanup;
> +            }
> +            VIR_DEBUG("Creating dev %s", device);
> +            create = true;
> +        } else {
> +            VIR_DEBUG("Skipping dev %s because of %s mount point",
> +                      device, data->devMountsPath[i]);
>          }
> -        create = true;
>      }
>  
>      if (isLink) {
> @@ -7951,6 +7970,8 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>      }
>  
>      data.path = devPath;
> +    data.devMountsPath = devMountsPath;
> +    data.ndevMountsPath = ndevMountsPath;
>  
>      if (virProcessSetupPrivateMountNS() < 0)
>          goto cleanup;

ACK

--
Cedric

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