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
