-----Original Message----- From: Gao feng [mailto:gaofeng@cn.fujitsu.com] Sent: Friday, October 04, 2013 12:55 PM To: Purcareata Bogdan-B43198 Cc: libvir-list(a)redhat.com Subject: Re: [libvirt] [PATCH v2] LXC: Detect fs support. Mount only supported filesystems On 10/02/2013 10:05 PM, Bogdan Purcareata wrote: > Kept ((access(dstpath, R_OK) < 0) || (!lxcCheckFSSupport(mnt->type))) > when determining support for the mount. Even if the filesystem type is > supported, there is still a chance to fail when building the dstpath > (virFileMakePath). If that call fails, starting the container will fail. > Specifically encountered this problem for securityfs, as I was unable > to mkdir /sys/kernel/security. > > Signed-off-by: Bogdan Purcareata <bogdan.purcareata(a)freescale.com&gt; > --- > src/lxc/lxc_container.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 989e920..496443d 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -509,6 +509,67 @@ static int lxcContainerChildMountSort(const void *a, const void *b) > # define MS_SLAVE (1<<19) > #endif > > +/* > + * This function attempts to detect kernel support > + * for a specific filesystem type. This is done by > + * inspecting /proc/filesystems. > + */ > +static int lxcCheckFSSupport(const char *fs_type) > +{ > + FILE *fp = NULL; > + int ret = -1; > + const char *fslist = "/proc/filesystems"; > + char *line = NULL; > + char *type; > + size_t n; > + > + /* there should be no problem mounting an entry > + * with NULL fs type, hence NULL fs types are > + * supported */ > + if (!fs_type) { > + ret = 1; > + goto out; > + } > + > + VIR_DEBUG("Checking kernel support for %s in %s", fs_type, fslist); > + > + if (!(fp = fopen(fslist, "r"))) { I don't know if we can open /proc/filesystems successfully here if container shares root directory with host, since the /proc filesystem has been unmounted in lxcContainerUnmountForSharedRoot.

> + virReportSystemError(errno, > + _("Unable to read %s"), > + fslist); > + goto out; > + } > + > + while(getline(&line, &n, fp) > 0) { > + type = strstr(line, fs_type); > + > + if (!type) > + continue; > + > + if (!strncmp(type, fs_type, strlen(type))) { The strncmp() function compares the only first (at most) n bytes of s1 and s2. please use STREQ here.

> + ret = 1; > + goto cleanup; > + } > + } > + > + if (ferror(fp)) { > + virReportSystemError(errno, > + _("Error reading line from %s"), > + fslist); > + goto cleanup; > + } > + > + VIR_DEBUG("No kernel support for %s", fs_type); > + > + ret = 0; > + You set ret to 0 here, so the return value 0 means this filesystem is unsupported by kernel, right? what the meaning of return value -1? you return -1 when ferror(fp) is true.

> +cleanup: > + VIR_FREE(line); > + VIR_FORCE_FCLOSE(fp); > +out: > + return ret; > +} > + > static int lxcContainerGetSubtree(const char *prefix, > char ***mountsret, > size_t *nmountsret) > @@ -789,17 +850,23 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > for (i = 0; i < ARRAY_CARDINALITY(lxcBasicMounts); i++) { > virLXCBasicMountInfo const *mnt = &lxcBasicMounts[i]; > const char *srcpath = NULL; > + const char *dstpath = NULL; > > VIR_DEBUG("Processing %s -> %s", > mnt->src, mnt->dst); > > srcpath = mnt->src; > + dstpath = mnt->dst; > > /* Skip if mount doesn't exist in source */ > if ((srcpath[0] == '/') && > (access(srcpath, R_OK) < 0)) > continue; > > + if ((access(dstpath, R_OK) < 0) || /* mount is not present on host */ > + (!lxcCheckFSSupport(mnt->type))) /* no fs support in kernel */ > + continue; > + The access is in the incorrect place, it should be called after we create mnt- >dst. so Move this check after virFileMakePath(mnt->dst).

> #if WITH_SELINUX > if (STREQ(mnt->src, SELINUX_MOUNT) && > (!is_selinux_enabled() || userns_enabled)) > Thanks

On 10/04/2013 07:33 PM, Purcareata Bogdan-B43198 wrote: >>> +/* >>> + * This function attempts to detect kernel support >>> + * for a specific filesystem type. This is done by >>> + * inspecting /proc/filesystems. >>> + */ >>> +static int lxcCheckFSSupport(const char *fs_type) >>> +{ >>> + FILE *fp = NULL; >>> + int ret = -1; >>> + const char *fslist = "/proc/filesystems"; >>> + char *line = NULL; >>> + char *type; >>> + size_t n; >>> + >>> + /* there should be no problem mounting an entry >>> + * with NULL fs type, hence NULL fs types are >>> + * supported */ >>> + if (!fs_type) { >>> + ret = 1; >>> + goto out; >>> + } >>> + >>> + VIR_DEBUG("Checking kernel support for %s in %s", fs_type, fslist); >>> + >>> + if (!(fp = fopen(fslist, "r"))) { >> >> I don't know if we can open /proc/filesystems successfully here if container >> shares >> root directory with host, since the /proc filesystem has been unmounted in >> lxcContainerUnmountForSharedRoot. > > Right. I just noticed the search for "proc" fails, since /proc/filesystem requires procfs to be mounted. (Un)fortunately, my handling of lxcCheckFSSupport() bypassed this error, and mounted procfs anyways. I will update the code with a proper handle for the error code. I just don't see how I can handle all filesystem entries in an uniform manner, since each one is so special. > >> So save the supported filesystem list before we unmount the proc filesystem, and in lxcCheckFSSupport use this list to check if the filesystem is supported by kernel. btw it's better to return the error of fopen to user. >>> + virReportSystemError(errno, >>> + _("Unable to read %s"), >>> + fslist); >>> + goto out; >>> + } >>> + >>> + while(getline(&line, &n, fp) > 0) { >>> + type = strstr(line, fs_type); >>> + >>> + if (!type) >>> + continue; >>> + >>> + if (!strncmp(type, fs_type, strlen(type))) { >> >> The strncmp() function compares the only first (at most) n bytes of s1 and s2. >> please use STREQ here. > > Thanks, I will update. > >> >>> + ret = 1; >>> + goto cleanup; >>> + } >>> + } >>> + >>> + if (ferror(fp)) { >>> + virReportSystemError(errno, >>> + _("Error reading line from %s"), >>> + fslist); >>> + goto cleanup; >>> + } >>> + >>> + VIR_DEBUG("No kernel support for %s", fs_type); >>> + >>> + ret = 0; >>> + >> >> You set ret to 0 here, so the return value 0 means this filesystem >> is unsupported by kernel, right? what the meaning of return value -1? >> >> you return -1 when ferror(fp) is true. > > So I thought it would be like this: > - -1 - error encountered > - 0 - no error, no kernel support for the filesystem > - 1 - no error, kernel support present > >> >>> +cleanup: >>> + VIR_FREE(line); >>> + VIR_FORCE_FCLOSE(fp); >>> +out: >>> + return ret; >>> +} >>> + >>> static int lxcContainerGetSubtree(const char *prefix, >>> char ***mountsret, >>> size_t *nmountsret) >>> @@ -789,17 +850,23 @@ static int lxcContainerMountBasicFS(bool >> userns_enabled) >>> for (i = 0; i < ARRAY_CARDINALITY(lxcBasicMounts); i++) { >>> virLXCBasicMountInfo const *mnt = &lxcBasicMounts[i]; >>> const char *srcpath = NULL; >>> + const char *dstpath = NULL; >>> >>> VIR_DEBUG("Processing %s -> %s", >>> mnt->src, mnt->dst); >>> >>> srcpath = mnt->src; >>> + dstpath = mnt->dst; >>> >>> /* Skip if mount doesn't exist in source */ >>> if ((srcpath[0] == '/') && >>> (access(srcpath, R_OK) < 0)) >>> continue; >>> >>> + if ((access(dstpath, R_OK) < 0) || /* mount is not present on host */ >>> + (!lxcCheckFSSupport(mnt->type))) /* no fs support in kernel */ >>> + continue; >>> + >> >> The access is in the incorrect place, it should be called after we create mnt- >>> dst. >> so Move this check after virFileMakePath(mnt->dst). > > My specific problem was that mounting security failed even before reaching the actual mount syscall. > > It failed when doing virFileMakePath("/sys/kernel/securityfs"), because /sys is previously mounted read only (I realized this just now). > > root@p4080ds:/sys/kernel# mkdir securityfs > mkdir: cannot create directory 'securityfs': No such file or directory > I don't know how this occurred, since the directory securityfs is created when you mount sysfs. Actually virFileMakePath will not create securityfs directory since it already exists.