Re: [Devel] [PATCH hci-8.1 v2 2/2] selftests: ve: Test sched_getaffinity in VE

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: ve: FIXTURE_SETUP skips teardown, leaking VE cgroup and fds
- [Medium] selftests: ve: get_nth_cpu() loop bound uses sizeof(*mask) instead of CPU_SETSIZE
- [Low] selftests: ve: copy-pasted header comment and stray semicolon after FIXTURE_SETUP
--

commit 72cb69c9cdabbcdd75bce8639364f25d3d6973b0
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

selftests: ve: Test sched_getaffinity in VE

This adds a new selftest, ve_sched_getaffinity_test, which checks that
sched_getaffinity() inside a VE respects both the cgroup cpuset and the
cpu.nr_cpus limits. It also registers the test in the Makefile and
.gitignore.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-141682

> diff --git a/tools/testing/selftests/ve/ve_sched_getaffinity_test.c b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> new file mode 100644
> index 0000000000000..376b65a215f12
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ve_mount_accounting selftests
> + *

[Severity: Low]
This isn't a bug, but the header comment names ve_mount_accounting, which
looks like it was copied from another test. This file tests
sched_getaffinity() and the cpuset and nr_cpus virtualization.

[ ... ]

> +static int get_nth_cpu(cpu_set_t *mask, int n)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(*mask); i++) {
> +		if (!CPU_ISSET(i, mask) || --n)
> +			continue;
> +
> +		return i;
> +	}
> +	return -1;
> +}

[Severity: Medium]
Is the loop bound in get_nth_cpu() correct? sizeof(*mask) is the size of
cpu_set_t in bytes (128), but CPU_ISSET() takes a bit index up to
CPU_SETSIZE (1024).

As written, only CPU ids 0..127 are scanned. If the nth allowed CPU has an
id of 128 or more, for example on a large host or under taskset, the
function returns -1.

In that case update_cg_cpuset fails on ASSERT_GE(cpu_leave, 0). The
limits_interaction test fails on ASSERT_TRUE(c1 >= 0 && c3 >= 0).

Would "i < CPU_SETSIZE" be the intended bound? The loop also compares a
signed int against a size_t.

> +FIXTURE_SETUP(ve_getaff_test)
> +{
> +	self->cgv2_fd = mount_cg2_fd();
> +	ASSERT_GE(self->cgv2_fd, 0);
> +
> +	ASSERT_EQ(write_file_at(self->cgv2_fd, "cgroup.subtree_control",
> +		  VE_CONTROLLERS), 0);
> +
> +	self->ctid = make_ve(self->cgv2_fd, CTID_MIN);
> +	ASSERT_GE(self->ctid, 0);
> +
> +	ASSERT_EQ(prepare_namespaces(&self->ns_ctx, CLONE_NEWVE,
> +				self->cgv2_fd, self->ctid), 0);
> +
> +
> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
> +	if (CPU_COUNT(&self->orig_cpuset) < 2)
> +		SKIP(return, "Not enough CPUs available");
> +
> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
> +};

[Severity: Medium]
Can this leak the VE cgroup and the namespace and cgroup fds? The
sched_getaffinity() ASSERT and the SKIP(return, "Not enough CPUs
available") both run after mount_cg2_fd(), make_ve() and
prepare_namespaces() have already succeeded.

In kselftest_harness.h, __TEST_F_IMPL() does this after the setup call:

    if (_metadata->exit_code)
        _exit(0);
    _metadata->setup_completed = true;

When setup exits early, setup_completed is never set. The teardown call is
guarded by setup_completed, so FIXTURE_TEARDOWN never runs.
close_namespace_fds(), destroy_ve() and close() are all skipped.

On a host with fewer than 2 allowed CPUs, each run would leave a CT cgroup
behind and use up a CTID.

Could the CPU count check be moved before any resources are created, or
could the resources be cleaned up explicitly before the SKIP and the
failing ASSERT paths?

Also, FIXTURE_SETUP ends with a stray semicolon ("};"), and there are
extra blank lines in this function. mmap() and munmap() are used without
including <sys/mman.h>, which currently only comes in transitively through
kselftest_harness.h.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ct-invalid-nrpoc-v2-0-968fe985ce27@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel