Re: [Devel] [PATCH hci-8.1 v3 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: get_nth_cpu() bounds loop by sizeof(cpu_set_t) bytes, not CPU_SETSIZE
- [Medium] selftests/ve: expected CT CPU count taken from caller's affinity, not CT cpuset
- [Low] selftests/ve: FIXTURE_SETUP leaks CT cgroup if prepare_namespaces() ASSERT fails
--

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?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029-ct-invalid-nrpoc-v3-0-89d4b0b7d520@virtuozzo.com?part=2

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