Re: [Devel] [PATCH hci-8.1 8/8] selftests/mm: test memory preservation across exec

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] selftests/mm: doexec child diagnostics are lost by _exit()/exec
- [Low] selftests/mm: doexec ignores write() return values
- [Low] selftests/mm: doexec treats any SIGSEGV in allocation sweep as expected
--

commit eee32e73516b760bb3007e1b97633a4999581fec
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

selftests/mm: test memory preservation across exec

This commit adds doexec, a selftest for MADV_DOEXEC. It checks that memory
backing long-term GUP pins and other mappings survives repeated exec and
fork. It also covers CLONE_VM handling, threads, /proc readers and an
optional allocation-failure sweep. The test is wired into run_vmtests.sh.

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

> diff --git a/tools/testing/selftests/mm/doexec.c b/tools/testing/selftests/mm/doexec.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/mm/doexec.c

[ ... ]

> +static void exec_stage(int fd, char *ram, size_t size, int kind, int stage)
> +{
> +	char fd_arg[32], addr_arg[32], size_arg[32], kind_arg[32], stage_arg[32];
> +
> [ ... ]
> +	execl("/proc/self/exe", "doexec", "--exec", fd_arg, addr_arg, size_arg,
> +	      kind_arg, stage_arg, NULL);
> +	ksft_print_msg("exec: %s\n", strerror(errno));
> +	_exit(KSFT_FAIL);
> +}

[Severity: Low]
Can the diagnostics printed by the forked children get lost here?

ksft_print_msg() writes through buffered stdio. When stdout is a pipe or a
file, as under run_vmtests.sh or a CI harness, the "exec: ..." message is
still in the buffer when _exit(KSFT_FAIL) runs, and _exit() does not flush
it.

The same applies to the other messages printed from the per-test children,
for example:

    before_exec()  -> "pin: ...", "MADV_DOEXEC: ..."
    check_pin()    -> "read pin: ...", "CPU/pin mismatch at byte N"
    allocation_failures() -> "fail-nth N: ..."

In main(), the fflush(stdout) calls run in the parent before fork(). They
prevent duplicated output but do not flush what the child prints itself.
Messages printed before the execl() into the --exec stage are likewise
discarded by the exec.

A failing run would then show only a bare "not ok" line. Would an
fflush(stdout) before each _exit() and execl() in the children be
appropriate?

> +static int shared_mm_exec(bool own_thread)
> +{

[ ... ]

> +	if (own_thread) {
> +		if (pthread_create(&thread, NULL, blocked_thread, &pipe_fd[0]))
> +			return KSFT_FAIL;
> +		check_exec(ram, size, 1, false);
> +		write(pipe_fd[1], "x", 1);
> +		pthread_join(thread, NULL);
> +		return KSFT_FAIL;
> +	}

[ ... ]

> +	write(pipe_fd[1], "x", 1);
> +	if (wait_child(pid))
> +		ret = KSFT_FAIL;

[Severity: Low]
Is it intended that the write() results are ignored here?

write() is warn_unused_result under glibc _FORTIFY_SOURCE, so this can
produce -Wunused-result warnings.

If this write ever failed, the CLONE_VM child or thread would stay blocked
in read(), and wait_child() or pthread_join() would hang because the test
has no timeout.

> +static int clone_race_child(void *arg)
> +{
> +	struct clone_race *race = arg;
> +	unsigned int i;
> +
> +	for (i = 0; i < 8; i++) {
> +		if (__atomic_load_n(race->ram, __ATOMIC_RELAXED) != 0x31) {
> +			write(race->report, "x", 1);
> +			return KSFT_FAIL;
> +		}
> +		usleep(1000);
> +	}
> +	return KSFT_PASS;
> +}

[Severity: Low]
Should the result of this write() be checked as well?

This write is the only signal that the shared memory was stolen. If it fails,
clone_exec_race() sees EOF on the report pipe and passes despite the detected
failure. clone_race_thread() and the --check handler in main() also drop
write() results.

> +	if (argc == 5 && !strcmp(argv[1], "--check")) {
> +		char *ram = (char *)(uintptr_t)strtoull(argv[2], NULL, 10);
> +		size_t size = strtoull(argv[3], NULL, 10);
> +		size_t stride = strtoull(argv[4], NULL, 10), i;
> +		int fd = open("/proc/self/fail-nth", O_WRONLY);
> +
> +		if (fd >= 0) {
> +			write(fd, "0", 1);
> +			close(fd);
> +		}
> +		for (i = 0; i < size; i += stride)
> +			if (ram[i] != 0x31)
> +				return KSFT_FAIL;
> +		return KSFT_PASS;
> +	}

[ ... ]

> +		if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV)
> +			fatal++;

[ ... ]

> +	/* A sweep that injected no recoverable/fatal errors provides no coverage. */
> +	return early && fatal ? KSFT_PASS : KSFT_SKIP;
> +}

[Severity: Low]
Can the allocation_failures() sweep pass even when the preserved mapping has
been lost?

do_execveat_common() deliberately raises SIGSEGV when exec fails after the
point of no return:

    if (bprm->point_of_no_return && !fatal_signal_pending(current))
            force_fatal_sig(SIGSEGV);

So SIGSEGV is a legitimate outcome of the sweep. However, the test cannot
tell that kernel-forced SIGSEGV apart from a user-space fault.

After a successful preserving exec, the --check image dereferences ram[i]
in main(). If a kernel bug dropped or unmapped the preserved range, that load
would also raise SIGSEGV. allocation_failures() would count it as fatal++.

The final check is "early && fatal ? KSFT_PASS : KSFT_SKIP", and it requires
at least one SIGSEGV. A regression that loses the mapping can therefore help
the test pass.

Is there a way to distinguish the two cases? One option is to have the --check
image report a lost mapping without faulting, for example by probing the range
with mincore() or msync() before reading it.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261014-b4-prep-khorenko-vstor-149847-2-doexec-pinned-memory-v1-0-36f1120f9b57@virtuozzo.com?part=8

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