[lxc-devel] [PATCH v2] Update absolute paths for overlay and aufs mounts
Christian Brauner
christianvanbrauner at gmail.com
Tue Oct 20 22:26:14 UTC 2015
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.:
lxc.mount.entry = /sys/fs/fuse/connections sys/fs/fuse/connections none bind,optional 0 0
We need to preserve the old ones somehow.
>
> > + ret = snprintf(new_upper, MAXPATHLEN, "%s=%s/%s", tmp, cleanpath, newname);
> > + if (ret < 0 || ret >= MAXPATHLEN)
> > + goto err;
> > +
> > + if (strstr(mnt_entry, old_upper)) {
> > + tmp_mnt_entry = lxc_string_replace(old_upper, new_upper, mnt_entry);
> > + tmp_unexpanded_config = lxc_string_replace(old_upper, new_upper, lxc_conf->unexpanded_config);
> > + }
> > +
> > + if (strstr(mnt_entry, old_work)) {
> > + if (tmp_mnt_entry)
> > + new_mnt_entry = lxc_string_replace(old_work, new_work, tmp_mnt_entry);
> > + if (tmp_unexpanded_config)
> > + new_unexpanded_config = lxc_string_replace(old_work, new_work, tmp_unexpanded_config);
> > + }
> > +
> > + if (new_mnt_entry) {
> > + free(iterator->elem);
> > + iterator->elem = strdup(new_mnt_entry);
> > + } else if (tmp_mnt_entry) {
> > + free(iterator->elem);
> > + iterator->elem = strdup(tmp_mnt_entry);
> > + }
> > +
> > + if (new_unexpanded_config) {
> > + free(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_config = strdup(new_unexpanded_config);
> > + } else if (tmp_unexpanded_config) {
> > + free(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_config = strdup(tmp_unexpanded_config);
> > + }
> > +
> > + if ((new_unexpanded_config || tmp_unexpanded_config) && lxc_conf->unexpanded_config) {
> > + len = strlen(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_len = len;
> > + lxc_conf->unexpanded_alloced = len + 1;
> > + }
> > +
> > + free(new_mnt_entry);
> > + free(new_unexpanded_config);
> > + free(tmp_mnt_entry);
> > + free(tmp_unexpanded_config);
>
> In general, when you have to free all the same things as you do in the 'out'
> label, it's better to keep a function return variable which stays -1, then
> below where you do 'return 0' do 'ret = 0' instead and fall through the label.
>
> > + mnt_entry = NULL;
> > + new_mnt_entry = NULL;
> > + new_unexpanded_config = NULL;
> > + tmp_mnt_entry = NULL;
> > + tmp_unexpanded_config = NULL;
> > + tmp = NULL;
> > +
> > + if (!iterator->elem || !lxc_conf->unexpanded_config)
> > + goto err;
> > + }
> > + }
> > + free(cleanpath);
> > + return 0;
> > +
> > +err:
> > + free(cleanpath);
> > + free(new_mnt_entry);
> > + free(new_unexpanded_config);
> > + free(tmp_mnt_entry);
> > + free(tmp_unexpanded_config);
> > + INFO("%s", "Failed to update config file");
> > + return -1;
> > +}
> > +
> > static struct lxc_container *do_lxcapi_clone(struct lxc_container *c, const char *newname,
> > const char *lxcpath, int flags,
> > const char *bdevtype, const char *bdevdata, uint64_t newsize,
> > @@ -2953,7 +3077,9 @@ static struct lxc_container *do_lxcapi_clone(struct lxc_container *c, const char
> > fclose(fout);
> > c->lxc_conf->rootfs.path = origroot;
> >
> > - sprintf(newpath, "%s/%s/rootfs", lxcpath, newname);
> > + ret = snprintf(newpath, MAXPATHLEN, "%s/%s/rootfs", lxcpath, newname);
> > + if (ret < 0 || ret >= MAXPATHLEN)
> > + goto out;
> > if (mkdir(newpath, 0755) < 0) {
> > SYSERROR("error creating %s", newpath);
> > goto out;
> > @@ -3009,10 +3135,15 @@ static struct lxc_container *do_lxcapi_clone(struct lxc_container *c, const char
> > }
> > }
> >
> > + // update absolute paths for union mount directories
> > + if (update_union_mount_entry_paths(c2->lxc_conf, c->config_path, c->name, lxcpath, newname) < 0)
> > + goto out;
> > +
> > // We've now successfully created c2's storage, so clear it out if we
> > // fail after this
> > storage_copied = 1;
> >
> > +
> > if (!c2->save_config(c2, NULL))
> > goto out;
> >
> > --
> > 2.6.1
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
-------------- 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/20151021/cae430d1/attachment.sig>
More information about the lxc-devel
mailing list