Messages in this thread | | | Subject | Re: [PATCH 1/3] selftests: vm: add process_mrelease tests | From | Shuah Khan <> | Date | Tue, 10 May 2022 09:43:28 -0600 |
| |
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
| |