Re: [Devel] [PATCH hci-8.1 1/5] selftests/ve: check CPUID override emulation for VE tasks

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests/ve: child may run CPUID before faulting is armed, so tests are racy
- [Low] selftests/ve: ve_cpuid_override_test binary missing from .gitignore
- [Low] selftests/ve: x86-only cpuid asm built unconditionally for all ARCH
- [Low] selftests/ve: 4096-byte snapshot of cpuid_override can truncate the table, so restore fails
- [Low] selftests/ve: VE cgroup leaks when an ASSERT fails in FIXTURE_SETUP after make_ve()
- [Low] selftests/ve: leaf 1 EAX compared against single-CPU sample while child is unpinned
--

commit 931878a5cb863ada037d978eae5d17e57f54cba3
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check CPUID override emulation for VE tasks

This commit adds a selftest that writes a one-entry /proc/vz/cpuid_override
table and checks that a task in a fresh VE sees the table values while the
host sees the hardware values. A second test flushes the table after the
task was armed and expects the hardware values again.

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

> diff --git a/tools/testing/selftests/ve/Makefile b/tools/testing/selftests/ve/Makefile
> --- a/tools/testing/selftests/ve/Makefile
> +++ b/tools/testing/selftests/ve/Makefile
> @@ -5,5 +5,6 @@ CFLAGS += -g -Wall -O2
>  TEST_GEN_PROGS += ve_ns_owner_test
>  TEST_GEN_PROGS += ve_perms_test
>  TEST_GEN_PROGS += ve_mount_accounting_test
> +TEST_GEN_PROGS += ve_cpuid_override_test

[Severity: Low]
This isn't a bug in functionality, but the new binary ve_cpuid_override_test
is not added to tools/testing/selftests/ve/.gitignore, which lists only
ve_ns_owner_test, ve_perms_test and ve_mount_accounting_test.

Running make in tools/testing/selftests/ve will leave this binary untracked
in git status. Should it get an entry there?

[Severity: Low]
The test is added unconditionally for every architecture, and do_cpuid()
below uses x86-only inline asm.

Since tools/testing/selftests/Makefile has an unconditional "TARGETS += ve",
can this break the build of the whole ve selftest directory on non-x86 ARCH,
and on 32-bit PIC x86 because of the "=b" constraint? Would an arch check in
this Makefile be appropriate?

[ ... ]

> diff --git a/tools/testing/selftests/ve/ve_cpuid_override_test.c b/tools/testing/selftests/ve/ve_cpuid_override_test.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_cpuid_override_test.c

[ ... ]

> +static int cpuid_child(struct child_ctx *ctx)
> +{
> +	struct cpuid_regs r;
> +	char go;
> +
> +	close(ctx->result_pipe[0]);
> +	close(ctx->go_pipe[1]);
> +
> +	if (unshare(CLONE_NEWVE) < 0)
> +		return 21;
> +	if (write(ctx->result_pipe[1], "R", 1) != 1)
> +		return 22;
> +	if (read(ctx->go_pipe[0], &go, 1) != 1)
> +		return 23;
> +
> +	do_cpuid(CPUID_LEAF, &r);

[Severity: Medium]
Is the child guaranteed to be armed before it executes CPUID here?

CPUID faulting is only switched on in __switch_to_xtra() at the next
context switch, when prev_cpuid != next_cpuid:

arch/x86/kernel/process.c:__switch_to_xtra() {
    ...
	prev_cpuid = (tifp & _TIF_NOCPUID) || (tifp & _TIF_CPUID_OVERRIDE);
	next_cpuid = (tifn & _TIF_NOCPUID) || (tifn & _TIF_CPUID_OVERRIDE);
	if (prev_cpuid != next_cpuid)
		set_cpuid_faulting(next_cpuid);
    ...
}

The only blocking point of the child is read(ctx->go_pipe[0]). In
ve_sees_table_host_sees_hardware() the parent writes "G" right after
spawn_cpuid_child() has read "R".

On SMP the byte can already be in the pipe when the child calls read(). The
read then returns without sleeping, no context switch occurs, and the child
executes CPUID natively. It would then report the hardware EDX and fail
EXPECT_EQ(r.edx, self->host.edx & ~self->bit).

In flushed_table_falls_back_to_hardware() the parent calls
write_cpuid_override("\n") right after reading "R", and nothing waits for
the child to sleep. Can the child end up never being armed, so that the test
passes without exercising the armed-then-flushed path?

Later patches in the series (including "selftests/ve: check CPUID override
right after entering a VE") keep this handshake, and the final series state
still has the "R" then immediate "G" pattern with no wait for the child to
sleep, so the race is not fixed there either.

> +	if (write(ctx->result_pipe[1], &r, sizeof(r)) != sizeof(r))
> +		return 24;
> +	return 0;
> +}

[ ... ]

> +FIXTURE_SETUP(ve_cpuid_override)
> +{
> +	int fd;
> +
> +	fd = open(CPUID_OVERRIDE_PATH, O_RDONLY);
> +	if (fd < 0)
> +		SKIP(return, "%s: %s", CPUID_OVERRIDE_PATH, strerror(errno));
> +	close(fd);
> +
> +	ASSERT_GE(read_file_at(AT_FDCWD, CPUID_OVERRIDE_PATH, self->saved,
> +			       sizeof(self->saved)), 0);

[Severity: Low]
Can this snapshot be truncated? self->saved is CPUID_TABLE_MAX (4096) bytes,
and read_file_at() fills at most buflen-1 bytes without reporting an error.

With up to 128 entries of roughly 45 to 64 bytes each, a large pre-existing
table can exceed 4095 bytes. If the cut falls mid-entry, the restore write in
FIXTURE_TEARDOWN_PARENT() fails with -EINVAL. If it falls on an entry
boundary, a shorter table is installed.

> +
> +	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);
> +
> +	/* Mask one feature bit the host has; SSE2 when present. */
> +	do_cpuid(CPUID_LEAF, &self->host);
> +	self->bit = 1u << 26;
> +	if (!(self->host.edx & self->bit))
> +		self->bit = self->host.edx & -self->host.edx;
> +	ASSERT_NE(self->bit, 0);

[Severity: Low]
If an ASSERT fails in FIXTURE_SETUP after make_ve() (for example
ASSERT_NE(self->bit, 0)), will the VE cgroup be leaked?

The harness runs teardown only when setup has completed, so the statement in
the commit message that "The teardown runs in the parent because the harness
skips a child-side teardown once an ASSERT has fired" only covers the test
body, not a failure in setup. The ASSERT_NE path is very unlikely on x86, so
the impact looks small.

[ ... ]

> +FIXTURE_TEARDOWN_PARENT(ve_cpuid_override)
> +{
> +	int cgv2_fd;
> +
> +	write_cpuid_override(self->saved[0] ? self->saved : "\n");

[Severity: Low]
Should the return value of write_cpuid_override() be checked here? This is
the only restore of host state, and if it fails, the test's one-entry table
stays installed with no diagnostic.

[ ... ]

> +TEST_F(ve_cpuid_override, flushed_table_falls_back_to_hardware)
> +{
> +	struct child_ctx ctx;
> +	struct cpuid_regs r;
> +	pid_t pid;
> +
> +	ASSERT_EQ(write_cpuid_override(self->table), 0);
> +
> +	pid = spawn_cpuid_child(_metadata, self->cgv2_fd, self->ctid, &ctx);
> +
> +	ASSERT_EQ(write_cpuid_override("\n"), 0);
> +	ASSERT_EQ(write(ctx.go_pipe[1], "G", 1), 1);
> +	collect_cpuid_child(_metadata, pid, &ctx, &r);
> +
> +	EXPECT_EQ(r.eax, self->host.eax);
> +	EXPECT_NE(r.edx & self->bit, 0);

[Severity: Low]
Can EXPECT_EQ(r.eax, self->host.eax) fail spuriously on hybrid CPUs?

In this test the child's CPUID falls through to the real hardware, but
self->host was sampled by the parent in FIXTURE_SETUP(), and neither process
is pinned with sched_setaffinity(). If leaf 1 EAX (family/model/stepping)
differs between core types and the child runs on a different core type, the
comparison can fail without any kernel bug.

> +}
> +
> +TEST_HARNESS_MAIN

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-b4-prep-khorenko-vstor-148656-cpuid-v1-0-741609f4f03a@virtuozzo.com?part=1

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