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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

	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;

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.

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.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp