[lxc-devel] [PATCH v4] Parse rootfs->path

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at 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 <christianvanbrauner at 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.  (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)

-serge