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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 10/7/26 00:42, Eva Kurchatova (Virtuozzo) wrote:
> 
> 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.


Timeout: agreed, no settings file. The run is a fixed wall-clock deadline, a slow kernel does not stretch it.


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

  No ioctl, old or new. Just do not check at all.

      name="vtty-$(uuidgen)"
      prlctl create "$name" --vmtype ct --ostemplate almalinux-10-x86_64
      prlctl start "$name"

  This is how a container gets a unique id in real life. Take a random
  veid in the test the same way and drop the probe.

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

kmsg part is fine, leave it.

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