[libvirt] [PATCH] Unmount existing filesystems under user specified mounts in LXC
From: "Daniel P. Berrange" <berrange@redhat.com> If the user requests a mount for /run, this may hide any existing mounts that are lower down in /run. The result is that the container still sees the mounts in /proc/mounts, but cannot access them sh-4.2# df df: '/run/user/501/gvfs': No such file or directory df: '/run/media/berrange/LIVE': No such file or directory df: '/run/media/berrange/SecureDiskA1': No such file or directory df: '/run/libvirt/lxc/sandbox': No such file or directory Filesystem 1K-blocks Used Available Use% Mounted on /dev/mapper/vg_t500wlan-lv_root 151476396 135390200 8384900 95% / tmpfs 1970888 3204 1967684 1% /run /dev/sda1 194241 155940 28061 85% /boot devfs 64 0 64 0% /dev tmpfs 64 0 64 0% /sys/fs/cgroup tmpfs 1970888 1200 1969688 1% /etc/libvirt-sandbox/scratch Before mounting any filesystem at a particular location, we must recursively unmount anything at or below the target mount point Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 218 ++++++++++++++++++++++++------------------------ 1 file changed, 111 insertions(+), 107 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index e59bfdf..30738bb 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -409,6 +409,113 @@ static int lxcContainerChildMountSort(const void *a, const void *b) # define MS_SLAVE (1<<19) #endif +static int lxcContainerGetSubtree(const char *prefix, + char ***mountsret, + size_t *nmountsret) +{ + FILE *procmnt; + struct mntent mntent; + char mntbuf[1024]; + int ret = -1; + char **mounts = NULL; + size_t nmounts = 0; + + VIR_DEBUG("prefix=%s", prefix); + + *mountsret = NULL; + *nmountsret = 0; + + if (!(procmnt = setmntent("/proc/mounts", "r"))) { + virReportSystemError(errno, "%s", + _("Failed to read /proc/mounts")); + return -1; + } + + while (getmntent_r(procmnt, &mntent, mntbuf, sizeof(mntbuf)) != NULL) { + VIR_DEBUG("Got %s", mntent.mnt_dir); + if (!STRPREFIX(mntent.mnt_dir, prefix)) + continue; + + if (VIR_REALLOC_N(mounts, nmounts+1) < 0) { + virReportOOMError(); + goto cleanup; + } + if (!(mounts[nmounts] = strdup(mntent.mnt_dir))) { + virReportOOMError(); + goto cleanup; + } + nmounts++; + VIR_DEBUG("Grabbed %s", mntent.mnt_dir); + } + + if (mounts) + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); + + ret = 0; +cleanup: + *mountsret = mounts; + *nmountsret = nmounts; + endmntent(procmnt); + return ret; +} + +static int lxcContainerUnmountSubtree(const char *prefix, + bool isOldRootFS) +{ + char **mounts = NULL; + size_t nmounts = 0; + size_t i; + int saveErrno; + const char *failedUmount = NULL; + int ret = -1; + + VIR_DEBUG("Unmount subtreee from %s", prefix); + + if (lxcContainerGetSubtree(prefix, &mounts, &nmounts) < 0) + goto cleanup; + for (i = 0 ; i < nmounts ; i++) { + VIR_DEBUG("Umount %s", mounts[i]); + if (umount(mounts[i]) < 0) { + char ebuf[1024]; + failedUmount = mounts[i]; + saveErrno = errno; + VIR_WARN("Failed to unmount '%s', trying to detach subtree '%s': %s", + failedUmount, mounts[nmounts-1], + virStrerror(errno, ebuf, sizeof(ebuf))); + break; + } + } + + if (failedUmount) { + /* This detaches the subtree */ + if (umount2(mounts[nmounts-1], MNT_DETACH) < 0) { + virReportSystemError(saveErrno, + _("Failed to unmount '%s' and could not detach subtree '%s'"), + failedUmount, mounts[nmounts-1]); + goto cleanup; + } + /* This unmounts the tmpfs on which the old root filesystem was hosted */ + if (isOldRootFS && + umount(mounts[nmounts-1]) < 0) { + virReportSystemError(saveErrno, + _("Failed to unmount '%s' and could not unmount old root '%s'"), + failedUmount, mounts[nmounts-1]); + goto cleanup; + } + } + + ret = 0; + +cleanup: + for (i = 0 ; i < nmounts ; i++) + VIR_FREE(mounts[i]); + VIR_FREE(mounts); + + return ret; +} + + static int lxcContainerPrepareRoot(virDomainDefPtr def, virDomainFSDefPtr root) { @@ -1254,6 +1361,10 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef, STREQ(vmDef->fss[i]->dst, "/")) continue; + if (lxcContainerUnmountSubtree(vmDef->fss[i]->dst, + false) < 0) + return -1; + if (lxcContainerMountFS(vmDef->fss[i], dstprefix, sec_mount_options) < 0) return -1; } @@ -1624,113 +1735,6 @@ static int lxcContainerSetupAllHostdevs(virDomainDefPtr vmDef, } -static int lxcContainerGetSubtree(const char *prefix, - char ***mountsret, - size_t *nmountsret) -{ - FILE *procmnt; - struct mntent mntent; - char mntbuf[1024]; - int ret = -1; - char **mounts = NULL; - size_t nmounts = 0; - - VIR_DEBUG("prefix=%s", prefix); - - *mountsret = NULL; - *nmountsret = 0; - - if (!(procmnt = setmntent("/proc/mounts", "r"))) { - virReportSystemError(errno, "%s", - _("Failed to read /proc/mounts")); - return -1; - } - - while (getmntent_r(procmnt, &mntent, mntbuf, sizeof(mntbuf)) != NULL) { - VIR_DEBUG("Got %s", mntent.mnt_dir); - if (!STRPREFIX(mntent.mnt_dir, prefix)) - continue; - - if (VIR_REALLOC_N(mounts, nmounts+1) < 0) { - virReportOOMError(); - goto cleanup; - } - if (!(mounts[nmounts] = strdup(mntent.mnt_dir))) { - virReportOOMError(); - goto cleanup; - } - nmounts++; - VIR_DEBUG("Grabbed %s", mntent.mnt_dir); - } - - if (mounts) - qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); - - ret = 0; -cleanup: - *mountsret = mounts; - *nmountsret = nmounts; - endmntent(procmnt); - return ret; -} - -static int lxcContainerUnmountSubtree(const char *prefix, - bool isOldRootFS) -{ - char **mounts = NULL; - size_t nmounts = 0; - size_t i; - int saveErrno; - const char *failedUmount = NULL; - int ret = -1; - - VIR_DEBUG("Unmount subtreee from %s", prefix); - - if (lxcContainerGetSubtree(prefix, &mounts, &nmounts) < 0) - goto cleanup; - for (i = 0 ; i < nmounts ; i++) { - VIR_DEBUG("Umount %s", mounts[i]); - if (umount(mounts[i]) < 0) { - char ebuf[1024]; - failedUmount = mounts[i]; - saveErrno = errno; - VIR_WARN("Failed to unmount '%s', trying to detach subtree '%s': %s", - failedUmount, mounts[nmounts-1], - virStrerror(errno, ebuf, sizeof(ebuf))); - break; - } - } - - if (failedUmount) { - /* This detaches the subtree */ - if (umount2(mounts[nmounts-1], MNT_DETACH) < 0) { - virReportSystemError(saveErrno, - _("Failed to unmount '%s' and could not detach subtree '%s'"), - failedUmount, mounts[nmounts-1]); - goto cleanup; - } - /* This unmounts the tmpfs on which the old root filesystem was hosted */ - if (isOldRootFS && - umount(mounts[nmounts-1]) < 0) { - virReportSystemError(saveErrno, - _("Failed to unmount '%s' and could not unmount old root '%s'"), - failedUmount, mounts[nmounts-1]); - goto cleanup; - } - } - - ret = 0; - -cleanup: - for (i = 0 ; i < nmounts ; i++) - VIR_FREE(mounts[i]); - VIR_FREE(mounts); - - return ret; -} - - struct lxcContainerCGroup { const char *dir; const char *linkDest; -- 1.8.1.4
On 04/08/2013 09:12 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If the user requests a mount for /run, this may hide any existing mounts that are lower down in /run. The result is that the container still sees the mounts in /proc/mounts, but cannot access them
sh-4.2# df df: '/run/user/501/gvfs': No such file or directory df: '/run/media/berrange/LIVE': No such file or directory df: '/run/media/berrange/SecureDiskA1': No such file or directory df: '/run/libvirt/lxc/sandbox': No such file or directory Filesystem 1K-blocks Used Available Use% Mounted on /dev/mapper/vg_t500wlan-lv_root 151476396 135390200 8384900 95% / tmpfs 1970888 3204 1967684 1% /run /dev/sda1 194241 155940 28061 85% /boot devfs 64 0 64 0% /dev tmpfs 64 0 64 0% /sys/fs/cgroup tmpfs 1970888 1200 1969688 1% /etc/libvirt-sandbox/scratch
Before mounting any filesystem at a particular location, we must recursively unmount anything at or below the target mount point
Makes sense.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 218 ++++++++++++++++++++++++------------------------ 1 file changed, 111 insertions(+), 107 deletions(-)
+static int lxcContainerGetSubtree(const char *prefix, + char ***mountsret, + size_t *nmountsret)
We aren't very consistent on the style: static int lxcContainerGetSubtree(...
+{ + FILE *procmnt; + struct mntent mntent; + char mntbuf[1024];
fixed-width buffer...
+ int ret = -1; + char **mounts = NULL; + size_t nmounts = 0; + + VIR_DEBUG("prefix=%s", prefix); + + *mountsret = NULL; + *nmountsret = 0; + + if (!(procmnt = setmntent("/proc/mounts", "r"))) { + virReportSystemError(errno, "%s", + _("Failed to read /proc/mounts")); + return -1; + } + + while (getmntent_r(procmnt, &mntent, mntbuf, sizeof(mntbuf)) != NULL) {
...and getmntent_r can return ERANGE if it is too small (where it is definitely feasible that the user could supply a mount point larger than 1024) [well, I assume an ERANGE failure, even though the man page is buggy for failing to mention errno values on failure, based on similarity to other functions that fail with ERANGE when given a too-small buffer]...
+ VIR_DEBUG("Got %s", mntent.mnt_dir); + if (!STRPREFIX(mntent.mnt_dir, prefix)) + continue; + + if (VIR_REALLOC_N(mounts, nmounts+1) < 0) {
Is VIR_EXPAND or VIR_RESIZE better than VIR_REALLOC_N?
+ virReportOOMError();
Another place that will conflict with OOM overhaul :)
+ goto cleanup; + } + if (!(mounts[nmounts] = strdup(mntent.mnt_dir))) { + virReportOOMError(); + goto cleanup; + } + nmounts++; + VIR_DEBUG("Grabbed %s", mntent.mnt_dir); + }
...but you aren't checking errno or allowing for the possibility of needing to enlarge the buffer.
+ + if (mounts) + qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); + + ret = 0; +cleanup: + *mountsret = mounts; + *nmountsret = nmounts; + endmntent(procmnt); + return ret; +} + -static int lxcContainerUnmountSubtree(const char *prefix, - bool isOldRootFS) -{
Looks like this patch is a bit of code shuffling coupled with some tweaks; would it be better if it were split into two parts so that the code shuffling is done with no semantic change, making it easier for the second patch to show only what changed? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake