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

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

On 10/29/26 11:22, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] selftests/ve: get_nth_cpu() bounds loop by sizeof(cpu_set_t) bytes, not CPU_SETSIZE

Replied in v2.

> - [Medium] selftests/ve: expected CT CPU count taken from caller's affinity, not CT cpuset

This is not a CT. This test is a root-only, no need for complications.

> - [Low] selftests/ve: FIXTURE_SETUP leaks CT cgroup if prepare_namespaces() ASSERT fails

This is fine IMO. With skip it was correct to move it to the top.
Here if something fails we should leave all resources, to simplify
debugging.

> --
> 
> commit 55fc819ea7f7954caf6a8e968697a2cc48fd1982
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> selftests: ve: Test sched_getaffinity in VE
> 
> This adds a selftest that checks sched_getaffinity() inside a VE respects
> both the cgroup cpuset and the cpu.nr_cpus limit. It adds three test cases
> and registers the binary 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 000000000000..98acc246ede0
>> --- /dev/null
>> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> 
> [ ... ]
> 
>> +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), while CPU_ISSET() takes a bit index and the set
> holds CPU_SETSIZE (1024) bits:
> 
>      for (i = 0; i < sizeof(*mask); i++) {
>          if (!CPU_ISSET(i, mask) || --n)
> 
> The scan stops at CPU 127.  If the 2nd or 3rd allowed CPU in orig_cpuset
> is numbered 128 or higher (for example under taskset, a restricted cpuset,
> or a large sparse host), get_nth_cpu() returns -1.
> 
> update_cg_cpuset then fails ASSERT_GE(cpu_leave, 0), and limits_interaction
> fails ASSERT_TRUE(c1 >= 0 && c3 >= 0), without sched_getaffinity() in the
> VE being exercised.
> 
> The loop also compares a signed int with a size_t.  Should the bound be
> CPU_SETSIZE?
> 
>> +FIXTURE_SETUP(ve_getaff_test)
>> +{
>> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
>> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
> 
> [Severity: Medium]
> Is orig_cpucount the right expected value for the CT?  It is taken from the
> runner's own affinity mask, not from the CT cgroup's effective cpuset.
> 
> In getaffinity_ve(), run_in_ve() forks a child that does:
> 
>      enter_cgroup(cgv2_fd, ctid);
>      enter_namespaces(ctx, flags);
>      ret = fn(arg);
> 
> Nothing in the child restores the parent's affinity after joining the
> cgroup.  So the mask that __getaffinity_ve() reports derives from the CT
> cgroup's cpuset, which is inherited from the parent cgroup.
> 
> I did not read cpuset_attach() in this tree.  From memory of mainline, it
> resets cpus_mask to the cgroup's effective_cpus on attach, but that is
> unverified.
> 
> If the runner was restricted, for example with taskset -c 0,1 on a host
> with more CPUs, the CT may report more CPUs than orig_cpucount.  Then this
> fails in update_cg_cpuset, nr_proc_bound and limits_interaction:
> 
>      ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));
> 
> Would it be better to derive the expected count from the CT cgroup's
> effective cpuset, or to skip when the two differ?
> 
> Separately, sched_getaffinity() with a fixed sizeof(cpu_set_t) returns
> EINVAL when nr_cpu_ids is greater than 1024.  FIXTURE_SETUP asserts on that
> call rather than skipping.
> 
> [ ... ]
> 
>> +	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);
>> +}
> 
> [Severity: Low]
> Can this leak the CT cgroup created by make_ve()?  If prepare_namespaces()
> fails, the ASSERT_EQ() aborts FIXTURE_SETUP before setup_completed is set.
> 
> In kselftest_harness.h, __TEST_F_IMPL only calls the teardown when
> _metadata->setup_completed is true:
> 
>      if (_metadata->setup_completed && !fixture_name##_teardown_parent && \
>              __sync_bool_compare_and_swap(teardown, false, true)) \
>          fixture_name##_teardown(_metadata, self, variant->data); \
> 
> So FIXTURE_TEARDOWN(ve_getaff_test) never runs.  close_namespace_fds(),
> destroy_ve() and close(cgv2_fd) are all skipped.
> 
> I inferred, but did not verify in this tree, that the cgroup persists after
> the process exits.  If so, each leaked cgroup keeps the first free id in
> the CTID_MIN..CTID_MAX range that make_ve() scans:
> 
>      for (id = from; id < CTID_MAX; id++) {
>          snprintf(name, sizeof(name), "%d", id);
>          if (faccessat(cgv2_fd, name, F_OK, 0) != 0 && errno == ENOENT)
>              break;
>      }
> 
> This only happens on a failure path.  The same pattern exists in other ve
> selftests such as ve_ns_owner_test and ve_perms_test, so this patch adds
> another instance rather than a new class of problem.
> 
> Could the cleanup be done on that failure path, for example by destroying
> the CT before asserting?
> 

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

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