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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/mm: vhost_owner_exec() closes the fd, so VHOST_RESET_OWNER is not tested
- [Low] selftests/mm: doexec binary missing from .gitignore
- [Low] selftests/mm: doexec.c ELF note asm uses @note, breaks on ARM
- [Low] selftests/mm: doexec.c ignores write()/pwrite() return values
- [Low] selftests/mm: doexec THP and mlock cases never verify THP or VM_LOCKED after exec
--

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

selftests/mm: test memory preservation across exec

This adds a selftest, doexec, that checks MADV_DOEXEC mappings keep their
backing pages across repeated exec and a later fork. It covers long-term
GUP pins, CLONE_VM rejection and retry, proc readers, vhost owner reset,
and an optional allocation-failure sweep. The test is built through
TEST_GEN_FILES and added to run_vmtests.sh.

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

> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -74,6 +74,7 @@ endif
>
>  TEST_GEN_FILES = cow
>  TEST_GEN_FILES += compaction_test
> +TEST_GEN_FILES += doexec

[Severity: Low]
This isn't a bug, but the new generated binary has no entry in
tools/testing/selftests/mm/.gitignore, where siblings such as
gup_longterm are listed.

Should doexec be added there so it does not show up as untracked in git
status after an in-tree build?

>  TEST_GEN_FILES += gup_longterm
>  TEST_GEN_FILES += gup_test
>  TEST_GEN_FILES += hmm-tests

[ ... ]

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

[ ... ]

> +/* The downstream ELF loader requires this opt-in in a PT_NOTE segment. */
> +asm(".pushsection .note.doexec,\"a\",@note\n"
> +	".balign 4\n"

[Severity: Low]
Will this assemble on 32-bit ARM?

GNU as on ARM treats '@' as a comment character, so the section type
there has to be spelled %note. Since the Makefile adds doexec to
TEST_GEN_FILES with no architecture guard, an ARM build of the mm selftests
could either fail or produce a section that is not SHT_NOTE, so the loader
opt-in would not be recognised.

Would a portable spelling, or an architecture guard, be more suitable?

[ ... ]

> +	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]
Several write() and pwrite() return values are ignored: here in
shared_mm_exec(), and also in clone_race_child(), clone_race_thread(), the
--check handler in main(), and allocation_failures(). These will trigger
glibc warn_unused_result warnings.

In shared_mm_exec(), the write() above is what wakes the CLONE_VM task
blocked in read() inside blocked_task(). If it failed, wouldn't the
following wait_child() block forever?

Could the return values be checked?

[ ... ]

> +/* The caller must release its backend owner before preserving exec. */
> +static int vhost_owner_exec(void)
> +{
> +	size_t size = sysconf(_SC_PAGESIZE);
> +	char *ram;
> +	int fd = open("/dev/vhost-net", O_RDWR);
> +
> +	if (fd < 0)
> +		return KSFT_SKIP;
> +	ram = preserved_ram(size);
> +	if (!ram)
> +		return errno == EINVAL ? KSFT_SKIP : KSFT_FAIL;
> +	if (ioctl(fd, VHOST_SET_OWNER))
> +		return KSFT_SKIP;
> +	if (ioctl(fd, VHOST_RESET_OWNER))
> +		return KSFT_FAIL;
> +	close(fd);
> +	check_exec(ram, size, 1, false);
> +	return KSFT_FAIL;
> +}

[Severity: Medium]
Does this test actually exercise VHOST_RESET_OWNER? The fd is closed before
the exec, and close() releases the owner's mm through the same cleanup
path:

vhost_net_release()
  vhost_dev_cleanup(&n->dev)
    vhost_detach_mm()

VHOST_RESET_OWNER reaches the same function:

vhost_net_reset_owner()
  vhost_dev_reset_owner()
    vhost_dev_cleanup()

So the exec sees the same state whether or not the reset ioctl worked, and
a broken reset-owner would go unnoticed, despite the subtest name "vhost
owner: reset before preserving exec".

Could the exec be done with the fd still open after the reset? It would
also be useful to check that the exec is refused while the owner is still
set, so the rejection side is covered too.

[ ... ]

> +static int after_exec(int fd, char *ram, size_t size, int kind, int stage)
> +{
> +	unsigned char before = stage == 1 ? 0x31 : 0x42;
> +	unsigned char after = stage == 1 ? 0x42 : 0x53;
> +	pid_t pid;
> +
> +	if (check_pin(fd, ram, size, before))
> +		return KSFT_FAIL;
> +	if (kind == PROTECTED && mprotect(ram, size, PROT_READ | PROT_WRITE))
> +		return KSFT_FAIL;
> +	memset(ram, after, size);
> +	if (check_pin(fd, ram, size, after))
> +		return KSFT_FAIL;

[ ... ]

> +	if (kind == HUGE && madvise(ram, size, MADV_COLLAPSE))
> +		return KSFT_SKIP;
> +	if (kind == LOCKED && mlock(ram, size))
> +		return KSFT_SKIP;

[Severity: Low]
Do the "PMD huge pages" and "mlocked pages" subtests verify the property
they are named for? The THP collapse and the mlock() are done only in
before_exec(). After exec, after_exec() looks at kind only for PROTECTED,
and otherwise compares bytes and pin contents.

A regression that splits the PMD THP or drops VM_LOCKED during the
preserving exec would still report PASS.

Could after_exec() check AnonHugePages in /proc/self/smaps and VmLck in
/proc/self/status for these two kinds?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-1901cd6c3c74@virtuozzo.com?part=4

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