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,