Re: [Devel] [PATCH hci-8.1 2/2] selftests/ve: check that the NUMA API syscalls return ENOSYS in a VE

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/ve: numa syscalls test fails instead of skipping on non-NUMA host
- [Low] selftests/ve: FIXTURE_SETUP does not initialise ctid before teardown uses it
--

commit 3833f07c24efbbde914f6df791979fabf50e44b6
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check that the NUMA API syscalls return ENOSYS in a VE

This adds a selftest that calls all six NUMA API syscalls on the host and
inside a VE. It expects 0 on the host and -ENOSYS in the VE.

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

> diff --git a/tools/testing/selftests/ve/ve_numa_syscalls_prohibited_test.c b/tools/testing/selftests/ve/ve_numa_syscalls_prohibited_test.c
> new file mode 100644
> index 000000000000..4cae1c74b46b
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_numa_syscalls_prohibited_test.c

[ ... ]

> +FIXTURE(numa_syscalls)
> +{
> +	int cgv2_fd;
> +	int ctid;
> +};
> +
> +FIXTURE_SETUP(numa_syscalls)
> +{
> +	unsigned long allowed[NODE_BITS / LONG_BITS] = { 0 };
> +	unsigned int i;
> +
> +	self->cgv2_fd = -1;
> +
> +	if (geteuid() != 0)
> +		SKIP(return, "creating a ve cgroup needs root");

[ ... ]

> +	ASSERT_EQ(syscall(__NR_get_mempolicy, NULL, allowed, NODE_BITS + 1UL,
> +			  0UL, MPOL_F_MEMS_ALLOWED), 0);

[Severity: Medium]
Can this probe turn into a false failure on a host without NUMA support?

If the host kernel lacks CONFIG_NUMA or CONFIG_MIGRATION, or seccomp blocks
these syscalls, get_mempolicy() returns -ENOSYS on the host. The
ASSERT_EQ(..., 0) in FIXTURE_SETUP(numa_syscalls) then aborts setup with a
failure, and ve_calls_return_enosys never runs.

The sibling ve_numa_migrate_test uses SKIP for missing NUMA preconditions.
Would a SKIP be more appropriate here when the probe returns -1 with
errno == ENOSYS?

[ ... ]

> +FIXTURE_TEARDOWN(numa_syscalls)
> +{
> +	if (self->cgv2_fd >= 0) {
> +		destroy_ve(self->cgv2_fd, self->ctid);
> +		close(self->cgv2_fd);
> +	}
> +}

[Severity: Low]
Is self->ctid initialized before FIXTURE_TEARDOWN(numa_syscalls) uses it?

FIXTURE_SETUP(numa_syscalls) sets self->cgv2_fd = -1 but never sets
self->ctid. If an ASSERT fails after mount_cg2_fd() succeeds and before
make_ve() assigns ctid, for example the write_file_at() of
cgroup.subtree_control, the teardown sees cgv2_fd >= 0 and calls
destroy_ve(self->cgv2_fd, self->ctid) with ctid unset.

destroy_ve() only returns early for id < 0, so it would try to remove
cgroup "0" with its retry loop. Would setting self->ctid = -1 next to
self->cgv2_fd = -1 avoid this?

> +TEST_F(numa_syscalls, host_calls_succeed)
> +{
> +	struct call_result res[NR_CALLS];
> +	unsigned int i;
> +
> +	run_calls(res);
> +	for (i = 0; i < NR_CALLS; i++) {
> +		EXPECT_EQ(res[i].ret, 0)
> +			TH_LOG("%s on the host: %s", numa_calls[i].name,
> +			       strerror(res[i].err));
> +	}
> +}

[Severity: Medium]
Does host_calls_succeed report a failure when a call returns -ENOSYS on the
host?

On a host where the NUMA syscalls are unavailable, the EXPECT_EQ(res[i].ret,
0) check fails the test instead of skipping it. The commit message says the
host result is what shows the ENOSYS in the VE comes from the VE check, so
an environment that cannot provide that baseline might be better reported
as a skip.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261105-b4-prep-khorenko-vstor-153679-mbind-ve-v1-0-b4fcd27c8001@virtuozzo.com?part=2

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