[libvirt] [PATCH v2] LXC: Detect fs support. Mount only supported filesystems

[Purcareata Bogdan-B43198]

> -----Original Message-----
> From: Gao feng [mailto:gaofeng at cn.fujitsu.com]
> Sent: Friday, October 04, 2013 12:55 PM
> To: Purcareata Bogdan-B43198
> Cc: libvir-list at 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 at freescale.com>
> > ---
> >  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.

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.

> 
> > +        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 at p4080ds:/sys/kernel# mkdir securityfs
mkdir: cannot create directory 'securityfs': No such file or directory

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