| Message ID | 20191009160532.20674-2-ckellner@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Oct 9, 2019 at 6:10 PM Christian Kellner <ckellner@redhat.com> wrote: > Add tests that check that if pid namespaces are configured the fdinfo > file of a pidfd contains an NSpid: entry containing the process id > in the current and additionally all nested namespaces. [...] > +static int compare_fdinfo_nspid(int pidfd, char *expect, size_t len) > +{ > + char path[512]; > + FILE *f; > + size_t n = 0; > + ssize_t k; > + char *line = NULL; > + int r = -1; > + > + snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", pidfd); (Maybe at some point the selftests code should add some more concise alternative to snprintf() calls on separate lines. A macro or something like that so that you can write stuff like `f = fopen(tprintf("/proc/self/fdinfo/%d", pidfd), "re")`.) > + f = fopen(path, "re"); > + if (!f) > + return -1; > + > + while ((k = getline(&line, &n, f)) != -1) { > + if (strncmp(line, "NSpid:", 6)) > + continue; > + > + line[k - 1] = '\0'; > + ksft_print_msg("Child: fdinfo NSpid line: '%s'.\n", line); > + r = strncmp(line + 6, expect, len); Wouldn't it be better to get rid of the nullbyte assignment and change the strncmp() into a strcmp() here... [...] > + /* The child will have pid 1 in the new pid namespace, > + * so the line must be 'NSPid:\t<pid>\t1' > + */ > + n = snprintf(expect, sizeof(expect), "\t%d\t%d", pid, 1); ... and add a "\n" to the format string? It's shorter and doesn't silently ignore it if the line doesn't end at that point.
On Fri, Oct 11, 2019 at 05:09:29PM +0200, Jann Horn wrote: > On Wed, Oct 9, 2019 at 6:10 PM Christian Kellner <ckellner@redhat.com> wrote: > > Add tests that check that if pid namespaces are configured the fdinfo > > file of a pidfd contains an NSpid: entry containing the process id > > in the current and additionally all nested namespaces. > [...] > > +static int compare_fdinfo_nspid(int pidfd, char *expect, size_t len) > > +{ > > + char path[512]; > > + FILE *f; > > + size_t n = 0; > > + ssize_t k; > > + char *line = NULL; > > + int r = -1; > > + > > + snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", pidfd); > > (Maybe at some point the selftests code should add some more concise > alternative to snprintf() calls on separate lines. A macro or > something like that so that you can write stuff like `f = > fopen(tprintf("/proc/self/fdinfo/%d", pidfd), "re")`.) > > > + f = fopen(path, "re"); > > + if (!f) > > + return -1; > > + > > + while ((k = getline(&line, &n, f)) != -1) { > > + if (strncmp(line, "NSpid:", 6)) > > + continue; > > + > > + line[k - 1] = '\0'; > > + ksft_print_msg("Child: fdinfo NSpid line: '%s'.\n", line); > > + r = strncmp(line + 6, expect, len); > > Wouldn't it be better to get rid of the nullbyte assignment and change > the strncmp() into a strcmp() here... > > [...] > > + /* The child will have pid 1 in the new pid namespace, > > + * so the line must be 'NSPid:\t<pid>\t1' > > + */ > > + n = snprintf(expect, sizeof(expect), "\t%d\t%d", pid, 1); > > ... and add a "\n" to the format string? It's shorter and doesn't > silently ignore it if the line doesn't end at that point. Also, what Christian just told me and what I wanted to suggest is that we add tests for sending around pidfds and reading fdinfo too.
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index 464c9b76148f..b7784dc488b8 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS += -g -I../../../../usr/include/ -lpthread -TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test pidfd_wait +TEST_GEN_PROGS := pidfd_test pdfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait include ../lib.mk diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index c6bc68329f4b..2946d788645b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -84,4 +84,16 @@ static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info, return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags); } +static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *)) +{ + size_t stack_size = 1024; + char *stack[1024] = { 0 }; + +#ifdef __ia64__ + return __clone2(fn, stack, stack_size, flags | SIGCHLD, NULL, pidfd); +#else + return clone(fn, stack + stack_size, flags | SIGCHLD, NULL, pidfd); +#endif +} + #endif /* __PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c new file mode 100644 index 000000000000..fbae502ad8ad --- /dev/null +++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0 + +#define _GNU_SOURCE +#include <errno.h> +#include <fcntl.h> +#include <linux/types.h> +#include <sched.h> +#include <signal.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <syscall.h> +#include <sys/wait.h> + +#include "pidfd.h" +#include "../kselftest.h" + +static int child_fdinfo_nspid_test(void *args) +{ + ksft_print_msg("Child: pid %d\n", getpid()); + return 0; +} + +static int compare_fdinfo_nspid(int pidfd, char *expect, size_t len) +{ + char path[512]; + FILE *f; + size_t n = 0; + ssize_t k; + char *line = NULL; + int r = -1; + + snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", pidfd); + + f = fopen(path, "re"); + if (!f) + return -1; + + while ((k = getline(&line, &n, f)) != -1) { + if (strncmp(line, "NSpid:", 6)) + continue; + + line[k - 1] = '\0'; + ksft_print_msg("Child: fdinfo NSpid line: '%s'.\n", line); + r = strncmp(line + 6, expect, len); + break; + } + + free(line); + fclose(f); + + return r; +} + +static void test_pidfd_fdinfo_nspid(void) +{ + char expect[512]; + int pid, pidfd = 0; + int n, r; + const char *test_name = "pidfd check for NSpid information in fdinfo"; + + pid = pidfd_clone(CLONE_PIDFD | CLONE_NEWPID | CLONE_NEWUSER, &pidfd, + child_fdinfo_nspid_test); + if (pid < 0) + ksft_exit_fail_msg( + "%s test: pidfd_clone failed (ret %d, errno %d)\n", + test_name, pid, errno); + + ksft_print_msg("Parent: child-pid: %d\n", pid); + + /* The child will have pid 1 in the new pid namespace, + * so the line must be 'NSPid:\t<pid>\t1' + */ + n = snprintf(expect, sizeof(expect), "\t%d\t%d", pid, 1); + r = compare_fdinfo_nspid(pidfd, expect, n); + + (void)close(pidfd); + + if (wait_for_pid(pid)) + ksft_exit_fail_msg( + "%s test: waitpid failed (ret %d, errno %d)\n", + test_name, r, errno); + + if (r != 0) + ksft_exit_fail_msg("%s test: Failed\n", test_name); + else + ksft_test_result_pass("%s test: Passed\n", test_name); +} + +int main(int argc, char **argv) +{ + ksft_print_header(); + ksft_set_plan(1); + + test_pidfd_fdinfo_nspid(); + + return ksft_exit_pass(); +} diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c index 7aff2d3b42c0..9cf0b6b3e389 100644 --- a/tools/testing/selftests/pidfd/pidfd_test.c +++ b/tools/testing/selftests/pidfd/pidfd_test.c @@ -27,18 +27,6 @@ #define MAX_EVENTS 5 -static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *)) -{ - size_t stack_size = 1024; - char *stack[1024] = { 0 }; - -#ifdef __ia64__ - return __clone2(fn, stack, stack_size, flags | SIGCHLD, NULL, pidfd); -#else - return clone(fn, stack + stack_size, flags | SIGCHLD, NULL, pidfd); -#endif -} - static int signal_received; static void set_signal_received_on_sigusr1(int sig)