Re: [RFC 3/3] lxc: Mount NBD devices before clone

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/15/2018 05:25 PM, Radostin Stoyanov wrote:
> When user-namespace is enabled we are not allowed
> to mount block/NBD devices.
> 
> Instead, mount /dev/nbdX to /run/libvirt/lxc/<domain>.root
> and set:
> 
> 	fs->src->path = /run/libvirt/lxc/<domain>.root
> 	fs->type = VIR_DOMAIN_FS_TYPE_MOUNT
> ---
>  src/lxc/lxc_container.c  | 53 ------------------------------------------------
>  src/lxc/lxc_controller.c | 49 +++++++++++++++++++++++++++++---------------
>  2 files changed, 33 insertions(+), 69 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 3b8cb966e..420bb20ab 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -658,55 +658,6 @@ static int lxcContainerResolveSymlinks(virDomainFSDefPtr fs, bool gentle)
>      return 0;
>  }
>  

Why is this being removed?  Not clear from commit message...

> -static int lxcContainerPrepareRoot(virDomainDefPtr def,
> -                                   virDomainFSDefPtr root,
> -                                   const char *sec_mount_options)
> -{
> -    char *dst;
> -    char *tmp;
> -
> -    VIR_DEBUG("Prepare root %d", root->type);
> -
> -    if (root->type == VIR_DOMAIN_FS_TYPE_MOUNT)
> -        return 0;
> -
> -    if (root->type == VIR_DOMAIN_FS_TYPE_FILE) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Unexpected root filesystem without loop device"));
> -        return -1;
> -    }
> -
> -    if (root->type != VIR_DOMAIN_FS_TYPE_BLOCK) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("Unsupported root filesystem type %s"),
> -                       virDomainFSTypeToString(root->type));
> -        return -1;
> -    }
> -
> -    if (lxcContainerResolveSymlinks(root, false) < 0)
> -        return -1;
> -
> -    if (virAsprintf(&dst, "%s/%s.root",
> -                    LXC_STATE_DIR, def->name) < 0)
> -        return -1;
> -
> -    tmp = root->dst;
> -    root->dst = dst;
> -
> -    if (lxcContainerMountFSBlock(root, "", sec_mount_options) < 0) {
> -        root->dst = tmp;
> -        VIR_FREE(dst);
> -        return -1;
> -    }
> -
> -    root->dst = tmp;
> -    root->type = VIR_DOMAIN_FS_TYPE_MOUNT;
> -    VIR_FREE(root->src->path);
> -    root->src->path = dst;
> -
> -    return 0;
> -}
> -
>  static int lxcContainerPivotRoot(virDomainFSDefPtr root)
>  {
>      int ret;
> @@ -1755,10 +1706,6 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef,
>      if (virFileResolveAllLinks(LXC_STATE_DIR, &stateDir) < 0)
>          goto cleanup;
>  
> -    /* Ensure the root filesystem is mounted */
> -    if (lxcContainerPrepareRoot(vmDef, root, sec_mount_options) < 0)
> -        goto cleanup;
> -
>      /* Gives us a private root, leaving all parent OS mounts on /.oldroot */
>      if (lxcContainerPivotRoot(root) < 0)
>          goto cleanup;
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 61d9ed07b..d1ae60b1d 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -530,9 +530,12 @@ static int virLXCControllerAppendNBDPids(virLXCControllerPtr ctrl,
>  }
>  
>  
> -static int virLXCControllerSetupNBDDeviceFS(virDomainFSDefPtr fs)
> +static int virLXCControllerSetupNBDDeviceFS(virLXCControllerPtr ctrl,
> +                                            virDomainFSDefPtr fs)
>  {
> -    char *dev;
> +    char *dev, *dst, *tmp, *sec_mount_options;

There are those that prefer one per line.

> +    virDomainDefPtr def = ctrl->def;
> +    virSecurityManagerPtr securityDriver = ctrl->securityManager;
>  
>      if (fs->format <= VIR_STORAGE_FILE_NONE) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -540,22 +543,42 @@ static int virLXCControllerSetupNBDDeviceFS(virDomainFSDefPtr fs)
>          return -1;
>      }
>  
> +    if (virAsprintf(&dst, "%s/%s.root/",
> +                    LXC_STATE_DIR, def->name) < 0)
> +        return -1;
> +
> +    if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, def)))
> +        return -1;

This would leak dst

> +
>      if (virFileNBDDeviceAssociate(fs->src->path,
>                                    fs->format,
>                                    fs->readonly,
>                                    &dev) < 0)
>          return -1;

This would leak dst, sec_mount_options

>  
> -    VIR_DEBUG("Changing fs %s to use type=block for dev %s",
> -              fs->src->path, dev);
> -    /*
> -     * We now change it into a block device type, so that
> -     * the rest of container setup 'just works'
> -     */
> -    fs->type = VIR_DOMAIN_FS_TYPE_BLOCK;
>      VIR_FREE(fs->src->path);
>      fs->src->path = dev;
>  
> +    tmp = fs->dst;
> +    fs->dst = dst;
> +
> +    if (lxcContainerMountFSBlock(fs, "", sec_mount_options) < 0) {
> +        fs->dst = tmp;
> +        VIR_FREE(dst);

This would leak sec_mount_options

> +        return -1;
> +    }
> +
> +    fs->dst = tmp;
> +    fs->type = VIR_DOMAIN_FS_TYPE_MOUNT;
> +
> +    /* The NBD device will be cleaned up while the cgroup will end.
> +     * For this we need to remember the qemu-nbd pid and add it to
> +     * the cgroup*/
> +    if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)

Bad cut-n-paste...

What would we do if < 0?? VIR_FREE(fs->src->path);

otherwise, we'd leak it and just set @dst to fs->src->path

> +
> +    VIR_FREE(fs->src->path);
> +    fs->src->path = dst;
> +

still leaving sec_mount_options leaked.

Perhaps real cleanup type processing should be used instead and an
initialized "int ret = -1;" with the ret = 0 once everything is
successful.  You can use VIR_STEAL_PTR for dst and dev and have cleanup
have the VIR_FREE for dev, dst, and sec_mount_options assuming they are
all initialized to NULL.

>      return 0;
>  }
>  
> @@ -637,13 +660,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl)
>              }
>              ctrl->loopDevFds[ctrl->nloopDevs - 1] = fd;
>          } else if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_NBD) {
> -            if (virLXCControllerSetupNBDDeviceFS(fs) < 0)
> -                goto cleanup;
> -
> -            /* The NBD device will be cleaned up while the cgroup will end.
> -             * For this we need to remember the qemu-nbd pid and add it to
> -             * the cgroup*/
> -            if (virLXCControllerAppendNBDPids(ctrl, fs->src->path) < 0)
> +            if (virLXCControllerSetupNBDDeviceFS(ctrl, fs) < 0)
>                  goto cleanup;

Perhaps a patch between this and the last one to move the call to
virLXCControllerAppendNBDPids into virLXCControllerSetupNBDDeviceFS
would be appropriate and return -1 instead of cleanup just before return
0.

That may make this followup patch a bit easier to follow.

John

>          } else {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux