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

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/ve: nvcsw baseline sampled too early to prove the child left the CPU
--

commit e70aeb0733728a60c8d4ac220677ea3eddf060e7
Author: Konstantin Khorenko <khorenko at virtuozzo.com>

selftests/ve: check CPUID override emulation for VE tasks

This 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
keeps the hardware values. A second test flushes the table after the task
was armed and expects the hardware values back. The child stops itself with
SIGSTOP so that CPUID faulting is armed at the next context switch, and the
parent waits on the child's voluntary context switch count before sending
SIGCONT.

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

[ ... ]

> +static int cpuid_child(struct child_ctx *ctx)
> +{
> +	struct cpuid_regs r;
> +	unsigned long nvcsw;
> +
> +	close(ctx->result_pipe[0]);
> +
> +	if (unshare(CLONE_NEWVE) < 0)
> +		return 21;
> +	if (read_nvcsw("/proc/self/status", &nvcsw))
> +		return 22;
> +	if (write(ctx->result_pipe[1], &nvcsw, sizeof(nvcsw)) != sizeof(nvcsw))
> +		return 23;
> +	if (raise(SIGSTOP))
> +		return 24;

[Severity: Medium]
Is the voluntary context switch baseline sampled early enough in
cpuid_child()?

The count is read by read_nvcsw() before the write() on the result pipe
and before raise(SIGSTOP):

    read_nvcsw("/proc/self/status", &nvcsw)
    write(ctx->result_pipe[1], &nvcsw, ...)
    raise(SIGSTOP)

If anything between the read and the stop sleeps voluntarily, for example
a page fault that blocks on IO in the first-use glibc write() or raise()
paths, nvcsw grows before the child actually stops.

[ ... ]

> +static void wait_switched_out(struct __test_metadata *_metadata, pid_t pid,
> +			      unsigned long nvcsw_before)
> +{
> +	char status[64];
> +	unsigned long nvcsw;
> +	int i;
> +
> +	snprintf(status, sizeof(status), "/proc/%d/status", pid);
> +	for (i = 0; i < SWITCH_WAIT_ROUNDS; i++) {
> +		ASSERT_EQ(read_nvcsw(status, &nvcsw), 0);
> +		if (nvcsw > nvcsw_before)
> +			return;

[Severity: Medium]
Can this condition tell the context switch caused by the stop apart from
an earlier voluntary switch?

The sequence would be:

    child:  read_nvcsw() -> baseline
    child:  voluntary sleep before raise(SIGSTOP), nvcsw grows
    parent: waitpid(WUNTRACED) returns on the stop
    parent: wait_switched_out() sees nvcsw > nvcsw_before, returns
    parent: kill(pid, SIGCONT) while the child is still in
            do_signal_stop() and has not left the CPU

The child would then resume without a context switch, so CPUID faulting,
which is switched on at the next context switch, is never armed.

In ve_sees_table_host_sees_hardware this would make the child report the
hardware EDX and the test would fail spuriously. In
flushed_table_falls_back_to_hardware the test would pass without proving
that faulting was armed, which is the false pass the commit message says
the first test is meant to rule out.

I checked the end of the series, including the commit "selftests/ve: check
CPUID override emulation for a prefixed CPUID", and the baseline is still
sampled before write() and raise(SIGSTOP). The later patches only add the
stop flag and more test cases, so this is not fixed there.

Would it work to sample nvcsw as the last step before raise(), or to also
require task state T in /proc/<pid>/stat before the parent sends SIGCONT?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261007-b4-prep-khorenko-vstor-148656-cpuid-v5-0-652c5c9e56da@virtuozzo.com?part=1