On Thu, Oct 15, 2015 at 08:32:25PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > The mount_entry_create_*_dirs() functions currently assume that the rootfs
> > of
> > the container is actually named "rootfs". This has the consequence that
> >
> > del = strstr(lxcpath, "/rootfs");
> > if (!del) {
> > free(lxcpath);
> > lxc_free_array((void **)opts, free);
> > return -1;
> > }
> > *del = '\0';
> >
> > will return NULL when the rootfs of a container is not actually named
> > "rootfs".
> > This means the we return -1 and do not create the necessary upperdir/workdir
> > directories required for the overlay/aufs mount to work. Hence, let's not
> > make
> > that assumption. We now pass lxc_path and lxc_name to
> > mount_entry_create_*_dirs() and create the path directly. To prevent
> > failure we
> > also have mount_entry_create_*_dirs() check that lxc_name and lxc_path are
> > not
> > empty when they are passed in.
> >
> > Signed-off-by: Christian Brauner <christianvanbrau...@gmail.com>
>
> Yeah this was bugging me a few years ago. Overall the patch looks fine
> to me, I'm running a full testsuite to ease my mind about it. Will ack
> after that passes and I look over it again.
We should also consider parsing path->rootfs when the container is an overlay or
aufs backed container. Because in this case the right hand side of the check:
if ((strncmp(upperdir, lxcpath, dirlen) == 0) &&
(strncmp(upperdir, rootfs->path, rootfslen) != 0))
will be trivially true since path->rootfs will e.g. be overlayfs:/path1:path2.
Parsing path->rootfs to extract path2 before doing the second check would be
safer... Thoughts?
>
> > ---
> > src/lxc/conf.c | 91
> > ++++++++++++++++++++++++++--------------------------------
> > 1 file changed, 41 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> > index 0e3421b..16a62f8 100644
> > --- a/src/lxc/conf.c
> > +++ b/src/lxc/conf.c
> > @@ -1816,20 +1816,22 @@ static void cull_mntent_opt(struct mntent *mntent)
> > }
> >
> > static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
> > - const struct lxc_rootfs *rootfs)
> > + const struct lxc_rootfs *rootfs,
> > + const char *lxc_name,
> > + const char *lxc_path)
> > {
> > - char *del = NULL;
> > - char *lxcpath = NULL;
> > + char lxcpath[MAXPATHLEN];
> > char *upperdir = NULL;
> > char *workdir = NULL;
> > char **opts = NULL;
> > + int ret = 0;
> > size_t arrlen = 0;
> > size_t dirlen = 0;
> > size_t i;
> > size_t len = 0;
> > size_t rootfslen = 0;
> >
> > - if (!rootfs->path)
> > + if (!rootfs->path || !lxc_name || !lxc_path)
> > return -1;
> >
> > opts = lxc_string_split(mntent->mnt_opts, ',');
> > @@ -1845,19 +1847,11 @@ static int mount_entry_create_overlay_dirs(const
> > struct mntent *mntent,
> > workdir = opts[i] + len;
> > }
> >
> > - lxcpath = strdup(rootfs->path);
> > - if (!lxcpath) {
> > - lxc_free_array((void **)opts, free);
> > - return -1;
> > - }
> > -
> > - del = strstr(lxcpath, "/rootfs");
> > - if (!del) {
> > - free(lxcpath);
> > + ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, lxc_name);
> > + if (ret < 0 || ret >= MAXPATHLEN) {
> > lxc_free_array((void **)opts, free);
> > return -1;
> > }
> > - *del = '\0';
> >
> > dirlen = strlen(lxcpath);
> > rootfslen = strlen(rootfs->path);
> > @@ -1877,25 +1871,26 @@ static int mount_entry_create_overlay_dirs(const
> > struct mntent *mntent,
> > WARN("Failed to create workdir");
> > }
> >
> > - free(lxcpath);
> > lxc_free_array((void **)opts, free);
> > return 0;
> > }
> >
> > static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
> > - const struct lxc_rootfs *rootfs)
> > + const struct lxc_rootfs *rootfs,
> > + const char *lxc_name,
> > + const char *lxc_path)
> > {
> > - char *del = NULL;
> > - char *lxcpath = NULL;
> > + char lxcpath[MAXPATHLEN];
> > char *scratch = NULL;
> > char *tmp = NULL;
> > char *upperdir = NULL;
> > char **opts = NULL;
> > + int ret = 0;
> > size_t arrlen = 0;
> > size_t i;
> > size_t len = 0;
> >
> > - if (!rootfs->path)
> > + if (!rootfs->path || !lxc_name || !lxc_path)
> > return -1;
> >
> > opts = lxc_string_split(mntent->mnt_opts, ',');
> > @@ -1919,19 +1914,11 @@ static int mount_entry_create_aufs_dirs(const
> > struct mntent *mntent,
> > return -1;
> > }
> >
> > - lxcpath = strdup(rootfs->path);
> > - if (!lxcpath) {
> > - lxc_free_array((void **)opts, free);
> > - return -1;
> > - }
> > -
> > - del = strstr(lxcpath, "/rootfs");
> > - if (!del) {
> > - free(lxcpath);
> > + ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, lxc_name);
> > + if (ret < 0 || ret >= MAXPATHLEN) {
> > lxc_free_array((void **)opts, free);
> > return -1;
> > }
> > - *del = '\0';
> >
> > /* We neither allow users to create upperdirs outside the containerdir
> > * nor inside the rootfs. The latter might be debatable. */
> > @@ -1940,23 +1927,24 @@ static int mount_entry_create_aufs_dirs(const
> > struct mntent *mntent,
> > WARN("Failed to create upperdir");
> > }
> >
> > - free(lxcpath);
> > lxc_free_array((void **)opts, free);
> > return 0;
> > }
> >
> > +
> > static int mount_entry_create_dir_file(const struct mntent *mntent,
> > - const char* path, const struct
> > lxc_rootfs *rootfs)
> > + const char* path, const struct
> > lxc_rootfs *rootfs,
> > + const char *lxc_name, const char
> > *lxc_path)
> > {
> > char *pathdirname = NULL;
> > int ret = 0;
> > FILE *pathfile = NULL;
> >
> > if (strncmp(mntent->mnt_type, "overlay", 7) == 0) {
> > - if (mount_entry_create_overlay_dirs(mntent, rootfs) < 0)
> > + if (mount_entry_create_overlay_dirs(mntent, rootfs, lxc_name,
> > lxc_path) < 0)
> > return -1;
> > } else if (strncmp(mntent->mnt_type, "aufs", 4) == 0) {
> > - if (mount_entry_create_aufs_dirs(mntent, rootfs) < 0)
> > + if (mount_entry_create_aufs_dirs(mntent, rootfs, lxc_name,
> > lxc_path) < 0)
> > return -1;
> > }
> >
> > @@ -1986,14 +1974,15 @@ static int mount_entry_create_dir_file(const struct
> > mntent *mntent,
> > }
> >
> > static inline int mount_entry_on_generic(struct mntent *mntent,
> > - const char* path, const struct lxc_rootfs *rootfs)
> > + const char* path, const struct lxc_rootfs *rootfs,
> > + const char *lxc_name, const char *lxc_path)
> > {
> > unsigned long mntflags;
> > char *mntdata;
> > int ret;
> > bool optional = hasmntopt(mntent, "optional") != NULL;
> >
> > - ret = mount_entry_create_dir_file(mntent, path, rootfs);
> > + ret = mount_entry_create_dir_file(mntent, path, rootfs, lxc_name,
> > lxc_path);
> >
> > if (ret < 0)
> > return optional ? 0 : -1;
> > @@ -2015,12 +2004,13 @@ static inline int mount_entry_on_generic(struct
> > mntent *mntent,
> >
> > static inline int mount_entry_on_systemfs(struct mntent *mntent)
> > {
> > - return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL);
> > + return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL, NULL, NULL);
> > }
> >
> > static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
> > const struct lxc_rootfs *rootfs,
> > - const char *lxc_name)
> > + const char *lxc_name,
> > + const char *lxc_path)
> > {
> > char *aux;
> > char path[MAXPATHLEN];
> > @@ -2062,11 +2052,13 @@ skipabs:
> > return -1;
> > }
> >
> > - return mount_entry_on_generic(mntent, path, rootfs);
> > + return mount_entry_on_generic(mntent, path, rootfs, lxc_name, lxc_path);
> > }
> >
> > static int mount_entry_on_relative_rootfs(struct mntent *mntent,
> > - const struct lxc_rootfs *rootfs)
> > + const struct lxc_rootfs *rootfs,
> > + const char *lxc_name,
> > + const char *lxc_path)
> > {
> > char path[MAXPATHLEN];
> > int ret;
> > @@ -2078,11 +2070,11 @@ static int mount_entry_on_relative_rootfs(struct
> > mntent *mntent,
> > return -1;
> > }
> >
> > - return mount_entry_on_generic(mntent, path, rootfs);
> > + return mount_entry_on_generic(mntent, path, rootfs, lxc_name, lxc_path);
> > }
> >
> > static int mount_file_entries(const struct lxc_rootfs *rootfs, FILE *file,
> > - const char *lxc_name)
> > + const char *lxc_name, const char *lxc_path)
> > {
> > struct mntent mntent;
> > char buf[4096];
> > @@ -2098,13 +2090,12 @@ static int mount_file_entries(const struct
> > lxc_rootfs *rootfs, FILE *file,
> >
> > /* We have a separate root, mounts are relative to it */
> > if (mntent.mnt_dir[0] != '/') {
> > - if (mount_entry_on_relative_rootfs(&mntent,
> > - rootfs))
> > + if (mount_entry_on_relative_rootfs(&mntent, rootfs,
> > lxc_name, lxc_path))
> > goto out;
> > continue;
> > }
> >
> > - if (mount_entry_on_absolute_rootfs(&mntent, rootfs, lxc_name))
> > + if (mount_entry_on_absolute_rootfs(&mntent, rootfs, lxc_name,
> > lxc_path))
> > goto out;
> > }
> >
> > @@ -2116,7 +2107,7 @@ out:
> > }
> >
> > static int setup_mount(const struct lxc_rootfs *rootfs, const char *fstab,
> > - const char *lxc_name)
> > + const char *lxc_name, const char *lxc_path)
> > {
> > FILE *file;
> > int ret;
> > @@ -2130,7 +2121,7 @@ static int setup_mount(const struct lxc_rootfs
> > *rootfs, const char *fstab,
> > return -1;
> > }
> >
> > - ret = mount_file_entries(rootfs, file, lxc_name);
> > + ret = mount_file_entries(rootfs, file, lxc_name, lxc_path);
> >
> > endmntent(file);
> > return ret;
> > @@ -2158,7 +2149,7 @@ FILE *write_mount_file(struct lxc_list *mount)
> > }
> >
> > static int setup_mount_entries(const struct lxc_rootfs *rootfs, struct
> > lxc_list *mount,
> > - const char *lxc_name)
> > + const char *lxc_name, const char *lxc_path)
> > {
> > FILE *file;
> > int ret;
> > @@ -2167,7 +2158,7 @@ static int setup_mount_entries(const struct
> > lxc_rootfs *rootfs, struct lxc_list
> > if (!file)
> > return -1;
> >
> > - ret = mount_file_entries(rootfs, file, lxc_name);
> > + ret = mount_file_entries(rootfs, file, lxc_name, lxc_path);
> >
> > fclose(file);
> > return ret;
> > @@ -3891,12 +3882,12 @@ int lxc_setup(struct lxc_handler *handler)
> > return -1;
> > }
> >
> > - if (setup_mount(&lxc_conf->rootfs, lxc_conf->fstab, name)) {
> > + if (setup_mount(&lxc_conf->rootfs, lxc_conf->fstab, name, lxcpath)) {
> > ERROR("failed to setup the mounts for '%s'", name);
> > return -1;
> > }
> >
> > - if (!lxc_list_empty(&lxc_conf->mount_list) &&
> > setup_mount_entries(&lxc_conf->rootfs, &lxc_conf->mount_list, name)) {
> > + if (!lxc_list_empty(&lxc_conf->mount_list) &&
> > setup_mount_entries(&lxc_conf->rootfs, &lxc_conf->mount_list, name,
> > lxcpath)) {
> > ERROR("failed to setup the mount entries for '%s'", name);
> > return -1;
> > }
> > --
> > 2.6.1
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel@lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
