Re: [Devel] [PATCH hci-8.1 v5 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:
- [Low] selftests/ve: vtty stress test can hijack a live container's vtty pair via veid clash
- [Low] selftests/ve: kmsg_check() fails the test on any unrelated kernel warning
- [Low] selftests/ve: slave node on possibly-nodev /tmp gives hard failure instead of skip
--

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

selftests: ve: introduce vtty pair teardown stress test

This commit adds ve_vtty_stress, a selftest that opens and closes a vtty
pair from the master and slave ends at the same time. It checks the
kernel log for vtty count mismatches and for generic warnings, oopses and
lockups. The test is wired into the ve selftests Makefile and .gitignore.

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

[ ... ]

> +/* The slave end, inside the VE. Returns the errno that stopped it, or 0. */
> +static int hammer_slave(const char *node, time_t deadline,
> +			unsigned long *opened)
> +{
> +	int err = 0;
> +
> +	while (time(NULL) < deadline) {
> +		int fd = open(node, O_RDWR | O_NOCTTY | O_NONBLOCK);
> +
> +		if (fd >= 0) {
> +			(*opened)++;
> +			close(fd);
> +		} else if (!expected_errno(errno)) {
> +			err = errno;
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}

[ ... ]

> +static int kmsg_check(int fd)
> +{

[ ... ]

> +		if (strstr(buf, "WARNING: ") || strstr(buf, "Oops: ") ||
> +		    strstr(buf, ";BUG: ") || strstr(buf, "soft lockup - ") ||
> +		    strstr(buf, "general protection fault")) {
> +			ksft_print_msg("kernel log: %s", buf);
> +			found++;
> +		}
> +	}
> +	return found;
> +}

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

kmsg_open() seeks to the end of /dev/kmsg, and kmsg_check() then scans
every record logged anywhere in the kernel during the run. Only the
"tty->count" and "bad slave count" match is restricted to vtty.

The second condition is global:

    strstr(buf, "WARNING: ") || strstr(buf, "Oops: ") || ...

Any warning from another subsystem (a lockdep splat or a driver WARN, for
example) increments found. main() then reaches:

    ksft_exit_fail_msg("kernel complained %d time(s)\n", status);

On a busy node this gives a failure that has nothing to do with vtty.
Would it make sense to limit these matches to records that mention vtty,
or to the test's own tasks?

> +int main(int argc, char **argv)
> +{

[ ... ]

> +	ctid = make_ve(cg, CTID_MIN);
> +	if (ctid < 0)
> +		ksft_exit_skip("cannot make a VE cgroup\n");
> +
> +	/* 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) {
> +		destroy_ve(cg, ctid);
> +		ksft_exit_skip("cannot set ve.veid: %s\n", strerror(errno));
> +	}

[Severity: Low]
Can this test pick up a live container's vtty pair?

make_ve() picks the first cgroup directory name in the 108..199 range
that does not exist. That is a check on cgroup names, not on veids.

The chosen number is then written to ve.veid. ve_id_write() in
kernel/ve/ve.c has this comment and does no uniqueness check:

    /* FIXME: check veid is uniqul */

The vtty map is keyed by veid. A running container with the same veid
but a different cgroup name, such as a UUID-named cgroup, would
therefore be reached by open_master(). The test could then take over or
tear down that container's console pair and report spurious failures.

The commit message acknowledges this and says the range makes it
unlikely. Is there a way to check that the veid is unused before
writing it to ve.veid, or to skip the test when other containers are
running?

[ ... ]

> +	snprintf(node, sizeof(node), "%s/slave", dir);
> +	if (mknod(node, S_IFCHR | 0600, makedev(TTY_MAJOR, SLAVE_MINOR)) < 0) {
> +		status = errno;
> +		rmdir(dir);
> +		destroy_ve(cg, ctid);
> +		ksft_exit_fail_msg("mknod: %s\n", strerror(status));
> +	}

[Severity: Low]
Can a nodev /tmp turn this into a failure instead of a skip?

The node is created with mknod() under a mkdtemp() directory in /tmp.
On a host where /tmp is mounted nodev (systemd tmp.mount with
nosuid,nodev, for example), mknod() succeeds. The node is never
probe-opened before the fork.

In the child, hammer_slave() then gets EACCES from open(). Since
expected_errno() accepts only EBUSY, EIO and ENXIO, hammer_slave()
returns at once, and main() ends with:

    ksft_exit_fail_msg("the slave end stopped: %s\n", ...);

The test skips for the other environment problems, such as a missing
master. Should the node be probe-opened before the fork, with a skip on
EACCES, or be created on a filesystem that allows device nodes?

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

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