[CRIU] [PATCH] cg: don't leak properties when cg dir exists

[Andrew Vagin]

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. 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?

> 
> [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