When setting up mount namespace for a qemu domain the following steps are executed: 1) get list of mountpoints under /dev/ 2) move them to /var/run/libvirt/qemu/$domName.ext 3) start constructing new device tree under /var/run/libvirt/qemu/$domName.dev 4) move the mountpoint of the new device tree to /dev 5) restore original mountpoints from step 2) Not the problem with this approach is that if some device in step

3) requires access to a mountpoint from step 2) it will fail as the mountpoint is not there anymore. For instance consider the following domain disk configuration:     <disk type='file' device='disk'>       <driver name='qemu' type='raw'/>       <source file='/dev/shm/vhostmd0'/>       <target dev='vdb' bus='virtio'/>       <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>     </disk> In this case operation fails as we are unable to create vhostmd0 in the new device tree because after step 2) there is no /dev/shm anymore. Leave aside fact that we shouldn't try to create devices living in other mountpoints. That's a separate bug that will be addressed later. Currently, the order described above is rearranged to: 1) get list of mountpoints under /dev/ 2) start constructing new device tree under /var/run/libvirt/qemu/$domName.dev 3) move them to /var/run/libvirt/qemu/$domName.ext 4) move the mountpoint of the new device tree to /dev 5) restore original mountpoints from step 3) Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ---  src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++-------------------------  1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 00b0b4a..be02d54 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7950,33 +7950,6 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,      if (qemuDomainSetupDev(cfg, mgr, vm, devPath) < 0)          goto cleanup;   -    /* Save some mount points because we want to share them with the host */ -    for (i = 0; i < ndevMountsPath; i++) { -        struct stat sb; - -        if (devMountsSavePath[i] == devPath) -            continue; - -        if (stat(devMountsPath[i], &sb) < 0) { -            virReportSystemError(errno, -                                 _("Unable to stat: %s"), -                                 devMountsPath[i]); -            goto cleanup; -        } - -        /* At this point, devMountsPath is either a regular file or a directory. */ -        if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) || -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) { -            virReportSystemError(errno, -                                 _("Failed to create %s"), -                                 devMountsSavePath[i]); -            goto cleanup; -        } - -        if (virFileMoveMount(devMountsPath[i], devMountsSavePath[i]) < 0) -            goto cleanup; -    } -      if (qemuDomainSetupAllDisks(cfg, vm, devPath) < 0)          goto cleanup;   @@ -8001,6 +7974,33 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,      if (qemuDomainSetupAllRNGs(cfg, vm, devPath) < 0)          goto cleanup;   +    /* Save some mount points because we want to share them with the host */ +    for (i = 0; i < ndevMountsPath; i++) { +        struct stat sb; + +        if (devMountsSavePath[i] == devPath) +            continue; + +        if (stat(devMountsPath[i], &sb) < 0) { +            virReportSystemError(errno, +                                 _("Unable to stat: %s"), +                                 devMountsPath[i]); +            goto cleanup; +        } + +        /* At this point, devMountsPath is either a regular file or a directory. */ +        if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) || +            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) { +            virReportSystemError(errno, +                                 _("Failed to create %s"), +                                 devMountsSavePath[i]); +            goto cleanup; +        } + +        if (virFileMoveMount(devMountsPath[i], devMountsSavePath[i]) < 0) +            goto cleanup; +    } +      if (virFileMoveMount(devPath, "/dev") < 0)          goto cleanup;  

Currently, all we need to do in qemuDomainCreateDeviceRecursive() is to take given @device, get all kinds of info on it (major & minor numbers, owner, seclabels) and create its copy at a temporary location @path (usually /var/run/libvirt/qemu/$domName.dev), if @device live under /dev. This is, however, very loose condition, as it also means /dev/shm/* is created too. Therefor, we will need to pass more arguments into the function for better decision making (e.g. list of mount points under /dev). Instead of adding more arguments to all the functions (not easily reachable because some functions are callback with strictly defined type), lets just turn this one 'const char *' into a 'struct *'. New "arguments" can be then added at no cost. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ---  src/qemu/qemu_domain.c | 106 ++++++++++++++++++++++++++-----------------------  1 file changed, 57 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index be02d54..9e18f7e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7337,9 +7337,14 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,  }     +struct qemuDomainCreateDeviceData { +    const char *path;     /* Path to temp new /dev location */ +}; + +  static int  qemuDomainCreateDeviceRecursive(const char *device, -                                const char *path, +                                const struct qemuDomainCreateDeviceData *data,                                  bool allow_noent,                                  unsigned int ttl)  { @@ -7388,7 +7393,7 @@ qemuDomainCreateDeviceRecursive(const char *device,       */      if (STRPREFIX(device, DEVPREFIX)) {          if (virAsprintf(&devicePath, "%s/%s", -                        path, device + strlen(DEVPREFIX)) < 0) +                        data->path, device + strlen(DEVPREFIX)) < 0)              goto cleanup;            if (virFileMakeParentPath(devicePath) < 0) { @@ -7449,7 +7454,7 @@ qemuDomainCreateDeviceRecursive(const char *device,              tmp = NULL;          }   -        if (qemuDomainCreateDeviceRecursive(target, path, +        if (qemuDomainCreateDeviceRecursive(target, data,                                              allow_noent, ttl - 1) < 0)              goto cleanup;      } else { @@ -7533,12 +7538,12 @@ qemuDomainCreateDeviceRecursive(const char *device,    static int  qemuDomainCreateDevice(const char *device, -                       const char *path, +                       const struct qemuDomainCreateDeviceData *data,                         bool allow_noent)  {      long symloop_max = sysconf(_SC_SYMLOOP_MAX);   -    return qemuDomainCreateDeviceRecursive(device, path, +    return qemuDomainCreateDeviceRecursive(device, data,                                             allow_noent, symloop_max);  }   @@ -7546,7 +7551,7 @@ qemuDomainCreateDevice(const char *device,  static int  qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,                            virDomainObjPtr vm ATTRIBUTE_UNUSED, -                          const char *path) +                          const struct qemuDomainCreateDeviceData *data)  {      const char *const *devices = (const char *const *) cfg->cgroupDeviceACL;      size_t i; @@ -7556,7 +7561,7 @@ qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,          devices = defaultDeviceACL;        for (i = 0; devices[i]; i++) { -        if (qemuDomainCreateDevice(devices[i], path, true) < 0) +        if (qemuDomainCreateDevice(devices[i], data, true) < 0)              goto cleanup;      }   @@ -7570,7 +7575,7 @@ static int  qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,                     virSecurityManagerPtr mgr,                     virDomainObjPtr vm, -                   const char *path) +                   const struct qemuDomainCreateDeviceData *data)  {      char *mount_options = NULL;      char *opts = NULL; @@ -7592,10 +7597,10 @@ qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,                      "mode=755,size=65536%s", mount_options) < 0)          goto cleanup;   -    if (virFileSetupDev(path, opts) < 0) +    if (virFileSetupDev(data->path, opts) < 0)          goto cleanup;   -    if (qemuDomainPopulateDevices(cfg, vm, path) < 0) +    if (qemuDomainPopulateDevices(cfg, vm, data) < 0)          goto cleanup;        ret = 0; @@ -7609,7 +7614,7 @@ qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,  static int  qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                      virDomainDiskDefPtr disk, -                    const char *devPath) +                    const struct qemuDomainCreateDeviceData *data)  {      virStorageSourcePtr next;      char *dst = NULL; @@ -7621,7 +7626,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,              continue;          }   -        if (qemuDomainCreateDevice(next->path, devPath, false) < 0) +        if (qemuDomainCreateDevice(next->path, data, false) < 0)              goto cleanup;      }   @@ -7635,7 +7640,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupAllDisks(virQEMUDriverConfigPtr cfg,                          virDomainObjPtr vm, -                        const char *devPath) +                        const struct qemuDomainCreateDeviceData *data)  {      size_t i;      VIR_DEBUG("Setting up disks"); @@ -7643,7 +7648,7 @@ qemuDomainSetupAllDisks(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->ndisks; i++) {          if (qemuDomainSetupDisk(cfg,                                  vm->def->disks[i], -                                devPath) < 0) +                                data) < 0)              return -1;      }   @@ -7655,7 +7660,7 @@ qemuDomainSetupAllDisks(virQEMUDriverConfigPtr cfg,  static int  qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                         virDomainHostdevDefPtr dev, -                       const char *devPath) +                       const struct qemuDomainCreateDeviceData *data)  {      int ret = -1;      char **path = NULL; @@ -7665,7 +7670,7 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,          goto cleanup;        for (i = 0; i < npaths; i++) { -        if (qemuDomainCreateDevice(path[i], devPath, false) < 0) +        if (qemuDomainCreateDevice(path[i], data, false) < 0)              goto cleanup;      }   @@ -7681,7 +7686,7 @@ qemuDomainSetupHostdev(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupAllHostdevs(virQEMUDriverConfigPtr cfg,                             virDomainObjPtr vm, -                           const char *devPath) +                           const struct qemuDomainCreateDeviceData *data)  {      size_t i;   @@ -7689,7 +7694,7 @@ qemuDomainSetupAllHostdevs(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->nhostdevs; i++) {          if (qemuDomainSetupHostdev(cfg,                                     vm->def->hostdevs[i], -                                   devPath) < 0) +                                   data) < 0)              return -1;      }      VIR_DEBUG("Setup all hostdevs"); @@ -7700,19 +7705,19 @@ qemuDomainSetupAllHostdevs(virQEMUDriverConfigPtr cfg,  static int  qemuDomainSetupMemory(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                        virDomainMemoryDefPtr mem, -                      const char *devPath) +                      const struct qemuDomainCreateDeviceData *data)  {      if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM)          return 0;   -    return qemuDomainCreateDevice(mem->nvdimmPath, devPath, false); +    return qemuDomainCreateDevice(mem->nvdimmPath, data, false);  }      static int  qemuDomainSetupAllMemories(virQEMUDriverConfigPtr cfg,                             virDomainObjPtr vm, -                           const char *devPath) +                           const struct qemuDomainCreateDeviceData *data)  {      size_t i;   @@ -7720,7 +7725,7 @@ qemuDomainSetupAllMemories(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->nmems; i++) {          if (qemuDomainSetupMemory(cfg,                                    vm->def->mems[i], -                                  devPath) < 0) +                                  data) < 0)              return -1;      }      VIR_DEBUG("Setup all memories"); @@ -7733,26 +7738,26 @@ qemuDomainSetupChardev(virDomainDefPtr def ATTRIBUTE_UNUSED,                         virDomainChrDefPtr dev,                         void *opaque)  { -    const char *devPath = opaque; +    const struct qemuDomainCreateDeviceData *data = opaque;        if (dev->source->type != VIR_DOMAIN_CHR_TYPE_DEV)          return 0;   -    return qemuDomainCreateDevice(dev->source->data.file.path, devPath, false); +    return qemuDomainCreateDevice(dev->source->data.file.path, data, false);  }      static int  qemuDomainSetupAllChardevs(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                             virDomainObjPtr vm, -                           const char *devPath) +                           const struct qemuDomainCreateDeviceData *data)  {      VIR_DEBUG("Setting up chardevs");        if (virDomainChrDefForeach(vm->def,                                 true,                                 qemuDomainSetupChardev, -                               (void *) devPath) < 0) +                               (void *) data) < 0)          return -1;        VIR_DEBUG("Setup all chardevs"); @@ -7763,7 +7768,7 @@ qemuDomainSetupAllChardevs(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupTPM(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                     virDomainObjPtr vm, -                   const char *devPath) +                   const struct qemuDomainCreateDeviceData *data)  {      virDomainTPMDefPtr dev = vm->def->tpm;   @@ -7775,7 +7780,7 @@ qemuDomainSetupTPM(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,      switch (dev->type) {      case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:          if (qemuDomainCreateDevice(dev->data.passthrough.source.data.file.path, -                                   devPath, false) < 0) +                                   data, false) < 0)              return -1;          break;   @@ -7792,7 +7797,7 @@ qemuDomainSetupTPM(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                          virDomainGraphicsDefPtr gfx, -                        const char *devPath) +                        const struct qemuDomainCreateDeviceData *data)  {      const char *rendernode = gfx->data.spice.rendernode;   @@ -7801,14 +7806,14 @@ qemuDomainSetupGraphics(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,          !rendernode)          return 0;   -    return qemuDomainCreateDevice(rendernode, devPath, false); +    return qemuDomainCreateDevice(rendernode, data, false);  }      static int  qemuDomainSetupAllGraphics(virQEMUDriverConfigPtr cfg,                             virDomainObjPtr vm, -                           const char *devPath) +                           const struct qemuDomainCreateDeviceData *data)  {      size_t i;   @@ -7816,7 +7821,7 @@ qemuDomainSetupAllGraphics(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->ngraphics; i++) {          if (qemuDomainSetupGraphics(cfg,                                      vm->def->graphics[i], -                                    devPath) < 0) +                                    data) < 0)              return -1;      }   @@ -7828,13 +7833,13 @@ qemuDomainSetupAllGraphics(virQEMUDriverConfigPtr cfg,  static int  qemuDomainSetupInput(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                       virDomainInputDefPtr input, -                     const char *devPath) +                     const struct qemuDomainCreateDeviceData *data)  {      int ret = -1;        switch ((virDomainInputType) input->type) {      case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: -        if (qemuDomainCreateDevice(input->source.evdev, devPath, false) < 0) +        if (qemuDomainCreateDevice(input->source.evdev, data, false) < 0)              goto cleanup;          break;   @@ -7855,7 +7860,7 @@ qemuDomainSetupInput(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupAllInputs(virQEMUDriverConfigPtr cfg,                           virDomainObjPtr vm, -                         const char *devPath) +                         const struct qemuDomainCreateDeviceData *data)  {      size_t i;   @@ -7863,7 +7868,7 @@ qemuDomainSetupAllInputs(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->ninputs; i++) {          if (qemuDomainSetupInput(cfg,                                   vm->def->inputs[i], -                                 devPath) < 0) +                                 data) < 0)              return -1;      }      VIR_DEBUG("Setup all inputs"); @@ -7874,11 +7879,11 @@ qemuDomainSetupAllInputs(virQEMUDriverConfigPtr cfg,  static int  qemuDomainSetupRNG(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,                     virDomainRNGDefPtr rng, -                   const char *devPath) +                   const struct qemuDomainCreateDeviceData *data)  {      switch ((virDomainRNGBackend) rng->backend) {      case VIR_DOMAIN_RNG_BACKEND_RANDOM: -        if (qemuDomainCreateDevice(rng->source.file, devPath, false) < 0) +        if (qemuDomainCreateDevice(rng->source.file, data, false) < 0)              return -1;        case VIR_DOMAIN_RNG_BACKEND_EGD: @@ -7894,7 +7899,7 @@ qemuDomainSetupRNG(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,  static int  qemuDomainSetupAllRNGs(virQEMUDriverConfigPtr cfg,                         virDomainObjPtr vm, -                       const char *devPath) +                       const struct qemuDomainCreateDeviceData *data)  {      size_t i;   @@ -7902,7 +7907,7 @@ qemuDomainSetupAllRNGs(virQEMUDriverConfigPtr cfg,      for (i = 0; i < vm->def->nrngs; i++) {          if (qemuDomainSetupRNG(cfg,                                 vm->def->rngs[i], -                               devPath) < 0) +                               data) < 0)              return -1;      }   @@ -7916,6 +7921,7 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,                           virSecurityManagerPtr mgr,                           virDomainObjPtr vm)  { +    struct qemuDomainCreateDeviceData data;      char *devPath = NULL;      char **devMountsPath = NULL, **devMountsSavePath = NULL;      size_t ndevMountsPath = 0, i; @@ -7944,34 +7950,36 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,          goto cleanup;      }   +    data.path = devPath; +      if (virProcessSetupPrivateMountNS() < 0)          goto cleanup;   -    if (qemuDomainSetupDev(cfg, mgr, vm, devPath) < 0) +    if (qemuDomainSetupDev(cfg, mgr, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllDisks(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllDisks(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllHostdevs(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllHostdevs(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllMemories(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllMemories(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllChardevs(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllChardevs(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupTPM(cfg, vm, devPath) < 0) +    if (qemuDomainSetupTPM(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllGraphics(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllGraphics(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllInputs(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllInputs(cfg, vm, &data) < 0)          goto cleanup;   -    if (qemuDomainSetupAllRNGs(cfg, vm, devPath) < 0) +    if (qemuDomainSetupAllRNGs(cfg, vm, &data) < 0)          goto cleanup;        /* Save some mount points because we want to share them with the host */

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(a)redhat.com&gt; ---  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;

Just like in previous commit, this fixes the same issue for hotplug. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ---  src/qemu/qemu_domain.c | 112 ++++++++++++++++++++++++++++++++++++++++++-------  1 file changed, 97 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5840c57..60f8f01 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8238,6 +8238,8 @@ static int  qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,                                       virDomainObjPtr vm,                                       const char *file, +                                     char * const *devMountsPath, +                                     size_t ndevMountsPath,                                       unsigned int ttl)  {      struct qemuDomainAttachDeviceMknodData data; @@ -8315,20 +8317,36 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,  #endif        if (STRPREFIX(file, DEVPREFIX)) { -        if (qemuSecurityPreFork(driver->securityManager) < 0) -            goto cleanup; +        size_t i;   -        if (virProcessRunInMountNamespace(vm->pid, -                                          qemuDomainAttachDeviceMknodHelper, -                                          &data) < 0) { +        for (i = 0; i < ndevMountsPath; i++) { +            if (STREQ(devMountsPath[i], "/dev")) +                continue; +            if (STRPREFIX(file, devMountsPath[i])) +                break; +        } + +        if (i == ndevMountsPath) { +            if (qemuSecurityPreFork(driver->securityManager) < 0) +                goto cleanup; + +            if (virProcessRunInMountNamespace(vm->pid, +                                              qemuDomainAttachDeviceMknodHelper, +                                              &data) < 0) { +                qemuSecurityPostFork(driver->securityManager); +                goto cleanup; +            }              qemuSecurityPostFork(driver->securityManager); -            goto cleanup; +        } else { +            VIR_DEBUG("Skipping dev %s because of %s mount point", +                      file, devMountsPath[i]);          } -        qemuSecurityPostFork(driver->securityManager);      }        if (isLink && -        qemuDomainAttachDeviceMknodRecursive(driver, vm, target, ttl -1) < 0) +        qemuDomainAttachDeviceMknodRecursive(driver, vm, target, +                                             devMountsPath, ndevMountsPath, +                                             ttl -1) < 0)          goto cleanup;        ret = 0; @@ -8345,11 +8363,15 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,  static int  qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,                              virDomainObjPtr vm, -                            const char *file) +                            const char *file, +                            char * const *devMountsPath, +                            size_t ndevMountsPath)  {      long symloop_max = sysconf(_SC_SYMLOOP_MAX);   -    return qemuDomainAttachDeviceMknodRecursive(driver, vm, file, symloop_max); +    return qemuDomainAttachDeviceMknodRecursive(driver, vm, file, +                                                devMountsPath, ndevMountsPath, +                                                symloop_max);  }     @@ -8389,6 +8411,9 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,                               virDomainObjPtr vm,                               virStorageSourcePtr src)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      virStorageSourcePtr next;      struct stat sb;      int ret = -1; @@ -8396,6 +8421,12 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))          return 0;   +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; +      for (next = src; next; next = next->backingStore) {          if (virStorageSourceIsEmpty(next) ||              !virStorageSourceIsLocalStorage(next)) { @@ -8414,12 +8445,15 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,            if (qemuDomainAttachDeviceMknod(driver,                                          vm, -                                        next->path) < 0) +                                        next->path, +                                        devMountsPath, ndevMountsPath) < 0)              goto cleanup;      }        ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8444,6 +8478,9 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,                                  virDomainObjPtr vm,                                  virDomainHostdevDefPtr hostdev)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;      char **path = NULL;      size_t i, npaths = 0; @@ -8454,10 +8491,17 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,      if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path, NULL) < 0)          goto cleanup;   +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; +      for (i = 0; i < npaths; i++) {          if (qemuDomainAttachDeviceMknod(driver,                                          vm, -                                        path[i]) < 0) +                                        path[i], +                                        devMountsPath, ndevMountsPath) < 0)          goto cleanup;      }   @@ -8466,6 +8510,8 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,      for (i = 0; i < npaths; i++)          VIR_FREE(path[i]);      VIR_FREE(path); +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8505,6 +8551,9 @@ qemuDomainNamespaceSetupMemory(virQEMUDriverPtr driver,                                 virDomainObjPtr vm,                                 virDomainMemoryDefPtr mem)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;        if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) @@ -8513,10 +8562,19 @@ qemuDomainNamespaceSetupMemory(virQEMUDriverPtr driver,      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))          return 0;   -    if (qemuDomainAttachDeviceMknod(driver, vm, mem->nvdimmPath) < 0) +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; + +    if (qemuDomainAttachDeviceMknod(driver, vm, mem->nvdimmPath, +                                    devMountsPath, ndevMountsPath) < 0)          goto cleanup;      ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8547,6 +8605,9 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver,                                  virDomainObjPtr vm,                                  virDomainChrDefPtr chr)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      const char *path;      int ret = -1;   @@ -8558,12 +8619,21 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver,        path = chr->source->data.file.path;   +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; +      if (qemuDomainAttachDeviceMknod(driver,                                      vm, -                                    path) < 0) +                                    path, +                                    devMountsPath, ndevMountsPath) < 0)          goto cleanup;      ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8598,6 +8668,9 @@ qemuDomainNamespaceSetupRNG(virQEMUDriverPtr driver,                              virDomainObjPtr vm,                              virDomainRNGDefPtr rng)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      const char *path = NULL;      int ret = -1;   @@ -8615,12 +8688,21 @@ qemuDomainNamespaceSetupRNG(virQEMUDriverPtr driver,          goto cleanup;      }   +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; +      if (qemuDomainAttachDeviceMknod(driver,                                      vm, -                                    path) < 0) +                                    path, +                                    devMountsPath, ndevMountsPath) < 0)          goto cleanup;      ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }  

Even though there are several checks before calling this function and for some scenarios we don't call it at all (e.g. on disk hot unplug), it may be possible to sneak in some weird files (e.g. if domain would have RNG with /dev/shm/some_file as its backend). No matter how improbable, we shouldn't unlink it as we would be unlinking a file from the host which we haven't created in the first place. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; ---  src/qemu/qemu_domain.c | 86 ++++++++++++++++++++++++++++++++++++++++++++------  1 file changed, 76 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 60f8f01..c393d5e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8395,14 +8395,32 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED,  static int  qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,                               virDomainObjPtr vm, -                             const char *file) +                             const char *file, +                             char * const *devMountsPath, +                             size_t ndevMountsPath)  { -    if (virProcessRunInMountNamespace(vm->pid, -                                      qemuDomainDetachDeviceUnlinkHelper, -                                      (void *)file) < 0) -        return -1; +    int ret = -1; +    size_t i;   -    return 0; +    if (STRPREFIX(file, DEVPREFIX)) { +        for (i = 0; i < ndevMountsPath; i++) { +            if (STREQ(devMountsPath[i], "/dev")) +                continue; +            if (STRPREFIX(file, devMountsPath[i])) +                break; +        } + +        if (i == ndevMountsPath) { +            if (virProcessRunInMountNamespace(vm->pid, +                                              qemuDomainDetachDeviceUnlinkHelper, +                                              (void *)file) < 0) +                goto cleanup; +        } +    } + +    ret = 0; + cleanup: +    return ret;  }     @@ -8521,6 +8539,9 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,                                     virDomainObjPtr vm,                                     virDomainHostdevDefPtr hostdev)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;      char **path = NULL;      size_t i, npaths = 0; @@ -8532,8 +8553,15 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,                                   &npaths, &path, NULL) < 0)          goto cleanup;   +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; +      for (i = 0; i < npaths; i++) { -        if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0) +        if (qemuDomainDetachDeviceUnlink(driver, vm, path[i], +                                         devMountsPath, ndevMountsPath) < 0)              goto cleanup;      }   @@ -8542,6 +8570,8 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,      for (i = 0; i < npaths; i++)          VIR_FREE(path[i]);      VIR_FREE(path); +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8584,6 +8614,9 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver,                                    virDomainObjPtr vm,                                    virDomainMemoryDefPtr mem)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;        if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) @@ -8592,10 +8625,19 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver,      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))          return 0;   -    if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath) < 0) +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; + +    if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath, +                                     devMountsPath, ndevMountsPath) < 0)          goto cleanup;      ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8643,6 +8685,9 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver,                                     virDomainObjPtr vm,                                     virDomainChrDefPtr chr)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;      const char *path = NULL;   @@ -8654,11 +8699,20 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver,        path = chr->source->data.file.path;   -    if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0) +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; + +    if (qemuDomainDetachDeviceUnlink(driver, vm, path, +                                     devMountsPath, ndevMountsPath) < 0)          goto cleanup;        ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }   @@ -8712,6 +8766,9 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,                                 virDomainObjPtr vm,                                 virDomainRNGDefPtr rng)  { +    virQEMUDriverConfigPtr cfg = NULL; +    char **devMountsPath = NULL; +    size_t ndevMountsPath = 0;      int ret = -1;      const char *path = NULL;   @@ -8729,11 +8786,20 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,          goto cleanup;      }   -    if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0) +    cfg = virQEMUDriverGetConfig(driver); +    if (qemuDomainGetPreservedMounts(cfg, vm, +                                     &devMountsPath, NULL, +                                     &ndevMountsPath) < 0) +        goto cleanup; + +    if (qemuDomainDetachDeviceUnlink(driver, vm, path, +                                     devMountsPath, ndevMountsPath) < 0)          goto cleanup;        ret = 0;   cleanup: +    virStringListFreeCount(devMountsPath, ndevMountsPath); +    virObjectUnref(cfg);      return ret;  }