Re: [Devel] [PATCH hci-8.1 v2 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 3 potential issue(s) to consider:
- [Medium] selftests/ve: restore of saved cpuid_override table fails for tables of 4096 bytes or more
- [Low] selftests/ve: stopped child is never killed or reaped when an ASSERT fires
- [Low] selftests/ve: Makefile overwrites global ARCH, breaking native LLVM=1 builds on non-x86
--

commit 6db1939d0f0292549e9feb0b4de5043c4eac6ba7
Author: Konstantin Khorenko <khorenko@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
sees the hardware values. A second test flushes the table after the task
was armed and expects the hardware values again. The test is built on x86
only.

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
> @@ -6,4 +6,11 @@ TEST_GEN_PROGS += ve_ns_owner_test
>  TEST_GEN_PROGS += ve_perms_test
>  TEST_GEN_PROGS += ve_mount_accounting_test
>  
> +ifeq ($(ARCH),)
> +ARCH := $(shell uname -m 2>/dev/null || echo not)
> +endif
> +ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64))
> +TEST_GEN_PROGS += ve_cpuid_override_test
> +endif
> +
>  include ../lib.mk

[Severity: Low]
Can setting ARCH here break native LLVM=1 builds on non-x86 hosts?

The block assigns ARCH from uname -m before include ../lib.mk.  In lib.mk
the clang target selection depends on ARCH being empty:

    ifeq ($(ARCH),)
    CLANG_TARGET_FLAGS := $(shell $(CLANG) -print-target-triple)
    else
    CLANG_TARGET_FLAGS := $(CLANG_TARGET_FLAGS_$(ARCH))
    endif

With "make LLVM=1 -C tools/testing/selftests/ve" on a host such as aarch64
or ppc64le, with ARCH and CROSS_COMPILE unset, ARCH is now non-empty and
CLANG_TARGET_FLAGS_$(ARCH) is empty.  lib.mk then hits "$(error Specify
CROSS_COMPILE or add '--target=' option to lib.mk)", so
ve_ns_owner_test, ve_perms_test and ve_mount_accounting_test also stop
building.

Would a private variable such as uname_M be better than overwriting ARCH?
The series-end diff of tools/testing/selftests/ve/ shows no later change to
this Makefile.

> 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

[ ... ]

> +/* 128 entries of up to 67 characters each, as the kernel prints them. */
> +#define CPUID_TABLE_MAX		16384

[ ... ]

> +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));
> +
> +	cgv2_fd = mount_cg2_fd();
> +	if (cgv2_fd < 0)
> +		return;
> +	destroy_ve(cgv2_fd, self->ctid);
> +	close(cgv2_fd);
> +}

[Severity: Medium]
Can the restore of the saved table fail for large tables?

FIXTURE_SETUP reads the printed table into the 16384-byte saved[] buffer
and accepts anything below 16383 bytes.  FIXTURE_TEARDOWN_PARENT then
writes it back with a single write() through write_cpuid_override().

In arch/x86/kernel/cpuid_fault.c, cpuid_override_write() starts with:

    err = -E2BIG;
    if (count >= PAGE_SIZE)
            goto out;

A table that prints to 4096 bytes or more (roughly 61 or more entries at
about 67 characters each) is read fine but rejected with -E2BIG on
restore.  Such a table can only have been loaded by writing it in chunks.

Teardown only logs the failure with TH_LOG, so the host-wide table stays
replaced after the test and later VE tasks see the wrong leaf 1 values.
That also contradicts the commit message statement "restore the previous
table on exit".  Should the restore write the table in chunks below
PAGE_SIZE, or should setup skip when the saved table is too large to
restore?  The later patches in the series do not change the restore logic.

[Severity: Low]
Can a stopped child be left behind when an ASSERT fires?

FIXTURE_TEARDOWN_PARENT neither kills nor reaps the child, and the pid
returned by spawn_cpuid_child() is not kept in the fixture.  The child has
no PR_SET_PDEATHSIG.

> +	pid = spawn_cpuid_child(_metadata, self->cgv2_fd, self->ctid, &ctx);
> +
> +	ASSERT_EQ(write_cpuid_override("\n"), 0);
> +	resume_cpuid_child(_metadata, pid);
> +	collect_cpuid_child(_metadata, pid, &ctx, &r);

If ASSERT_EQ(write_cpuid_override("\n"), 0) fails, or the kill(SIGCONT)
ASSERT in resume_cpuid_child() fails, or any ASSERT in
collect_cpuid_child() fails, the test body returns before the child is
resumed and waited for.

The child stays SIGSTOPped inside the VE cgroup and is reparented to init.
Then destroy_ve() in FIXTURE_TEARDOWN_PARENT fails with EBUSY because the
child is still in the cgroup.

Could the pid be stored in the fixture so teardown can kill and reap it
before destroy_ve()?  The later patches add more callers of
spawn_cpuid_child() but do not add any such cleanup.

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

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