[CRIU] [PATCH] cg: don't leak properties when cg dir exists
Tycho Andersen
tycho.andersen at canonical.com
Thu Oct 2 08:16:48 PDT 2014
On Thu, Oct 02, 2014 at 07:09:32PM +0400, Andrew Vagin wrote:
> On Thu, Oct 02, 2014 at 09:58:23AM -0500, Tycho Andersen wrote:
> > On Thu, Oct 02, 2014 at 06:48:05PM +0400, Andrew Vagin wrote:
> > > On Thu, Oct 02, 2014 at 06:00:17PM +0400, Andrew Vagin wrote:
> > > > On Wed, Oct 01, 2014 at 08:57:28PM +0000, Tycho Andersen wrote:
> > > > > We're using NULL as a sentinel here to indicate that we shouldn't restore any
> > > > > cgroup properties. We should make sure that we don't leak this information and
> > > > > instead check the n_properties field, which we should also set correctly.
> > > >
> > > > Tycho, I beg you to run tests before sending patches.
> > > >
> > > > It's like brushing teeth before bedtime.
> > >
> > > I found that test passes if it's executed in a non-root cpuset group.
> > > mkdir /sys/fs/cgroup/cpuset/test
> > > echo 0 >> /sys/fs/cgroup/cpuset/test/cpuset.mems
> > > echo 0-3 >> /sys/fs/cgroup/cpuset/test/cpuset.cpus
> > > cho $$ > /sys/fs/cgroup/cpuset/test/tasks
> > >
> > > [root at avagin-fc19-cr criu]# bash test/zdtm.sh static/cgroup00
> > > ================================= CRIU CHECK =================================
> > > Looks good.
> > > Execute zdtm/live/static/cgroup00
> > > ./cgroup00 --pidfile=cgroup00.pid --outfile=cgroup00.out --dirname=cgroup00.test
> > > Dump 2708
> > > Executing pre-restore hook
> > > Cleaning cgclean.j8Mio8
> > > Left there is:
> > > cgroup.clone_children cgroup.procs cgroup.sane_behavior notify_on_release release_agent tasks
> > > Restore
> > > Check results 2708
> > > Waiting...
> > > Executing cleanup hook
> > > Cleaning cgclean.v4Pew1
> > > Left there is:
> > > cgroup.clone_children cgroup.procs cgroup.sane_behavior notify_on_release release_agent tasks
> > > 18:40:35.992: 2708: CMP [name=zdtmtst:/subcg00
> > > ] vs [name=zdtmtst:/subcg00
> > > ]
> > > 18:40:35.992: 2710: CMP [name=zdtmtst:/subcg00/subsubcg
> > > ] vs [name=zdtmtst:/subcg00/subsubcg
> > > ]
> > > 18:40:35.993: 2709: CMP [name=zdtmtst:/subcg00
> > > ] vs [name=zdtmtst:/subcg00
> > > ]
> > > 18:40:35.993: 2708: PASS
> > > Test: zdtm/live/static/cgroup00, Result: PASS
> > > ZDTM tests PASS.
> > >
> > > So I have a question. For which group does criu dump and restore
> > > parameters? It's obvious, the in our case the group doesn't belong to
> > > the test process.
> >
> > I'm not sure what you mean by this. You put $$ in the cgroup, which
> > means that the test process will be in the cgroup and thus criu should
> > dump it.
>
> It isn't obvious for me. Let's imagine the we create a container. We
> create a new set of cgroups and set parameters for these groups. Then we
> executing the init process, which forks other processes. But nobody of
> them can't change parameters for the initial set of cgroups. Is it true?
>
> Now we are going to restore the container from images. In this case we
> create a ndew set of cgroups,
Only if we use --cgroup-root to rewrite them; if not, it should
restore to the same set of cgroups that existed before. Except that
they may not exist (e.g. if you rebooted between dump/restore, and
didn't run any lxc commands that created cpuset:/lxc).
> configure them and execute "criu restore".
> And I doesn't expect that criu will change parameters for the initial
> set of cgroup. Could you explain why it should?
If you're using --cgroup-root, it should certainly not touch the
original ones. However, if you're not, it has to re-create them
because of the case I described above.
Tycho
>
> >
> > > Why do we need to dump parameters of groups, where
> > > is a root process lives? Should we skip the first level of groups and
> > > dump parameters of subgroups only?
> >
> > I think so at least for cpuset. The root cpuset.cpus and cpuset.mems
> > always have "all" the resources in them, so we don't need to do this
> > copy for the root. I can work on this and send a patch.
> >
> > Tycho
> >
> > > >
> > > > [root at avagin-fc19-cr criu]# bash test/zdtm.sh static/cgroup00
> > > > ================================= CRIU CHECK =================================
> > > > Looks good.
> > > > Execute zdtm/live/static/cgroup00
> > > > ./cgroup00 --pidfile=cgroup00.pid --outfile=cgroup00.out --dirname=cgroup00.test
> > > > Dump 1457
> > > > Executing pre-restore hook
> > > > Cleaning cgclean.OkAnTX
> > > > Left there is:
> > > > cgroup.clone_children cgroup.procs cgroup.sane_behavior notify_on_release release_agent tasks
> > > > Restore
> > > > Test: zdtm/live/static/cgroup00, Result: FAIL
> > > > ==================================== ERROR ====================================
> > > > Test: zdtm/live/static/cgroup00, Namespace:
> > > > Dump log : /root/git/criu/test/dump/static/cgroup00/1457/1/dump.log
> > > > --------------------------------- grep Error ---------------------------------
> > > > ------------------------------------- END -------------------------------------
> > > > Restore log: /root/git/criu/test/dump/static/cgroup00/1457/1/restore.log
> > > > --------------------------------- grep Error ---------------------------------
> > > > (00.032275) 1457: Error (cgroup.c:943): cg: copying property cpuset.cpus to cpuset////cpuset.cpus failed
> > > > (00.032319) 1457: Error (cgroup.c:968): cg: copying prop cpuset.cpus failed
> > > > (00.032336) 1457: Error (cgroup.c:1005): cg: Couldn't copy special cgroup properties
> > > > (00.032373) Error (cr-restore.c:1820): Restoring FAILED.
> > > > (00.039606) Error (cr-restore.c:1177): 1457 exited, status=1
> > > > ------------------------------------- END -------------------------------------
> > > > ================================= ERROR OVER =================================
> > > >
> > > > Thanks,
> > > > Andrey
> > > >
> > > > >
> > > > > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > > > > ---
> > > > > cgroup.c | 15 +++++++--------
> > > > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/cgroup.c b/cgroup.c
> > > > > index 26b0a17..ce49d04 100644
> > > > > --- a/cgroup.c
> > > > > +++ b/cgroup.c
> > > > > @@ -1114,12 +1114,7 @@ static int prepare_cgroup_dir_properties(char *path, int off, CgroupDirEntry **e
> > > > > size_t off2 = off;
> > > > >
> > > > > off2 += sprintf(path + off, "/%s", e->dir_name);
> > > > > - /*
> > > > > - * Check to see if we made e->properties NULL during restore
> > > > > - * because directory already existed and as such we don't want to
> > > > > - * change its properties
> > > > > - */
> > > > > - if (e->properties) {
> > > > > + if (e->n_properties > 0) {
> > > > > for (j = 0; j < e->n_properties; ++j) {
> > > > > if (restore_cgroup_prop(e->properties[j], path, off2) < 0)
> > > > > return -1;
> > > > > @@ -1168,7 +1163,7 @@ static int prepare_cgroup_dirs(char *paux, size_t off, CgroupDirEntry **ents, si
> > > > > /*
> > > > > * Checking to see if file already exists. If not, create it. If
> > > > > * it does exist, prevent us from overwriting the properties
> > > > > - * later by setting the CgroupDirEntry's properties pointer to NULL
> > > > > + * later by removing the CgroupDirEntry's properties.
> > > > > */
> > > > > if (access(paux, F_OK) < 0) {
> > > > > if (errno != ENOENT) {
> > > > > @@ -1181,7 +1176,11 @@ static int prepare_cgroup_dirs(char *paux, size_t off, CgroupDirEntry **ents, si
> > > > > }
> > > > > pr_info("Created dir %s\n", paux);
> > > > > } else {
> > > > > - e->properties = NULL;
> > > > > + if (e->n_properties > 0) {
> > > > > + xfree(e->properties);
> > > > > + e->properties = NULL;
> > > > > + e->n_properties = 0;
> > > > > + }
> > > > > pr_info("Determined dir %s already existed\n", paux);
> > > > > }
> > > > >
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > CRIU mailing list
> > > > > CRIU at openvz.org
> > > > > https://lists.openvz.org/mailman/listinfo/criu
> > > > _______________________________________________
> > > > CRIU mailing list
> > > > CRIU at openvz.org
> > > > https://lists.openvz.org/mailman/listinfo/criu
More information about the CRIU
mailing list