Re: [Devel] [PATCH hci-8.1 v7 2/2] selftests: ve: introduce vtty pair teardown stress test

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/ve: kmsg_check() fails on any unrelated kernel warning or log overrun
- [Low] selftests/ve: veid_in_use() relies on ENOKEY from an ioctl key marked unused
- [Low] selftests/ve: 30s default run has little margin under the 45s runner timeout
--

commit cfcd5cbc2bcb13ed42052dd5ac26511475fe149f
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

selftests: ve: introduce vtty pair teardown stress test

This adds ve_vtty_stress, a selftest that opens and closes a vtty pair from
the master end (vzctl ioctl) and the slave end (inside a VE) at the same
time. It also scans /dev/kmsg for warnings and splats while the test runs.

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

> 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

[ ... ]

> TEST_GEN_PROGS += ve_vtty_stress

[Severity: Low]
The ve selftest directory has no settings file, so the kselftest runner's
default per-test timeout applies. I believe that default is 45 seconds,
but I have not checked it in this tree.

The test runs for 30 seconds by default.

After that it blocks reading the child's count and then runs destroy_ve(),
which may retry for about a second. Setup also happens before the run.

Is there enough margin on a slow or loaded host? A timeout kill would be
reported as a failure that has nothing to do with vtty.

A larger VE_VTTY_SECS or argv[1] value would exceed the default timeout
unless the runner is configured otherwise. The commit message says the
default stays "under the timeout of the suite". Could a settings file be
added to back that up?

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

[ ... ]

> +/* Any key but the tty one makes ve_configure() look a running VE up */
> +static bool veid_in_use(int ctl_fd, unsigned int veid)
> +{
> +	struct vzctl_ve_configure c = {
> +		.veid = veid,
> +		.key = VE_CONFIGURE_OS_RELEASE,
> +	};
> +
> +	return ioctl(ctl_fd, VZCTL_VE_CONFIGURE, &c) < 0 && errno == ENOKEY;
> +}

[Severity: Low]
VE_CONFIGURE_OS_RELEASE is defined as "2 /* unused */" in
include/uapi/linux/vzcalluser.h. In kernel/ve/vecalls.c, ve_configure()
behaves like this:

    if (key == VE_CONFIGURE_OPEN_TTY)
            return vtty_open_master(veid, val);

    ve = get_ve_by_id(veid);
    if (!ve)
            return -EINVAL;

    put_ve(ve);
    return err;    /* err is -ENOKEY */

So veid_in_use() treats ENOKEY as "a running VE holds this veid". Is it
safe to depend on the return code of a key that is documented as unused?

If that key is later rejected earlier, for example with -ENOTTY or -EINVAL,
veid_in_use() will always return false. The only guard against taking over
a running container's vtty pair would then disappear without any notice.

Also, a VE that has ve.veid set but is not started is not in ve_idr, so the
probe cannot see it. The commit message only mentions "one started later"
as the case that is not seen.

[ ... ]

> +static int kmsg_check(int fd)
> +{
> +	/* How a warning, a splat or a sanitizer report starts in a record */
> +	static const char *const reports[] = {
> +		"WARNING: ", "Oops", ";BUG", "kernel BUG at", "UBSAN: ",
> +		"soft lockup - ", "general protection fault", "INFO: task ",
> +		"rcu: INFO: ",
> +	};

[ ... ]

> +	for (;;) {
> +		r = read(fd, buf, sizeof(buf) - 1);
> +		if (r < 0) {
> +			/* The ring wrapped since the last read, records are gone */
> +			if (errno == EPIPE) {
> +				ksft_print_msg("kernel log overrun, records lost\n");
> +				found++;
> +				continue;
> +			}

[ ... ]

> +		for (i = 0; i < ARRAY_SIZE(reports); i++) {
> +			if (!strstr(buf, reports[i]))
> +				continue;
> +			ksft_print_msg("kernel log: %s", buf);
> +			found++;
> +			break;
> +		}
> +	}
> +	return found;
> +}

[Severity: Medium]
Only the "vtty" checks in kmsg_check() are tied to vtty. The reports[]
patterns are matched with strstr() anywhere in a record. kmsg_open() seeks
to the end of /dev/kmsg, so every record from any subsystem is read for the
whole run.

Can a warning or hung-task report from an unrelated subsystem, or from a
test running at the same time on a shared CI host, fail this test even when
the vtty code is correct?

The EPIPE branch counts a ring overrun as a complaint. If other log traffic
overruns the ring, does the verdict then depend on host log volume instead
of on vtty behaviour?

The log is drained only about once per second from the master loop. The
final message "kernel complained %d time(s)" does not say that the cause
may be unrelated to vtty.

The commit message accepts the overrun rule deliberately. Could the
matching be narrowed, or the failure message made to say that the report
may be unrelated?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006221704.2507107-1-eva.kurchatova@virtuozzo.com?part=2

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