[prev in list] [next in list] [prev in thread] [next in thread]
List: lxc-devel
Subject: [lxc-devel] Fwd: Re: [PATCH 2/2 v2] Update absolute paths for overlay and aufs mounts
From: Christian Brauner <christianvanbrauner () gmail ! com>
Date: 2015-10-30 15:30:55
Message-ID: CALNVzyAvm72S3wkoGb6ROHUjQSXpgn-XYnbk2WmPukY=gG8e7g () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
---------- Forwarded message ----------
From: "Christian Brauner" <christianvanbrauner@gmail.com>
Date: Oct 30, 2015 4:27 PM
Subject: Re: [PATCH 2/2 v2] Update absolute paths for overlay and aufs
mounts
To: "Serge E. Hallyn" <serge.hallyn@ubuntu.com>
Cc: "lxc-dev" <lxc-devel@lists.linuxcontainers.org>
>
> On Oct 30, 2015 4:22 PM, "Serge Hallyn" <serge.hallyn@ubuntu.com> wrote:
> >
> > Quoting Christian Brauner (christianvanbrauner@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 (by calling
clone_update_unexp_ovl_dir())
> > >
> > > 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: 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@gmail.com>
> >
> > Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> >
> > > ---
> > > src/lxc/lxccontainer.c | 97
++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 97 insertions(+)
> > >
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index 42e23e7..8edf6a2 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -2894,6 +2894,99 @@ 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;
> > > + int i;
> > > + int fret = -1;
> > > + int ret = 0;
> > > + struct lxc_list *iterator;
> > > + const char *ovl_dirs[] = {"br", "upperdir", "workdir"};
> > > +
> > > + cleanpath = strdup(newpath);
> > > + if (!cleanpath)
> > > + goto err;
> > > +
> > > + remove_trailing_slashes(cleanpath);
> > > +
> >
> > Would be wortha comment here saying "we have to update the unexpanded
> > configuration separately from the expanded one."
I can insert it, add your Acked-by line to the cover letter and leave a
short reminder in the cover letter that I only inserted a comment per your
request so you know what's going on. Ok?
> >
> > > + for (i = 0; i < sizeof(ovl_dirs) / sizeof(ovl_dirs[0]); i++) {
> > > + if (!clone_update_unexp_ovl_dir(lxc_conf, lxc_path,
newpath,
> > > + lxc_name, newname,
ovl_dirs[i]))
> > > + goto err;
> > > + }
> > > +
> > > + 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) {
> > > + char *mnt_entry = NULL;
> > > + char *new_mnt_entry = NULL;
> > > + char *tmp = NULL;
> > > + char *tmp_mnt_entry = NULL;
> > > + mnt_entry = iterator->elem;
> > > +
> > > + if (strstr(mnt_entry, "overlay"))
> > > + tmp = "upperdir";
> > > + else if (strstr(mnt_entry, "aufs"))
> > > + tmp = "br";
> > > +
> > > + if (!tmp)
> > > + continue;
> > > +
> > > + ret = snprintf(old_upper, MAXPATHLEN,
"%s=%s/%s", tmp, lxc_path, lxc_name);
> > > + if (ret < 0 || ret >= MAXPATHLEN)
> > > + goto err;
> > > +
> > > + 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);
> > > + }
> > > +
> > > + if (strstr(mnt_entry, old_work)) {
> > > + if (tmp_mnt_entry)
> > > + new_mnt_entry =
lxc_string_replace(old_work, new_work, tmp_mnt_entry);
> > > + }
> > > +
> > > + 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);
> > > + }
> > > +
> > > + free(new_mnt_entry);
> > > + free(tmp_mnt_entry);
> > > + }
> > > +
> > > + fret = 0;
> > > +err:
> > > + free(cleanpath);
> > > + return fret;
> > > +}
> > > +
> > > 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,
> > > @@ -3009,6 +3102,10 @@ 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;
> > > --
> > > 2.6.2
> > >
[Attachment #5 (text/html)]
<p dir="ltr"><br>
---------- Forwarded message ----------<br>
From: "Christian Brauner" <<a \
href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>><br>
Date: Oct 30, 2015 4:27 PM<br>
Subject: Re: [PATCH 2/2 v2] Update absolute paths for overlay and aufs mounts<br>
To: "Serge E. Hallyn" <<a \
href="mailto:serge.hallyn@ubuntu.com">serge.hallyn@ubuntu.com</a>><br>
Cc: "lxc-dev" <<a \
href="mailto:lxc-devel@lists.linuxcontainers.org">lxc-devel@lists.linuxcontainers.org</a>></p>
<p dir="ltr">><br>
> On Oct 30, 2015 4:22 PM, "Serge Hallyn" <<a \
href="mailto:serge.hallyn@ubuntu.com">serge.hallyn@ubuntu.com</a>> wrote:<br> > \
><br> > > Quoting Christian Brauner (<a \
href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>):<br> \
> > > When using overlay and aufs mounts with lxc.mount.entry users have to \
specify<br> > > > absolute paths for upperdir and workdir which will then \
get created<br> > > > automatically by mount_entry_create_overlay_dirs() \
and<br> > > > mount_entry_create_aufs_dirs() in conf.c. When we clone a \
container with<br> > > > overlay or aufs lxc.mount.entry entries we need to \
update these absolute paths.<br> > > > In order to do this we add the \
function update_union_mount_entry_paths() in<br> > > > lxccontainer.c. The \
function updates the mounts in two locations:<br> > > ><br>
> > > 1) lxc_conf->mount_list<br>
> > ><br>
> > > and<br>
> > ><br>
> > > 2) lxc_conf->unexpanded_config (by calling \
clone_update_unexp_ovl_dir())<br> > > ><br>
> > > If we were to only update 2) we would end up with wrong upperdir and \
workdir<br> > > > mounts as the absolute paths would still point to the \
container that serves as<br> > > > the base for the clone. If we were to \
only update 1) we would end up with wrong<br> > > > upperdir and workdir \
lxc.mount.entry entries in the clone's config as the<br> > > > absolute \
paths in upperdir and workdir would still point to the container that<br> > > \
> serves as the base for the clone. Updating both will get the job done.<br> > \
> ><br> > > > NOTE: This function does not sanitize paths apart from \
removing trailing<br> > > > slashes. (So when a user specifies \
//home//someone/// it will be cleaned to<br> > > > //home//someone. This is \
the minimal path cleansing which is also done by<br> > > > \
lxc_container_new().) But the mount_entry_create_overlay_dirs() and<br> > > \
> mount_entry_create_aufs_dirs() functions both try to be extremely strict \
about<br> > > > when to create upperdirs and workdirs. They will only accept \
sanitized paths,<br> > > > i.e. they require /home/someone. I think this is \
a (safety) virtue and we<br> > > > should consider sanitizing paths in \
general. In short:<br> > > > update_union_mount_entry_paths() does update \
all absolute paths to the new<br> > > > container but \
mount_entry_create_overlay_dirs() and<br> > > > \
mount_entry_create_aufs_dirs() will still refuse to create upperdir and workdir<br> \
> > > when the updated path is unclean. This happens easily when e.g. a user \
calls<br> > > > lxc-clone -o OLD -n NEW -P //home//chb///.<br>
> > ><br>
> > > Signed-off-by: Christian Brauner <<a \
href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>><br> \
> ><br> > > Acked-by: Serge E. Hallyn <<a \
href="mailto:serge.hallyn@ubuntu.com">serge.hallyn@ubuntu.com</a>><br> > \
><br> > > > ---<br>
> > > src/lxc/lxccontainer.c | 97 \
++++++++++++++++++++++++++++++++++++++++++++++++++<br> > > > 1 file \
changed, 97 insertions(+)<br> > > ><br>
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c<br>
> > > index 42e23e7..8edf6a2 100644<br>
> > > --- a/src/lxc/lxccontainer.c<br>
> > > +++ b/src/lxc/lxccontainer.c<br>
> > > @@ -2894,6 +2894,99 @@ static int create_file_dirname(char *path, \
struct lxc_conf *conf)<br> > > > return ret;<br>
> > > }<br>
> > ><br>
> > > +/* When we clone a container with overlay or aufs lxc.mount.entry \
entries we<br> > > > +* need to update these absolute paths. In order to \
do this we add the function<br> > > > +* update_union_mount_entry_paths() \
in lxccontainer.c. The function operates on<br> > > > +* \
c->lxc_conf->unexpanded_config instead of the intuitively plausible<br> > \
> > +* c->lxc_conf->mount_list because the latter also contains mounts \
from other<br> > > > +* files as well as generic mounts. */<br>
> > > +static int update_union_mount_entry_paths(struct lxc_conf \
*lxc_conf,<br> > > > + \
const char *lxc_path,<br> > > > + \
const char *lxc_name,<br> > > > + \
const char *newpath,<br> > > > + \
const char *newname)<br> > > > +{<br>
> > > + char new_upper[MAXPATHLEN];<br>
> > > + char new_work[MAXPATHLEN];<br>
> > > + char old_upper[MAXPATHLEN];<br>
> > > + char old_work[MAXPATHLEN];<br>
> > > + char *cleanpath = NULL;<br>
> > > + int i;<br>
> > > + int fret = -1;<br>
> > > + int ret = 0;<br>
> > > + struct lxc_list *iterator;<br>
> > > + const char *ovl_dirs[] = {"br", \
"upperdir", "workdir"};<br> > > > +<br>
> > > + cleanpath = strdup(newpath);<br>
> > > + if (!cleanpath)<br>
> > > + goto err;<br>
> > > +<br>
> > > + remove_trailing_slashes(cleanpath);<br>
> > > +<br>
> ><br>
> > Would be wortha comment here saying "we have to update the \
unexpanded<br> > > configuration separately from the expanded one."</p>
<p dir="ltr">I can insert it, add your Acked-by line to the cover letter and leave a \
short reminder in the cover letter that I only inserted a comment per your request so \
you know what's going on. Ok?</p> <p dir="ltr">> ><br>
> > > + for (i = 0; i < sizeof(ovl_dirs) / sizeof(ovl_dirs[0]); \
i++) {<br> > > > + if \
(!clone_update_unexp_ovl_dir(lxc_conf, lxc_path, newpath,<br> > > > + \
lxc_name, newname, ovl_dirs[i]))<br> > > > + \
goto err;<br> > > > + }<br>
> > > +<br>
> > > + ret = snprintf(old_work, MAXPATHLEN, \
"workdir=%s/%s", lxc_path, lxc_name);<br> > > > + if (ret \
< 0 || ret >= MAXPATHLEN)<br> > > > + goto err;<br>
> > > +<br>
> > > + ret = snprintf(new_work, MAXPATHLEN, \
"workdir=%s/%s", cleanpath, newname);<br> > > > + if (ret \
< 0 || ret >= MAXPATHLEN)<br> > > > + goto err;<br>
> > > +<br>
> > > + lxc_list_for_each(iterator, &lxc_conf->mount_list) \
{<br> > > > + char *mnt_entry = NULL;<br>
> > > + char *new_mnt_entry = NULL;<br>
> > > + char *tmp = NULL;<br>
> > > + char *tmp_mnt_entry = NULL;<br>
> > > + mnt_entry = iterator->elem;<br>
> > > +<br>
> > > + if (strstr(mnt_entry, "overlay"))<br>
> > > + tmp = "upperdir";<br>
> > > + else if (strstr(mnt_entry, "aufs"))<br>
> > > + tmp = "br";<br>
> > > +<br>
> > > + if (!tmp)<br>
> > > + continue;<br>
> > > +<br>
> > > + ret = snprintf(old_upper, MAXPATHLEN, \
"%s=%s/%s", tmp, lxc_path, lxc_name);<br> > > > + \
if (ret < 0 || ret >= MAXPATHLEN)<br> > > > + \
goto err;<br> > > > +<br>
> > > + ret = snprintf(new_upper, MAXPATHLEN, \
"%s=%s/%s", tmp, cleanpath, newname);<br> > > > + \
if (ret < 0 || ret >= MAXPATHLEN)<br> > > > + \
goto err;<br> > > > +<br>
> > > + if (strstr(mnt_entry, old_upper)) \
{<br> > > > + tmp_mnt_entry = \
lxc_string_replace(old_upper, new_upper, mnt_entry);<br> > > > + \
}<br> > > > +<br>
> > > + if (strstr(mnt_entry, old_work)) \
{<br> > > > + if \
(tmp_mnt_entry)<br> > > > + \
new_mnt_entry = lxc_string_replace(old_work, new_work, tmp_mnt_entry);<br> > > \
> + }<br> > > > +<br>
> > > + if (new_mnt_entry) {<br>
> > > + \
free(iterator->elem);<br> > > > + \
iterator->elem = strdup(new_mnt_entry);<br> > > > + \
} else if (tmp_mnt_entry) {<br> > > > + \
free(iterator->elem);<br> > > > + \
iterator->elem = strdup(tmp_mnt_entry);<br> > > > + \
}<br> > > > +<br>
> > > + free(new_mnt_entry);<br>
> > > + free(tmp_mnt_entry);<br>
> > > + }<br>
> > > +<br>
> > > + fret = 0;<br>
> > > +err:<br>
> > > + free(cleanpath);<br>
> > > + return fret;<br>
> > > +}<br>
> > > +<br>
> > > static struct lxc_container *do_lxcapi_clone(struct lxc_container \
*c, const char *newname,<br> > > > const char \
*lxcpath, int flags,<br> > > > const char *bdevtype, \
const char *bdevdata, uint64_t newsize,<br> > > > @@ -3009,6 +3102,10 @@ \
static struct lxc_container *do_lxcapi_clone(struct lxc_container *c, const char<br> \
> > > }<br> > > > }<br>
> > ><br>
> > > + // update absolute paths for union mount directories<br>
> > > + if (update_union_mount_entry_paths(c2->lxc_conf, \
c->config_path, c->name, lxcpath, newname) < 0)<br> > > > + \
goto out;<br> > > > +<br>
> > > // We've now successfully created c2's storage, so \
clear it out if we<br> > > > // fail after this<br>
> > > storage_copied = 1;<br>
> > > --<br>
> > > 2.6.2<br>
> > ><br>
</p>
[Attachment #6 (text/plain)]
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic