[CRIU] [PATCH] cg: don't leak properties when cg dir exists
Andrew Vagin
avagin at parallels.com
Thu Oct 2 08:09:32 PDT 2014
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, 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?
>
> > 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