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

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 10/28/26 13:51, no-reply@sashiko.virtuozzo.com wrote:
> 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.

A shame, didn't notice. Will fix in v3.

> 
> [ ... ]
> 
>> +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.

Wouldn't bother:
1. We don't run our tests on hosts with 128+ CPUs.
2. This test is supposed to be run as root, so I, as a test author,
    really expect to see all CPUs available. If someone disabled first
    128 CPUs and left on 129..., then something is going terribly wrong.

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

Cool, this is true. I'll move cpu check to the beginning.

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

Also true, will fix in v3.

> [ ... ]
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

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