Re: [Devel] [PATCH hci-8.1 v6 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() ignores EPIPE ring overrun and can falsely PASS
- [Low] selftests/ve: kmsg_check() fails on any unrelated kernel WARNING/Oops/BUG
- [Low] selftests/ve: veid uniqueness check is a TOCTOU, so the stress may hit a live container
--

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

selftests: ve: introduce vtty pair teardown stress test

This adds a selftest that opens and closes a vtty pair from both the
master end (vzctl ioctl) and the slave end (inside a VE) for a
configurable time. It then scans /dev/kmsg for vtty count mismatches and
generic kernel warnings, to catch the use-after-free fixed in c5028cd61a69.

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

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

[ ... ]

> +static int kmsg_check(int fd)
> +{
> +	char buf[8192];
> +	int found = 0;
> +	ssize_t r;
> +
> +	if (fd < 0)
> +		return 0;
> +
> +	for (;;) {
> +		r = read(fd, buf, sizeof(buf) - 1);
> +		if (r < 0) {
> +			/* Ring overrun, continue with the oldest existing record */
> +			if (errno == EPIPE)
> +				continue;
> +			break;
> +		}

[Severity: Medium]
Can this test report PASS even though the kernel logged the bug it is
meant to detect?

kmsg_check() is called only once, after the whole run, so /dev/kmsg is
drained after up to 30 seconds of logging. If the log ring wraps during
that time, the next read() returns EPIPE, and the branch below silently
resumes from the oldest surviving record:

    if (errno == EPIPE)
            continue;

Records emitted early in the run, such as a "bad slave count" message or
a WARNING from a double release_tty(), would be dropped without being
counted. Nothing tells the user that records were lost.

Should the EPIPE case at least print a message that records were lost, or
make the test fail or skip?

> +		if (r == 0)
> +			break;
> +		buf[r] = '\0';
> +		/* Known symptoms of a buggy kernel */
> +		if (strstr(buf, "vtty") && (strstr(buf, "tty->count") ||
> +					    strstr(buf, "bad slave count"))) {
> +			ksft_print_msg("vtty mismatched count log: %s", buf);
> +			found++;
> +		}
> +		if (strstr(buf, "WARNING: ") || strstr(buf, "Oops: ") ||
> +		    strstr(buf, ";BUG: ") || strstr(buf, "soft lockup - ") ||
> +		    strstr(buf, "general protection fault")) {

[Severity: Low]
Can an unrelated kernel message fail this test?

kmsg_open() seeks to the end of /dev/kmsg, and kmsg_check() then reads
every record logged until the end of the run. Any record from another
subsystem that contains "WARNING: ", "Oops: ", ";BUG: ", "soft lockup - "
or "general protection fault" is counted in found.

main() then calls ksft_exit_fail_msg("kernel complained %d time(s)\n", ...).
Only the "tty->count" and "bad slave count" checks are restricted to vtty.

On a busy host, wouldn't this make the result depend on unrelated activity?
Would it be possible to filter by something related to the vtty or tty
code, such as the call trace?

> +			ksft_print_msg("kernel log: %s", buf);
> +			found++;
> +		}
> +	}
> +	return found;
> +}

[ ... ]

> +	ctid = make_ve(cg, CTID_MIN);
> +	if (ctid < 0)
> +		ksft_exit_skip("cannot make a VE cgroup\n");
> +
> +	/* The vtty map is keyed by veid, and ve.veid takes a taken one */
> +	if (veid_in_use(ctl, ctid)) {
> +		destroy_ve(cg, ctid);
> +		ksft_exit_skip("veid %d is in use by a running container\n",
> +			       ctid);
> +	}
> +
> +	/* The ve.veid must match between master / slave */
> +	snprintf(path, sizeof(path), "%d/ve.veid", ctid);
> +	snprintf(val, sizeof(val), "%d", ctid);
> +	if (write_file_at(cg, path, val) < 0) {

[Severity: Low]
Is the veid uniqueness check in main() racy?

veid_in_use() queries only running VEs, and ve.veid is written afterwards.
ve_id_write() in kernel/ve/ve.c carries "/* FIXME: check veid is uniqul */"
and does not reserve the id. The test's own VE enters the idr only later,
after the namespace switch.

If a container with the same veid is started between veid_in_use() and the
write of ve.veid, would the test not detect it? It could then drive that
container's vtty pair 0 from both ends.

The commit message says "Before writing ve.veid the test asks the kernel
... whether the veid is taken, and skips when it is." This describes the
check as a full safeguard, but it only narrows the window. Could the
commit message or a code comment say that the check is best-effort?

[ ... ]

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

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