On Tue, May 10, 2022 at 8:43 AM Shuah Khan skhan@linuxfoundation.org wrote:
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.
I'll try to improve the description but IMHO it does describe what it's testing - the process_mrelease syscall with valid and invalid inputs. I could omit the implementation details if that helps.
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.
Ack.
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.
Ack.
return -1;+#endif
Key off of syscall return instead of these ifdefs - same comment on all of the ifdefs
Ack. I was using some other test as an example but I guess that was not a good model.
+}
I am not seeing any reason for breaking this code up have a separate routine for pidfd_open().
I'm a bit unclear what you mean. Do you mean that userspace headers should already define pidfd_open() and I don't need to define it?
+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.
Ack.
+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.
I guess the question is whether an OOM condition should be considered a test failure? I guess we could skip since the test is specifically for process_mrelease.
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?
Technically that should not happen because we kill the child and after that it has to reparent but to be safe I'll add a timeout to prevent an infinite loop.
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"
Ack.
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?
Ack. Although I though all FDs are closed implicitly when the process exits (which is the case in here).
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.
Ack.
/* 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?
We expect it to fail with EINVAL. We fail the test if the call succeeds or fails for any other reason.
}/* 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?
Expected to fail with EINVAL.
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.
I'll separate them into a stand-alone function.
if (kill(pid, SIGKILL)) {perror("kill");Include test results in the change log - so we can see the test report.
Ack. Although the test does not print any results on success, it just succeeds or fails with an error description. I'll add outputs for both cases.
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.
Ack.
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.
Will try to refactor it for a better readability. Thanks for the review!
thanks, -- Shuah