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

["Eva Kurchatova (Virtuozzo)" <eva.kurchatova@virtuozzo.com>]

On 10/7/26 01:25, no-reply@sashiko.virtuozzo.com wrote:
> 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?

Yes, the test run time is 30 seconds, which gives enough room to shut

down before the 45 second timeout, and I am not sure why would we

need a settings file to extend that. Whenever this test is invoked with

altered duration, it is expected to be ran manually for bisecting 
purposes, etc.

>> 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.
>
> [ ... ]

I would like to hear Konstantin Khorenko opinion, as I am not sure about 
this ioctl()

API stability or why it would be marked as unused. Either this patch 
un-obsoletes

that call, or a dedicated ioctl() for checking VEID state should be 
implemented.

>> +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?

The test intentionally considers any symptom of a buggy kernel a problem,

since memory corruption in the original issue can surface in various ways.

Sure, this can report a failure due to some unrelated kernel bug, but that

would be still a kernel bug that will be investigated and fixed. An overrun

of the kernel kmsg ring during 1-second interval is an enormous amount

of messages that can't appear under normal circumstances.

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