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

Christian Brauner christianvanbrauner at gmail.com
Thu Oct 22 13:37:01 UTC 2015 

On Thu, Oct 22, 2015 at 01:13:35PM +0000, Serge Hallyn wrote:
> 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.
> 

I'll test that and get back with an updated patch. I *try* to think of something
smarter... Do you want me to avoid lxc_string_replace()?

> The need for unexpanded config is very unfortunate.
Well, we won't be able to work around it. Especially when starting an ephemeral
clone with lxc.mount.entry entries for overlay/aufs dir we need to update both
otherwise we'll end up either with a wrong config or with wrong entries in
lxc_conf->mount_list when we start it immediately after having cloned it.

(The only other option is to have users specify relative paths but (as you
argued) this might confuse them since these would be relative paths to the
containerdir and not the rootfs.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20151022/8e536723/attachment.sig>

More information about the lxc-devel mailing list