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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/ve: kmsg_check() fails on any unrelated host log record or ring overrun
- [Low] selftests/ve: master loop ignores early exit of slave child in main()
- [Low] selftests/ve: deadline uses 1s-granularity time(NULL) in main() and hammer_slave()
- [Low] selftests/ve: wait_child() leaves status 0 when waitpid() fails non-EINTR
- [Low] selftests/ve: stop flag check races with blocking waitpid()/read() in main()
--

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

selftests: ve: introduce vtty pair teardown stress test

This adds a selftest that opens and closes both ends of a vtty pair
concurrently, to exercise teardown ownership and thread safety. The
slave end runs in a forked child inside a VE, the master end is opened
via the vzctl ioctl, and the kernel log is scanned for splats.

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..f4bdd81133b8
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_vtty_stress.c

[ ... ]

> +/* The handler interrupts a wait, the child is still to be reaped */
> +static void wait_child(pid_t pid, int *status)
> +{
> +	*status = 0;
> +	do {
> +		if (stop)
> +			kill(pid, SIGKILL);
> +	} while (waitpid(pid, status, 0) < 0 && errno == EINTR);
> +}

[Severity: Low]
If waitpid() fails with an error other than EINTR (for example ECHILD),
the loop exits with *status still preset to 0. Callers then see
WIFEXITED(0) true and WEXITSTATUS(0) equal to 0, so slave_err is computed
as 0 and the child is treated as having exited cleanly. Could the
failure be reported to the caller instead of leaving a status that reads
as a clean exit? This path is unlikely, since the child is the parent's
own fork.

[Severity: Low]
Is the check of stop atomic with the blocking waitpid()? If SIGTERM or
SIGINT arrives after the "if (stop)" test but before waitpid() is
entered, the handler (installed with sa_flags = 0) sets stop but nothing
interrupts the wait. The parent then blocks until the child reaches its
own deadline. This is a delay bounded by secs, but if the runner
escalates to SIGKILL in the meantime, the cgroup, the mknod node and the
tmp directory are left behind. Would blocking the signals and using
ppoll()/pselect(), signalfd or a pidfd close this window?

[ ... ]

> +/*
> + * Check for kernel log warnings, one read() per record, hence
> + * the function name is not checked.
> + */
> +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]
Can this give wrong results in both directions? kmsg_open() seeks to the
end of /dev/kmsg, and kmsg_check() then scans every later record on the
host, matching generic substrings such as "WARNING: ", "Oops",
"INFO: task ", "rcu: INFO: " and "soft lockup - " with strstr() anywhere
in the record. There is no filtering by vtty, by tty, or by the pid of
the test.

A warning from an unrelated subsystem or container during the run is
therefore counted as a vtty complaint, and main() lets a complaint
outrank how the run ended. An EPIPE ring overrun is also counted as a
failure (found++), and since the log is drained only about once per
second in main(), a chatty host can overrun it.

In the other direction, without KASAN or a similar detector, a
use-after-free or a double release_tty() that logs nothing passes
silently, because the log scan is the only detector. Could the matching
be narrowed to vtty-related records, and could an overrun be treated
differently from a vtty failure?

[ ... ]

> +	deadline = time(NULL) + secs;

[Severity: Low]
Is a 1 second granularity deadline intended here? time(NULL) truncates
to whole seconds, so with VE_VTTY_SECS=1 the stress window can be almost
empty. The parent in main() and the child in hammer_slave() also
evaluate "time(NULL) < deadline" independently and stop at slightly
different instants, and time() is subject to wall-clock steps. Would
clock_gettime(CLOCK_MONOTONIC) be a better fit?

[ ... ]

> +	while (!stop && (now = time(NULL)) < deadline) {
> +		int fd = open_master(ctl, veid);
> +
> +		if (fd >= 0) {
> +			master_opened++;
> +			close(fd);
> +		} else if (!expected_errno(errno)) {
> +			master_err = errno;
> +			break;
> +		}

[Severity: Low]
Does this master loop notice when the slave child has already exited?
hammer_slave() returns on the first unexpected errno (for example EPERM
or EACCES from a device policy), and the child then writes its count and
exits. The loop condition only checks stop and the deadline, so the
master keeps hammering alone until the deadline, and only afterwards does
main() read count[0] and call wait_child().

In a denied-slave environment this means the full 30 second default
passes before the SKIP is reported, and a real slave-side failure is only
reported at the end, with the rest of the run exercising only one end of
the pair. Would polling the child with waitpid(WNOHANG), or poll() on
count[0], inside this loop help?

[ ... ]

> +	} else {
> +		if (read(count[0], &slave_opened, sizeof(slave_opened)) !=
> +		    sizeof(slave_opened))
> +			slave_opened = 0;
> +		wait_child(pid, &status);
> +		slave_err = WIFEXITED(status) ? WEXITSTATUS(status) : EINTR;
> +	}

[Severity: Low]
Is the same window present here? The blocking read(count[0], ...) has no
check of stop that is atomic with it, so a SIGTERM or SIGINT delivered
after the master loop exits but before read() is entered is not noticed
until the child finishes at its own deadline.

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

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