Re: [Devel] [PATCH hci-8.1 v3 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 1 potential issue(s) to consider:
- [Low] selftests/ve: stale self->child PID is killed in teardown after early reap
--

commit 2e6583525965f296409edf5f5ba686bf9a7c7bce
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check CPUID override emulation for VE tasks

This commit adds a selftest for /proc/vz/cpuid_override. It checks that a
task in a fresh VE sees the override table values while the host sees the
hardware values. It also checks that a task armed before the table is
flushed falls back to the real CPUID afterwards.

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

> 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
> index 000000000000..0e3537099ddb
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_cpuid_override_test.c

[ ... ]

> +FIXTURE_TEARDOWN_PARENT(ve_cpuid_override)
> +{
> +	int cgv2_fd;
> +
> +	if (write_cpuid_override(self->saved[0] ? self->saved : "\n"))
> +		TH_LOG("could not restore %s: %s", CPUID_OVERRIDE_PATH,
> +		       strerror(errno));
> +
> +	/* A child left stopped by a failed ASSERT keeps the VE cgroup busy. */
> +	if (self->child > 0)
> +		kill(self->child, SIGKILL);

[Severity: Low]
Can this kill() hit a PID that no longer belongs to the test child?

In spawn_cpuid_child(), self->child is set right after fork():

	self->child = pid;
	...
	ASSERT_GE(waitpid(pid, &status, WUNTRACED), 0);
	if (WIFEXITED(status))
		TH_LOG("child exited with %d before stopping",
		       WEXITSTATUS(status));
	ASSERT_TRUE(WIFSTOPPED(status));

If the child exits or is killed before it stops (for example cpuid_child()
returning 21, 22 or 24), waitpid() reaps it. ASSERT_TRUE(WIFSTOPPED(status))
then fails and aborts the test, but self->child is not cleared on that path.

self->child is only cleared in collect_cpuid_child(), which this path never
reaches. The teardown runs in the parent and sees the stale non-zero
self->child. It then calls kill(self->child, SIGKILL) on a PID that has
already been reaped and could have been reused by an unrelated process.

Would it make sense to clear self->child in spawn_cpuid_child() once
waitpid() has reaped the child, before the ASSERT_TRUE(WIFSTOPPED(status))
check?

[ ... ]

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

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