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

[xuyang2018.jy@fujitsu.com]

Hi Richard

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

Will follow kernel selftest style, use child_a and child_b two test cgroups.

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

After some try, I think may use the old way ie safe_cg_printf but store 
the fd array and return arry size is more clear. Please see my RFC-V3 patch.

Best Regards
Yang Xu
> Best Regards
> Yang Xu
>>
>