Quoting Christian Brauner (christianvanbrau...@gmail.com): > On Thu, Oct 22, 2015 at 02:59:28AM +0000, Serge Hallyn wrote: > > Quoting Christian Brauner (christianvanbrau...@gmail.com): > > > On Tue, Oct 20, 2015 at 09:21:48PM +0200, Christian Brauner wrote: > > > > The mount_entry_overlay_dirs() and mount_entry_aufs_dirs() functions > > > > create > > > > workdirs and upperdirs for overlay and aufs lxc.mount.entry entries. > > > > They try > > > > to make sure that the workdirs and upperdirs can only be created under > > > > the > > > > containerdir (e.g. /path/to/the/container/CONTAINERNAME). In order to > > > > do this > > > > the right hand side of > > > > > > > > if ((strncmp(upperdir, lxcpath, dirlen) == 0) && > > > > (strncmp(upperdir, rootfs->path, rootfslen) != 0)) > > > > > > > > was thought to check if the rootfs->path is not present in the workdir > > > > and > > > > upperdir mount options. But the current check is bogus since it will be > > > > trivially true whenever the container is a block-dev or overlay or aufs > > > > backed > > > > since the rootfs->path will then have a form like e.g. > > > > > > > > overlayfs:/some/path:/some/other/path > > > > > > > > This patch adds the function ovl_get_rootfs_dir() which parses > > > > rootfs->path by > > > > searching backwards for the first occurrence of the delimiter pair > > > > ":/". We do > > > > not simply search for ":" since it might be used in path names. If ":/" > > > > is not > > > > found we assume the container is directory backed and simply return > > > > strdup(rootfs->path). > > > > > > > > Signed-off-by: Christian Brauner <christianvanbrau...@gmail.com> > > > > --- > > > > src/lxc/conf.c | 101 > > > > +++++++++++++++++++++++++++++++++++++++++++-------------- > > > > 1 file changed, 76 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/src/lxc/conf.c b/src/lxc/conf.c > > > > index 16a62f8..fc072f7 100644 > > > > --- a/src/lxc/conf.c > > > > +++ b/src/lxc/conf.c > > > > @@ -1815,12 +1815,51 @@ static void cull_mntent_opt(struct mntent > > > > *mntent) > > > > } > > > > } > > > > > > > > +static char *ovl_get_rootfs_dir(const char *rootfs_path, size_t > > > > *rootfslen) > > > > +{ > > > > + char *rootfsdir = NULL; > > > > + char *s1 = NULL; > > > > + char *s2 = NULL; > > > > + char *s3 = NULL; > > > > + size_t s3len = 0; > > > > + > > > > + if (!rootfs_path || !rootfslen) > > > > + return NULL; > > > > + > > > > + s1 = strdup(rootfs_path); > > > > + if (!s1) > > > > + return NULL; > > > > + > > > > + if ((s2 = strstr(s1, ":/"))) { > > > > + s2 = s2 + 1; > > > > + if ((s3 = strstr(s2, ":/"))) { > > > > + s3len = strlen(s3); > > > > + s2[strlen(s2) - s3len] = '\0'; > > > > + } > > > > + rootfsdir = strdup(s2); > > > > + if (!rootfsdir) { > > > > + free(s1); > > > > + return NULL; > > > > + } > > > > + } > > > > + > > > > + if (!rootfsdir) > > > > + rootfsdir = s1; > > > > + else > > > > + free(s1); > > > > + > > > > + *rootfslen = strlen(rootfsdir); > > > > + > > > > + return rootfsdir; > > > > +} > > > > + > > > > static int mount_entry_create_overlay_dirs(const struct mntent *mntent, > > > > const struct lxc_rootfs > > > > *rootfs, > > > > const char *lxc_name, > > > > const char *lxc_path) > > > > { > > > > char lxcpath[MAXPATHLEN]; > > > > + char *rootfsdir = NULL; > > > > char *upperdir = NULL; > > > > char *workdir = NULL; > > > > char **opts = NULL; > > > > @@ -1832,13 +1871,13 @@ static int > > > > mount_entry_create_overlay_dirs(const struct mntent *mntent, > > > > size_t rootfslen = 0; > > > > > > > > if (!rootfs->path || !lxc_name || !lxc_path) > > > > - return -1; > > > > + goto err; > > > > > > > > opts = lxc_string_split(mntent->mnt_opts, ','); > > > > if (opts) > > > > arrlen = lxc_array_len((void **)opts); > > > > else > > > > - return -1; > > > > + goto err; > > > > > > > > for (i = 0; i < arrlen; i++) { > > > > if (strstr(opts[i], "upperdir=") && (strlen(opts[i]) > > > > > (len = strlen("upperdir=")))) > > > > @@ -1848,31 +1887,37 @@ static int > > > > mount_entry_create_overlay_dirs(const struct mntent *mntent, > > > > } > > > > > > > > ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, > > > > lxc_name); > > > > - if (ret < 0 || ret >= MAXPATHLEN) { > > > > - lxc_free_array((void **)opts, free); > > > > - return -1; > > > > - } > > > > + if (ret < 0 || ret >= MAXPATHLEN) > > > > + goto err; > > > > + > > > > + rootfsdir = ovl_get_rootfs_dir(rootfs->path, &rootfslen); > > > > + if (!rootfsdir) > > > > + goto err; > > > > > > > > dirlen = strlen(lxcpath); > > > > - rootfslen = strlen(rootfs->path); > > > > > > > > /* We neither allow users to create upperdirs and workdirs > > > > outside the > > > > * containerdir nor inside the rootfs. The latter might be > > > > debatable. */ > > > > if (upperdir) > > > > - if ((strncmp(upperdir, lxcpath, dirlen) == 0) && > > > > (strncmp(upperdir, rootfs->path, rootfslen) != 0)) > > > > + if ((strncmp(upperdir, lxcpath, dirlen) == 0) && > > > > (strncmp(upperdir, rootfsdir, rootfslen) != 0)) > > > > if (mkdir_p(upperdir, 0755) < 0) { > > > > WARN("Failed to create upperdir"); > > > > } > > > > > > We should also consider putting > > > > > > return -1; > > > > > > here when the check fails. Consider a container named AA which has an > > > lxc.mount.entry for an overlay mount: > > > > > > lxc.mount.entry = /lowerdir dest overlay > > > lowerdir=/lowerdir,upperdir=/path/to/BB/delta,workdir=/path/to/BB/workdir,create=dir > > > > > > whose upperdir and workdir refer to another container BB. Both, upperdir > > > and > > > workdir already exist under BB's containerdir. If we do not return -1 > > > when the > > > check fails the mount will succeed and we will write changes to > > > workdir/upperdir > > > of BB. That should not happen. Once you let me know that you're fine with > > > the > > > reimplementation of ovl_get_rootfs_dir() I would like to change that too > > > before > > > you ack it. > > > > Sorry, I just don't know who uses this or how they use it. The example you > > show could be problematic, but enforcing that the upperdir be in the > > container's > > lxcdir could annoy some people. > > > > -serge > > Hm, just to be clear, I didn't mean that we return error when no upperdir or > workdir is given, just when one is given but it fails the pre mkdir_p() check. > In code: > > if (upperdir) > if ((strncmp(upperdir, lxcpath, dirlen) == 0) && > (strncmp(upperdir, rootfsdir, rootfslen) != 0)) { > if (mkdir_p(upperdir, 0755) < 0) { > WARN("Failed to create upperdir"); > } > } else { > return -1; > } > > if (workdir) > if ((strncmp(workdir, lxcpath, dirlen) == 0) && > (strncmp(workdir, rootfsdir, rootfslen) != 0)) { > if (mkdir_p(workdir, 0755) < 0) { > WARN("Failed to create workdir"); > } > } else { > return -1; > } > > Your point about that this might annoy some people still stands though. But I > think not returning -1 or at least some informative INFO() where the upperdir > actually is located in this case could be a security issue.
Ok, might be worth asking in #lxcontainers whether this would get in anyone's way. If not, just do it and back it out if anyone complains. > > (For workdir that does not apply, in fact I'm > > tempted to say we should just auto-choose a random workdir if none is given) > > We can do this. I will need that function for lxc-copy anyway. What do you > think about something along the lines of: Cursory glance it looks fine, though just using mkdtemp() should also suffice, right? > char *generate_random_name(const char *name, const char *path, const char > *sep) > { > char testpath[MAXPATHLEN]; > char tmp[MAXPATHLEN]; > char *randname = NULL; > int ret = 0; > unsigned int seed; > int suffix = 0; > > do { > #ifndef HAVE_RAND_R > seed = randseed(true); > #endif > > #ifdef HAVE_RAND_R > seed = randseed(false); > suffix = rand_r(&seed); > #else > suffix = rand(); > #endif > > if (name) > ret = snprintf(tmp, MAXPATHLEN, "%s%s%x", name, sep, > suffix); > else > ret = snprintf(tmp, MAXPATHLEN, "%s%x", sep, suffix); > if (ret < 0 || ret >= MAXPATHLEN) { > return NULL; > } > > ret = snprintf(testpath, MAXPATHLEN, "%s/%s", path, tmp); > if (ret < 0 || ret >= MAXPATHLEN) { > return NULL; > } > } while (dir_exists(testpath)); > > randname = strdup(tmp); > if (!randname) > return NULL; > > return randname; > } > _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel