[LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy@fujitsu.com]

Hi Richard

> Hello,
> 
> OK, I see the new patches. However I just realised these tests are part
> of test_cgcore. These are not related to memcontrol. They should go in
> controllers/cgroup/core01.c.
> 
> Let's start at the beginning and look at the original selftest.
> 
> static int test_cgcore_lesser_euid_open(const char *root)
> {
> 	const uid_t test_euid = 65534;	/* usually nobody, any !root is fine */
> 	int ret = KSFT_FAIL;
> 	char *cg_test_a = NULL, *cg_test_b = NULL;
> 	char *cg_test_a_procs = NULL, *cg_test_b_procs = NULL;
> 	int cg_test_b_procs_fd = -1;
> 	uid_t saved_uid;
> 
> 	cg_test_a = cg_name(root, "cg_test_a");
> 	cg_test_b = cg_name(root, "cg_test_b");
> 
> 	if (!cg_test_a || !cg_test_b)
> 		goto cleanup;
> 
> 	cg_test_a_procs = cg_name(cg_test_a, "cgroup.procs");
> 	cg_test_b_procs = cg_name(cg_test_b, "cgroup.procs");
> 
> 	if (!cg_test_a_procs || !cg_test_b_procs)
> 		goto cleanup;
> 
> 	if (cg_create(cg_test_a) || cg_create(cg_test_b))
> 		goto cleanup;
> 
> So we create two subgroups this translates to
> 
> cg_child_a = tst_cg_group_mk(tst_cg, 'a');
> cg_child_b = tst_cg_group_mk(tst_cg, 'b');
> 
> 
> 	if (cg_enter_current(cg_test_a))
> 		goto cleanup;
> 
> This writes "0" to the cgroup.procs file which I guess is a shortcut for
> writing the current processes PID. We have to be careful here incase
> this behaviour is different on V1. Probably this translates to
> 
> SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");
> 
> It's not clear why the current PID is moved to this CG. It may be to
> ensure we have permission to move to a sibling CGroup and to eliminate
> other possible reasons for getting EACCES later on.

Yes, it's not clear.

But from kernel commit[1], it just check open fd's perms instead of 
current. So I don't use a and b two cgroup .  Also, it indeed fail on
unfixed kernel  by using only one a cgrpup.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1756d7994 

> 
> 	if (chown(cg_test_a_procs, test_euid, -1) ||
> 	    chown(cg_test_b_procs, test_euid, -1))
> 		goto cleanup;
> 
> Then the procs files are chowned to nobody. I see no reason this
> function can not be implemented in the same way safe_cg_printf is. We
> don't need to check which controller the file belongs to, just chown all
> of them.
> 
> 	saved_uid = geteuid();
> 	if (seteuid(test_euid))
> 		goto cleanup;

Yes, we can chown all cgroup procs files ie SAFE_CG_PRINTF did.

> 
> Then the current procs effective uid is changed. We need to make sure to
> set this back before doing cleanup (or fork like you did originally).
> 
> 	cg_test_b_procs_fd = open(cg_test_b_procs, O_RDWR);
> 
> 	if (seteuid(saved_uid))
> 		goto cleanup;
> 
> Then the file is opened and the euid set back
> 
> 	if (cg_test_b_procs_fd < 0)
> 		goto cleanup;
> 
> 	if (write(cg_test_b_procs_fd, "0", 1) >= 0 || errno != EACCES)
> 		goto cleanup;
> 
> Then we try to move to CG B and expect to get EACCES (because of the
> permissions present when opening the file).
> 
> 	ret = KSFT_PASS;
> ...
> 
> 
> Probably we want to run this test on any controllers which are
> available. Currently the API doesn't support that. We need some way of
> specifying that the test will use any available controllers in
> tst_cg_require and/or tst_test.

I will move this case into cgroup/cg_core01.c, kernel selftest seems use 
memory controller, I guess we can also use memory controller because it 
is most common.
> 
> Then we need to handle setting the euid between open and writing which
> stops us from using safe_cg_printf.
> 
> Probably the API shouldn't try to handle stuff this wierd. Instead we
> can create a function like
> 
> int n = tst_cg_group_dirfds(int *dirfds)
> 
> which copies tst_cgroup_group.dirs[i].dir_fd into dirfds (which can be
> statically allocated in the test code if we export CTRLS_MAX).
> 
> Then we can do
> 
> seteuid(nobody);
> 
> for (i = 0; i < n; i++) {
>      procfds[i] = openat(dirfds[i], "cgroup.procs")
> }
> 
> seteuid(saved_euid);
> 
> for (i = 0; i < n; i++) {
>      ret = write(procfds[i], "0", 1);
>      if (ret >= 0)
>         tst_res(TFAIL...)
>      ...
> 
>      close(procfds[i]);
> }
> 
> and whatever else is required by tests which are doing something unusual
> with the CG hierarchy.

OK. Will  try.

Best Regards
Yang Xu
>