Re: [PATCH 1/3] selftests: vm: add process_mrelease tests

On 5/9/22 9:00 PM, Suren Baghdasaryan wrote:

Introduce process_mrelease syscall sanity tests. They include tests of invalid pidfd and flags inputs, attempting to call process_mrelease with a live process and a valid usage of process_mrelease. Because process_mrelease has to be used against a process with a pending SIGKILL, it's possible that the process exits before process_mrelease gets called. In such cases we retry the test with a victim that allocates twice more memory up to 1GB. This would require the victim process to spend more time during exit and process_mrelease has a better chance of catching the process before it exits.

+1 on Mike's comments on improving the change log. List what is getting tested as opposed to describing the test code.

Signed-off-by: Suren Baghdasaryan surenb@google.com

tools/testing/selftests/vm/Makefile | 1 + tools/testing/selftests/vm/mrelease_test.c | 176 +++++++++++++++++++++ tools/testing/selftests/vm/run_vmtests.sh | 16 ++ 3 files changed, 193 insertions(+) create mode 100644 tools/testing/selftests/vm/mrelease_test.c

Please update .gitignore with the new executable.

diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index 04a49e876a46..733fccbff0ef 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -43,6 +43,7 @@ TEST_GEN_FILES += map_populate TEST_GEN_FILES += memfd_secret TEST_GEN_FILES += mlock-random-test TEST_GEN_FILES += mlock2-tests +TEST_GEN_FILES += mrelease_test TEST_GEN_FILES += mremap_dontunmap TEST_GEN_FILES += mremap_test TEST_GEN_FILES += on-fault-limit diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c new file mode 100644 index 000000000000..a61061bf8433 --- /dev/null +++ b/tools/testing/selftests/vm/mrelease_test.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright 2022 Google LLC
• */

+#define _GNU_SOURCE +#include <errno.h> +#include <stdio.h> +#include <stdlib.h> +#include <sys/wait.h> +#include <unistd.h>

+#include "util.h"

+static inline int pidfd_open(pid_t pid, unsigned int flags) +{ +#ifdef __NR_pidfd_open

• return syscall(__NR_pidfd_open, pid, flags);

+#else

• errno = ENOSYS;

This isn't an error - this would be skip because this syscall isn't supported.

• return -1;

+#endif

Key off of syscall return instead of these ifdefs - same comment on all of the ifdefs

+}

I am not seeing any reason for breaking this code up have a separate routine for pidfd_open().

+static inline int process_mrelease(int pidfd, unsigned int flags) +{ +#ifdef __NR_process_mrelease

• return syscall(__NR_process_mrelease, pidfd, flags);

+#else

• errno = ENOSYS;
• return -1;

+#endif> +}

Same comments on ifdefs and skips here as well.

+static void write_fault_pages(char *addr, unsigned long nr_pages) +{

• unsigned long i;
• for (i = 0; i < nr_pages; i++)

• *((unsigned long *)(addr + (i * PAGE_SIZE))) = i;
  

+}

+static int alloc_noexit(unsigned long nr_pages, int pipefd) +{

• int ppid = getppid();
• void *buf;
• buf = mmap(NULL, nr_pages * PAGE_SIZE, PROT_READ | PROT_WRITE,

•    MAP_PRIVATE | MAP_ANON, 0, 0);
  

• if (buf == MAP_FAILED) {

• perror("mmap");
  

A bit more descriptive message what the test would do will be helpful. Also consider if this should be a skip or fail for the test.

• return 1;
  

• }
• write_fault_pages((char *)buf, nr_pages);
• /* Signal the parent that the child is ready */
• if (write(pipefd, "", 1) < 0) {

• perror("write");
  

• return 1;
  

• }
• /* Wait to be killed (when reparenting happens) */
• while (getppid() == ppid)

• sleep(1);
  

What happens if reparenting doesn't happen? Will this loop for ever? This test could hang?

• munmap(buf, nr_pages * PAGE_SIZE);
• return 0;

+}

+#define MB(x) (x << 20) +#define MAX_SIZE_MB 1024

+int main(void) +{

• int res;
• int pipefd[2], pidfd;
• pid_t pid;
• char byte;
• size_t size;
• int negative_tests_done = 0;
• /* Test a wrong pidfd */
• if (!process_mrelease(-1, 0) || errno != EBADF) {

• perror("process_mrelease with wring pidfd");
  

Incorrect spelling "wring/wrong"

• exit(1);
  

• }
• /*

• * Start the test with 1MB allocation and double every time
  

• * process_mrelease fails
  

• */
  

• for (size = 1; size <= MAX_SIZE_MB; size *= 2) {

• /*
  

•  * Pipe for the child to signal when it's done allocating
  

•  * memory
  

•  */
  

• if (pipe(pipefd)) {
  

• 	perror("pipe");
  

• 	exit(1);
  

• }
  

• pid = fork();
  

• if (pid < 0) {
  

• 	perror("fork");
  

Close the pipe?

• 	exit(1);
  

• }
  

• if (pid == 0) {
  

• 	close(pipefd[0]);
  

• 	res = alloc_noexit(MB(size) / PAGE_SIZE, pipefd[1]);
  

• 	close(pipefd[1]);
  

• 	exit(res);
  

• }
  

• close(pipefd[1]);
  

• /* Block until the child is ready */
  

• res = read(pipefd[0], &byte, 1);
  

• close(pipefd[0]);
  

• if (res < 0) {
  

• 	perror("read");
  

• 	exit(1);
  

• }
  

• pidfd = pidfd_open(pid, 0);
  

• if (pidfd < 0) {
  

• 	perror("pidfd_open");
  

• 	exit(1);
  

• }
  

The code is very hard to read. Add comments to indicate parent and child paths clearly so reviewers can follow the logic and be able to do effective review.

• /* Run negative tests which require a valid child only once */
  

• if (!negative_tests_done) {
  

• 	/* Test invalid flags */
  

• 	if (!process_mrelease(pidfd, (unsigned int)-1) ||
  

• 	    errno != EINVAL) {
  

• 		perror("process_mrelease with wrong flags");
  

• 		exit(1);
  

So is this an expected fail or a test fail?

• 	}
  

• 	/* Test reapling while process is still alive */
  

• 	if (!process_mrelease(pidfd, 0) ||
  

• 	    errno != EINVAL) {
  

• 		perror("process_mrelease on a live process");
  

So is this an expected fail or a test fail?

• 		exit(1);
  

• 	}
  

• 	negative_tests_done = 1;
  

• }
  

Now the above negative_tests_done block could be in a separate function --- All the others aren't really needed. It will be good for abstraction and readability.

• if (kill(pid, SIGKILL)) {
  

• 	perror("kill");
  

Include test results in the change log - so we can see the test report.

• 	exit(1);
  

• }
  

• if (!process_mrelease(pidfd, 0)) {
  

• 	/* Terminate the test once process_mrelease succeeds */
  

• 	return 0;
  

• }
  

• /*
  

•  * Ignore the failure if the child exited before mrelease got
  

•  * called, increase allocation size and retry the test
  

•  */
  

Add more info. on why allocating more memory helps.

• if (errno != ESRCH) {
  

• 	perror("process_mrelease");
  

• 	exit(1);
  

• }
  

• if (waitpid(pid, NULL, 0) < 0) {
  

• 	perror("waitpid");
  

• 	exit(1);
  

• }
  

• close(pidfd);
  

• }
• printf("All process_mrelease attempts failed!\n");
• exit(1);

+} diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh index 352ba00cf26b..1986162fea39 100755 --- a/tools/testing/selftests/vm/run_vmtests.sh +++ b/tools/testing/selftests/vm/run_vmtests.sh @@ -287,6 +287,22 @@ else echo "[PASS]" fi +echo "---------------------" +echo "running mrelease_test" +echo "---------------------" +./mrelease_test +ret_val=$?

+if [ $ret_val -eq 0 ]; then

• echo "[PASS]"

+elif [ $ret_val -eq $ksft_skip ]; then

• echo "[SKIP]"
  

• exitcode=$ksft_skip
  

+else

• echo "[FAIL]"
• exitcode=1

+fi

• echo "-------------------" echo "running mremap_test" echo "-------------------"

In general, the code flow is hard to read to make sure resources are released e.g: pipefd in all the error paths. The code is broken up into smaller chunks where it isn't needed in some cases and left as a large block when it could benefit from abstraction e.g: negative test block.

Please make changes and send v2.

thanks, -- Shuah