[lxc-devel] [PATCH v2] Update absolute paths for overlay and aufs mounts

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at gmail.com):
> On Tue, Oct 20, 2015 at 04:17:18PM +0000, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > > When using overlay and aufs mounts with lxc.mount.entry users have to specify
> > > absolute paths for upperdir and workdir which will then get created
> > > automatically by mount_entry_create_overlay_dirs() and
> > > mount_entry_create_aufs_dirs() in conf.c. When we clone a container with
> > > overlay or aufs lxc.mount.entry entries we need to update these absolute paths.
> > > In order to do this we add the function update_union_mount_entry_paths() in
> > > lxccontainer.c. The function updates the mounts in two locations:
> > > 
> > >         1) lxc_conf->mount_list
> > > 
> > > and
> > > 
> > >         2) lxc_conf->unexpanded_config
> > > 
> > > If we were to only update 2) we would end up with wrong upperdir and workdir
> > > mounts as the absolute paths would still point to the container that serves as
> > > the base for the clone. If we were to only update 1) we would end up with wrong
> > > upperdir and workdir lxc.mount.entry entries in the clone's config as the
> > > absolute paths in upperdir and workdir would still point to the container that
> > > serves as the base for the clone. Updating both will get the job done. Note,
> > > that an entry in lxc_conf->mount_list will only be updated if it is also found
> > > in the clones config. Mounts from other files are hence not updated. In short,
> > > automatic overlay and aufs mounts with lxc.mount.entry should only be specified
> > > in the containers own config.
> > > 
> > > NOTE: This function does not sanitize paths apart from removing trailing
> > > slashes. (So when a user specifies //home//someone/// it will be cleaned to
> > > //home//someone. This is the minimal path cleansing which is also done by
> > > lxc_container_new().) But the mount_entry_create_overlay_dirs() and
> > > mount_entry_create_aufs_dirs() functions both try to be extremely strict about
> > > when to create upperdirs and workdirs. They will only accept sanitized paths,
> > > i.e. they require /home/someone. I think this is a (safety) virtue and we
> > > should consider sanitizing paths in general. In short:
> > > update_union_mount_entry_paths() does update all absolute paths to the new
> > > container but mount_entry_create_overlay_dirs() and
> > > mount_entry_create_aufs_dirs() will still refuse to create upperdir and workdir
> > > when the updated path is unclean. This happens easily when e.g. a user calls
> > > lxc-clone -o OLD -n NEW -P //home//chb///.
> > > 
> > > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> > > ---
> > >  src/lxc/lxccontainer.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 132 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index 42e23e7..5ed7697 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -2894,6 +2894,130 @@ static int create_file_dirname(char *path, struct lxc_conf *conf)
> > >  	return ret;
> > >  }
> > >  
> > > +/* When we clone a container with overlay or aufs lxc.mount.entry entries we
> > > +*  need to update these absolute paths. In order to do this we add the function
> > > +*  update_union_mount_entry_paths() in lxccontainer.c. The function operates on
> > > +*  c->lxc_conf->unexpanded_config instead of the intuitively plausible
> > > +*  c->lxc_conf->mount_list because the latter also contains mounts from other
> > > +*  files as well as generic mounts. */
> > > +static int update_union_mount_entry_paths(struct lxc_conf *lxc_conf,
> > > +					  const char *lxc_path,
> > > +					  const char *lxc_name,
> > > +					  const char *newpath,
> > > +					  const char *newname)
> > > +{
> > > +	char new_upper[MAXPATHLEN];
> > > +	char new_work[MAXPATHLEN];
> > > +	char old_upper[MAXPATHLEN];
> > > +	char old_work[MAXPATHLEN];
> > > +	char *cleanpath = NULL;
> > > +	char *mnt_entry = NULL;
> > > +	char *new_mnt_entry = NULL;
> > > +	char *new_unexpanded_config = NULL;
> > > +	char *tmp_mnt_entry = NULL;
> > > +	char *tmp_unexpanded_config = NULL;
> > > +	char *tmp = NULL;
> > > +	int ret = 0;
> > > +	size_t len = 0;
> > > +	struct lxc_list *iterator;
> > > +
> > > +	cleanpath = strdup(newpath);
> > > +	if (!cleanpath)
> > > +		goto err;
> > > +
> > > +	remove_trailing_slashes(cleanpath);
> > > +
> > > +	ret = snprintf(old_work, MAXPATHLEN, "workdir=%s/%s", lxc_path, lxc_name);
> > > +	if (ret < 0 || ret >= MAXPATHLEN)
> > > +		goto err;
> > > +
> > > +	ret = snprintf(new_work, MAXPATHLEN, "workdir=%s/%s", cleanpath, newname);
> > > +	if (ret < 0 || ret >= MAXPATHLEN)
> > > +		goto err;
> > > +
> > > +	lxc_list_for_each(iterator, &lxc_conf->mount_list) {
> > 
> > put
> > 
> > 		char *tmp = NULL, new_mnt_entry = NULL;
> > 		char *mnt_entry;
> > 
> > here (and remove from above etc).
> > 
> > > +		mnt_entry = iterator->elem;
> > > +
> > > +		if (strstr(mnt_entry, "overlay"))
> > > +			tmp = "upperdir";
> > > +		else if (strstr(mnt_entry, "aufs"))
> > > +			tmp = "br";
> > > +
> > > +		if (!tmp)
> > > +			continue;
> > > +
> > > +		if (strstr(lxc_conf->unexpanded_config, mnt_entry)) {
> > > +			ret = snprintf(old_upper, MAXPATHLEN, "%s=%s/%s", tmp, lxc_path, lxc_name);
> > > +			if (ret < 0 || ret >= MAXPATHLEN)
> > > +				goto err;
> > 
> > Alternate suggestion.  How about using clear_unexp_config_line() to remove all
> > lxc.mount.entry lines from unexpanded config first, then run through this loop
> > and simply add the new ones?
> > 
> > The lxc_string_replace approach feels problematic.
> 
> I thought so too, but clear_unexp_config_line() will not just clear mount
> entries from the containers config but also entries from files like common.conf
> which are also present in lxc_conf->mount_list e.g.:

Hm, might be the other way around, but yeah we can't do that.

But if the config file has say a commented version of the line before
the real line, you'll update only the comment right?

So can you do lik eclear_unexp_config_line() does and make sure you
are looking for only lines beginning with "lxc.mount.entry += +"
and then have the mnt_entry?  I guess that should do.

The need for unexpanded config is very unfortunate.