| Message ID | AM6PR03MB5080756ABBCCCBF664B374EB993D2@AM6PR03MB5080.eurprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Add open-coded style process file iterator and bpf_fget_task() kfunc | expand |
On Tue, Dec 10, 2024 at 02:03:51PM +0000, Juntong Deng wrote: > This patch adds test cases for open-coded style process file iterator. > > Test cases related to process files are run in the newly created child > process. Close all opened files inherited from the parent process in > the child process to avoid the files opened by the parent process > affecting the test results. > > In addition, this patch adds failure test cases where bpf programs > cannot pass the verifier due to uninitialized or untrusted > arguments, or not in RCU CS, etc. > > Signed-off-by: Juntong Deng <juntong.deng@outlook.com> > --- > .../testing/selftests/bpf/bpf_experimental.h | 7 ++ > .../testing/selftests/bpf/prog_tests/iters.c | 79 ++++++++++++ > .../selftests/bpf/progs/iters_task_file.c | 88 ++++++++++++++ > .../bpf/progs/iters_task_file_failure.c | 114 ++++++++++++++++++ > 4 files changed, 288 insertions(+) > create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file.c > create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file_failure.c > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h > index cd8ecd39c3f3..ce1520c56b55 100644 > --- a/tools/testing/selftests/bpf/bpf_experimental.h > +++ b/tools/testing/selftests/bpf/bpf_experimental.h > @@ -588,4 +588,11 @@ extern int bpf_iter_kmem_cache_new(struct bpf_iter_kmem_cache *it) __weak __ksym > extern struct kmem_cache *bpf_iter_kmem_cache_next(struct bpf_iter_kmem_cache *it) __weak __ksym; > extern void bpf_iter_kmem_cache_destroy(struct bpf_iter_kmem_cache *it) __weak __ksym; > > +struct bpf_iter_task_file; > +struct bpf_iter_task_file_item; > +extern int bpf_iter_task_file_new(struct bpf_iter_task_file *it, struct task_struct *task) __ksym; > +extern struct bpf_iter_task_file_item * > +bpf_iter_task_file_next(struct bpf_iter_task_file *it) __ksym; > +extern void bpf_iter_task_file_destroy(struct bpf_iter_task_file *it) __ksym; > + > #endif > diff --git a/tools/testing/selftests/bpf/prog_tests/iters.c b/tools/testing/selftests/bpf/prog_tests/iters.c > index 3cea71f9c500..cfe5b56cc027 100644 > --- a/tools/testing/selftests/bpf/prog_tests/iters.c > +++ b/tools/testing/selftests/bpf/prog_tests/iters.c > @@ -1,6 +1,8 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */ > > +#define _GNU_SOURCE > +#include <sys/socket.h> > #include <sys/syscall.h> > #include <sys/mman.h> > #include <sys/wait.h> > @@ -16,11 +18,13 @@ > #include "iters_num.skel.h" > #include "iters_testmod.skel.h" > #include "iters_testmod_seq.skel.h" > +#include "iters_task_file.skel.h" > #include "iters_task_vma.skel.h" > #include "iters_task.skel.h" > #include "iters_css_task.skel.h" > #include "iters_css.skel.h" > #include "iters_task_failure.skel.h" > +#include "iters_task_file_failure.skel.h" > > static void subtest_num_iters(void) > { > @@ -291,6 +295,78 @@ static void subtest_css_iters(void) > iters_css__destroy(skel); > } > > +static int task_file_test_process(void *args) > +{ > + int pipefd[2], sockfd, err = 0; > + > + /* Create a clean file descriptor table for the test process */ > + close_range(0, ~0U, 0); > + > + if (pipe(pipefd) < 0) > + return 1; > + > + sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); > + if (sockfd < 0) { > + err = 2; > + goto cleanup_pipe; > + } > + > + usleep(1); > + > + close(sockfd); > +cleanup_pipe: > + close(pipefd[0]); > + close(pipefd[1]); > + return err; > +} > + > +static void subtest_task_file_iters(void) > +{ > + const int stack_size = 1024 * 1024; > + struct iters_task_file *skel; > + int child_pid, wstatus, err; > + char *stack; > + > + skel = iters_task_file__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "open_and_load")) > + return; > + > + if (!ASSERT_OK(skel->bss->err, "pre_test_err")) > + goto cleanup_skel; > + > + skel->bss->parent_pid = getpid(); > + skel->bss->count = 0; > + > + err = iters_task_file__attach(skel); > + if (!ASSERT_OK(err, "skel_attach")) > + goto cleanup_skel; > + > + stack = (char *)malloc(stack_size); > + if (!ASSERT_OK_PTR(stack, "clone_stack")) > + goto cleanup_attach; > + > + /* Note that there is no CLONE_FILES */ > + child_pid = clone(task_file_test_process, stack + stack_size, CLONE_VM | SIGCHLD, NULL); > + if (!ASSERT_GT(child_pid, -1, "child_pid")) > + goto cleanup_stack; > + > + if (!ASSERT_GT(waitpid(child_pid, &wstatus, 0), -1, "waitpid")) > + goto cleanup_stack; > + > + if (!ASSERT_OK(WEXITSTATUS(wstatus), "run_task_file_iters_test_err")) > + goto cleanup_stack; > + > + ASSERT_EQ(skel->bss->count, 1, "run_task_file_iters_test_count_err"); > + ASSERT_OK(skel->bss->err, "run_task_file_iters_test_failure"); > + > +cleanup_stack: > + free(stack); > +cleanup_attach: > + iters_task_file__detach(skel); > +cleanup_skel: > + iters_task_file__destroy(skel); > +} > + > void test_iters(void) > { > RUN_TESTS(iters_state_safety); > @@ -315,5 +391,8 @@ void test_iters(void) > subtest_css_task_iters(); > if (test__start_subtest("css")) > subtest_css_iters(); > + if (test__start_subtest("task_file")) > + subtest_task_file_iters(); > RUN_TESTS(iters_task_failure); > + RUN_TESTS(iters_task_file_failure); > } > diff --git a/tools/testing/selftests/bpf/progs/iters_task_file.c b/tools/testing/selftests/bpf/progs/iters_task_file.c > new file mode 100644 > index 000000000000..81bcd20041d8 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/iters_task_file.c > @@ -0,0 +1,88 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include "vmlinux.h" > +#include <bpf/bpf_tracing.h> > +#include <bpf/bpf_helpers.h> > +#include "bpf_misc.h" > +#include "bpf_experimental.h" > +#include "task_kfunc_common.h" > + > +char _license[] SEC("license") = "GPL"; > + > +int err, parent_pid, count; > + > +extern const void pipefifo_fops __ksym; > +extern const void socket_file_ops __ksym; > + > +SEC("fentry/" SYS_PREFIX "sys_nanosleep") > +int test_bpf_iter_task_file(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct bpf_iter_task_file_item *item; > + struct task_struct *task; > + > + task = bpf_get_current_task_btf(); > + if (task->parent->pid != parent_pid) > + return 0; > + > + count++; > + > + bpf_rcu_read_lock(); What does the RCU read lock do here exactly? > + bpf_iter_task_file_new(&task_file_it, task); > + > + item = bpf_iter_task_file_next(&task_file_it); > + if (item == NULL) { > + err = 1; > + goto cleanup; > + } > + > + if (item->fd != 0) { > + err = 2; > + goto cleanup; > + } > + > + if (item->file->f_op != &pipefifo_fops) { > + err = 3; > + goto cleanup; > + } > + > + item = bpf_iter_task_file_next(&task_file_it); > + if (item == NULL) { > + err = 4; > + goto cleanup; > + } > + > + if (item->fd != 1) { > + err = 5; > + goto cleanup; > + } > + > + if (item->file->f_op != &pipefifo_fops) { > + err = 6; > + goto cleanup; > + } > + > + item = bpf_iter_task_file_next(&task_file_it); > + if (item == NULL) { > + err = 7; > + goto cleanup; > + } > + > + if (item->fd != 2) { > + err = 8; > + goto cleanup; > + } > + > + if (item->file->f_op != &socket_file_ops) { > + err = 9; > + goto cleanup; > + } > + > + item = bpf_iter_task_file_next(&task_file_it); > + if (item != NULL) > + err = 10; > +cleanup: > + bpf_iter_task_file_destroy(&task_file_it); > + bpf_rcu_read_unlock(); > + return 0; > +} > diff --git a/tools/testing/selftests/bpf/progs/iters_task_file_failure.c b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c > new file mode 100644 > index 000000000000..c3de9235b888 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c > @@ -0,0 +1,114 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include "vmlinux.h" > +#include <bpf/bpf_tracing.h> > +#include <bpf/bpf_helpers.h> > +#include "bpf_misc.h" > +#include "bpf_experimental.h" > +#include "task_kfunc_common.h" > + > +char _license[] SEC("license") = "GPL"; > + > +SEC("syscall") > +__failure __msg("expected an RCU CS when using bpf_iter_task_file") > +int bpf_iter_task_file_new_without_rcu_lock(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct task_struct *task; > + > + task = bpf_get_current_task_btf(); > + > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_iter_task_file_destroy(&task_file_it); > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("expected uninitialized iter_task_file as arg #1") > +int bpf_iter_task_file_new_inited_iter(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct task_struct *task; > + > + task = bpf_get_current_task_btf(); > + > + bpf_rcu_read_lock(); > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_iter_task_file_destroy(&task_file_it); > + bpf_rcu_read_unlock(); > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("Possibly NULL pointer passed to trusted arg1") > +int bpf_iter_task_file_new_null_task(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct task_struct *task = NULL; > + > + bpf_rcu_read_lock(); > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_iter_task_file_destroy(&task_file_it); > + bpf_rcu_read_unlock(); > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("R2 must be referenced or trusted") > +int bpf_iter_task_file_new_untrusted_task(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct task_struct *task; > + > + task = bpf_get_current_task_btf()->parent; > + > + bpf_rcu_read_lock(); > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_iter_task_file_destroy(&task_file_it); > + bpf_rcu_read_unlock(); > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("Unreleased reference") > +int bpf_iter_task_file_no_destory(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + struct task_struct *task; > + > + task = bpf_get_current_task_btf(); > + > + bpf_rcu_read_lock(); > + bpf_iter_task_file_new(&task_file_it, task); > + > + bpf_rcu_read_unlock(); > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("expected an initialized iter_task_file as arg #1") > +int bpf_iter_task_file_next_uninit_iter(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + > + bpf_iter_task_file_next(&task_file_it); > + > + return 0; > +} > + > +SEC("syscall") > +__failure __msg("expected an initialized iter_task_file as arg #1") > +int bpf_iter_task_file_destroy_uninit_iter(void *ctx) > +{ > + struct bpf_iter_task_file task_file_it; > + > + bpf_iter_task_file_destroy(&task_file_it); > + > + return 0; > +} > -- > 2.39.5 >
On 2024/12/10 14:37, Christian Brauner wrote: > On Tue, Dec 10, 2024 at 02:03:51PM +0000, Juntong Deng wrote: >> This patch adds test cases for open-coded style process file iterator. >> >> Test cases related to process files are run in the newly created child >> process. Close all opened files inherited from the parent process in >> the child process to avoid the files opened by the parent process >> affecting the test results. >> >> In addition, this patch adds failure test cases where bpf programs >> cannot pass the verifier due to uninitialized or untrusted >> arguments, or not in RCU CS, etc. >> >> Signed-off-by: Juntong Deng <juntong.deng@outlook.com> >> --- >> .../testing/selftests/bpf/bpf_experimental.h | 7 ++ >> .../testing/selftests/bpf/prog_tests/iters.c | 79 ++++++++++++ >> .../selftests/bpf/progs/iters_task_file.c | 88 ++++++++++++++ >> .../bpf/progs/iters_task_file_failure.c | 114 ++++++++++++++++++ >> 4 files changed, 288 insertions(+) >> create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file.c >> create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file_failure.c >> >> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h >> index cd8ecd39c3f3..ce1520c56b55 100644 >> --- a/tools/testing/selftests/bpf/bpf_experimental.h >> +++ b/tools/testing/selftests/bpf/bpf_experimental.h >> @@ -588,4 +588,11 @@ extern int bpf_iter_kmem_cache_new(struct bpf_iter_kmem_cache *it) __weak __ksym >> extern struct kmem_cache *bpf_iter_kmem_cache_next(struct bpf_iter_kmem_cache *it) __weak __ksym; >> extern void bpf_iter_kmem_cache_destroy(struct bpf_iter_kmem_cache *it) __weak __ksym; >> >> +struct bpf_iter_task_file; >> +struct bpf_iter_task_file_item; >> +extern int bpf_iter_task_file_new(struct bpf_iter_task_file *it, struct task_struct *task) __ksym; >> +extern struct bpf_iter_task_file_item * >> +bpf_iter_task_file_next(struct bpf_iter_task_file *it) __ksym; >> +extern void bpf_iter_task_file_destroy(struct bpf_iter_task_file *it) __ksym; >> + >> #endif >> diff --git a/tools/testing/selftests/bpf/prog_tests/iters.c b/tools/testing/selftests/bpf/prog_tests/iters.c >> index 3cea71f9c500..cfe5b56cc027 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/iters.c >> +++ b/tools/testing/selftests/bpf/prog_tests/iters.c >> @@ -1,6 +1,8 @@ >> // SPDX-License-Identifier: GPL-2.0 >> /* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */ >> >> +#define _GNU_SOURCE >> +#include <sys/socket.h> >> #include <sys/syscall.h> >> #include <sys/mman.h> >> #include <sys/wait.h> >> @@ -16,11 +18,13 @@ >> #include "iters_num.skel.h" >> #include "iters_testmod.skel.h" >> #include "iters_testmod_seq.skel.h" >> +#include "iters_task_file.skel.h" >> #include "iters_task_vma.skel.h" >> #include "iters_task.skel.h" >> #include "iters_css_task.skel.h" >> #include "iters_css.skel.h" >> #include "iters_task_failure.skel.h" >> +#include "iters_task_file_failure.skel.h" >> >> static void subtest_num_iters(void) >> { >> @@ -291,6 +295,78 @@ static void subtest_css_iters(void) >> iters_css__destroy(skel); >> } >> >> +static int task_file_test_process(void *args) >> +{ >> + int pipefd[2], sockfd, err = 0; >> + >> + /* Create a clean file descriptor table for the test process */ >> + close_range(0, ~0U, 0); >> + >> + if (pipe(pipefd) < 0) >> + return 1; >> + >> + sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); >> + if (sockfd < 0) { >> + err = 2; >> + goto cleanup_pipe; >> + } >> + >> + usleep(1); >> + >> + close(sockfd); >> +cleanup_pipe: >> + close(pipefd[0]); >> + close(pipefd[1]); >> + return err; >> +} >> + >> +static void subtest_task_file_iters(void) >> +{ >> + const int stack_size = 1024 * 1024; >> + struct iters_task_file *skel; >> + int child_pid, wstatus, err; >> + char *stack; >> + >> + skel = iters_task_file__open_and_load(); >> + if (!ASSERT_OK_PTR(skel, "open_and_load")) >> + return; >> + >> + if (!ASSERT_OK(skel->bss->err, "pre_test_err")) >> + goto cleanup_skel; >> + >> + skel->bss->parent_pid = getpid(); >> + skel->bss->count = 0; >> + >> + err = iters_task_file__attach(skel); >> + if (!ASSERT_OK(err, "skel_attach")) >> + goto cleanup_skel; >> + >> + stack = (char *)malloc(stack_size); >> + if (!ASSERT_OK_PTR(stack, "clone_stack")) >> + goto cleanup_attach; >> + >> + /* Note that there is no CLONE_FILES */ >> + child_pid = clone(task_file_test_process, stack + stack_size, CLONE_VM | SIGCHLD, NULL); >> + if (!ASSERT_GT(child_pid, -1, "child_pid")) >> + goto cleanup_stack; >> + >> + if (!ASSERT_GT(waitpid(child_pid, &wstatus, 0), -1, "waitpid")) >> + goto cleanup_stack; >> + >> + if (!ASSERT_OK(WEXITSTATUS(wstatus), "run_task_file_iters_test_err")) >> + goto cleanup_stack; >> + >> + ASSERT_EQ(skel->bss->count, 1, "run_task_file_iters_test_count_err"); >> + ASSERT_OK(skel->bss->err, "run_task_file_iters_test_failure"); >> + >> +cleanup_stack: >> + free(stack); >> +cleanup_attach: >> + iters_task_file__detach(skel); >> +cleanup_skel: >> + iters_task_file__destroy(skel); >> +} >> + >> void test_iters(void) >> { >> RUN_TESTS(iters_state_safety); >> @@ -315,5 +391,8 @@ void test_iters(void) >> subtest_css_task_iters(); >> if (test__start_subtest("css")) >> subtest_css_iters(); >> + if (test__start_subtest("task_file")) >> + subtest_task_file_iters(); >> RUN_TESTS(iters_task_failure); >> + RUN_TESTS(iters_task_file_failure); >> } >> diff --git a/tools/testing/selftests/bpf/progs/iters_task_file.c b/tools/testing/selftests/bpf/progs/iters_task_file.c >> new file mode 100644 >> index 000000000000..81bcd20041d8 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/progs/iters_task_file.c >> @@ -0,0 +1,88 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +#include "vmlinux.h" >> +#include <bpf/bpf_tracing.h> >> +#include <bpf/bpf_helpers.h> >> +#include "bpf_misc.h" >> +#include "bpf_experimental.h" >> +#include "task_kfunc_common.h" >> + >> +char _license[] SEC("license") = "GPL"; >> + >> +int err, parent_pid, count; >> + >> +extern const void pipefifo_fops __ksym; >> +extern const void socket_file_ops __ksym; >> + >> +SEC("fentry/" SYS_PREFIX "sys_nanosleep") >> +int test_bpf_iter_task_file(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct bpf_iter_task_file_item *item; >> + struct task_struct *task; >> + >> + task = bpf_get_current_task_btf(); >> + if (task->parent->pid != parent_pid) >> + return 0; >> + >> + count++; >> + >> + bpf_rcu_read_lock(); > > What does the RCU read lock do here exactly? > Thanks for your reply. This is used to solve the problem previously discussed in v3 [0]. Task ref may be released during iteration. [0]: https://lore.kernel.org/bpf/CAADnVQ+0LUXxmfm1YgyGDz=cciy3+dGGM-Zysq84fpAdaB74Qw@mail.gmail.com/ >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + item = bpf_iter_task_file_next(&task_file_it); >> + if (item == NULL) { >> + err = 1; >> + goto cleanup; >> + } >> + >> + if (item->fd != 0) { >> + err = 2; >> + goto cleanup; >> + } >> + >> + if (item->file->f_op != &pipefifo_fops) { >> + err = 3; >> + goto cleanup; >> + } >> + >> + item = bpf_iter_task_file_next(&task_file_it); >> + if (item == NULL) { >> + err = 4; >> + goto cleanup; >> + } >> + >> + if (item->fd != 1) { >> + err = 5; >> + goto cleanup; >> + } >> + >> + if (item->file->f_op != &pipefifo_fops) { >> + err = 6; >> + goto cleanup; >> + } >> + >> + item = bpf_iter_task_file_next(&task_file_it); >> + if (item == NULL) { >> + err = 7; >> + goto cleanup; >> + } >> + >> + if (item->fd != 2) { >> + err = 8; >> + goto cleanup; >> + } >> + >> + if (item->file->f_op != &socket_file_ops) { >> + err = 9; >> + goto cleanup; >> + } >> + >> + item = bpf_iter_task_file_next(&task_file_it); >> + if (item != NULL) >> + err = 10; >> +cleanup: >> + bpf_iter_task_file_destroy(&task_file_it); >> + bpf_rcu_read_unlock(); >> + return 0; >> +} >> diff --git a/tools/testing/selftests/bpf/progs/iters_task_file_failure.c b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c >> new file mode 100644 >> index 000000000000..c3de9235b888 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c >> @@ -0,0 +1,114 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +#include "vmlinux.h" >> +#include <bpf/bpf_tracing.h> >> +#include <bpf/bpf_helpers.h> >> +#include "bpf_misc.h" >> +#include "bpf_experimental.h" >> +#include "task_kfunc_common.h" >> + >> +char _license[] SEC("license") = "GPL"; >> + >> +SEC("syscall") >> +__failure __msg("expected an RCU CS when using bpf_iter_task_file") >> +int bpf_iter_task_file_new_without_rcu_lock(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct task_struct *task; >> + >> + task = bpf_get_current_task_btf(); >> + >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_iter_task_file_destroy(&task_file_it); >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("expected uninitialized iter_task_file as arg #1") >> +int bpf_iter_task_file_new_inited_iter(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct task_struct *task; >> + >> + task = bpf_get_current_task_btf(); >> + >> + bpf_rcu_read_lock(); >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_iter_task_file_destroy(&task_file_it); >> + bpf_rcu_read_unlock(); >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("Possibly NULL pointer passed to trusted arg1") >> +int bpf_iter_task_file_new_null_task(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct task_struct *task = NULL; >> + >> + bpf_rcu_read_lock(); >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_iter_task_file_destroy(&task_file_it); >> + bpf_rcu_read_unlock(); >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("R2 must be referenced or trusted") >> +int bpf_iter_task_file_new_untrusted_task(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct task_struct *task; >> + >> + task = bpf_get_current_task_btf()->parent; >> + >> + bpf_rcu_read_lock(); >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_iter_task_file_destroy(&task_file_it); >> + bpf_rcu_read_unlock(); >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("Unreleased reference") >> +int bpf_iter_task_file_no_destory(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + struct task_struct *task; >> + >> + task = bpf_get_current_task_btf(); >> + >> + bpf_rcu_read_lock(); >> + bpf_iter_task_file_new(&task_file_it, task); >> + >> + bpf_rcu_read_unlock(); >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("expected an initialized iter_task_file as arg #1") >> +int bpf_iter_task_file_next_uninit_iter(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + >> + bpf_iter_task_file_next(&task_file_it); >> + >> + return 0; >> +} >> + >> +SEC("syscall") >> +__failure __msg("expected an initialized iter_task_file as arg #1") >> +int bpf_iter_task_file_destroy_uninit_iter(void *ctx) >> +{ >> + struct bpf_iter_task_file task_file_it; >> + >> + bpf_iter_task_file_destroy(&task_file_it); >> + >> + return 0; >> +} >> -- >> 2.39.5 >>
On Tue, Dec 10, 2024 at 8:23 AM Juntong Deng <juntong.deng@outlook.com> wrote: > > >> +SEC("fentry/" SYS_PREFIX "sys_nanosleep") > >> +int test_bpf_iter_task_file(void *ctx) > >> +{ > >> + struct bpf_iter_task_file task_file_it; > >> + struct bpf_iter_task_file_item *item; > >> + struct task_struct *task; > >> + > >> + task = bpf_get_current_task_btf(); > >> + if (task->parent->pid != parent_pid) > >> + return 0; > >> + > >> + count++; > >> + > >> + bpf_rcu_read_lock(); > > > > What does the RCU read lock do here exactly? > > > > Thanks for your reply. > > This is used to solve the problem previously discussed in v3 [0]. > > Task ref may be released during iteration. > > [0]: > https://lore.kernel.org/bpf/CAADnVQ+0LUXxmfm1YgyGDz=cciy3+dGGM-Zysq84fpAdaB74Qw@mail.gmail.com/ I think you misunderstood my comment. "If this object _was_ RCU protected ..." Adding rcu_read_lock doesn't make 'task' pointer RCU protected. That's not how RCU works. So patch 1 doing: item->task = task; is not correct. See bpf_iter_task_vma_new(). It's doing: kit->data->task = get_task_struct(task); to make sure task stays valid while iterating. pw-bot: cr
On 2024/12/10 18:51, Alexei Starovoitov wrote: > On Tue, Dec 10, 2024 at 8:23 AM Juntong Deng <juntong.deng@outlook.com> wrote: >> >>>> +SEC("fentry/" SYS_PREFIX "sys_nanosleep") >>>> +int test_bpf_iter_task_file(void *ctx) >>>> +{ >>>> + struct bpf_iter_task_file task_file_it; >>>> + struct bpf_iter_task_file_item *item; >>>> + struct task_struct *task; >>>> + >>>> + task = bpf_get_current_task_btf(); >>>> + if (task->parent->pid != parent_pid) >>>> + return 0; >>>> + >>>> + count++; >>>> + >>>> + bpf_rcu_read_lock(); >>> >>> What does the RCU read lock do here exactly? >>> >> >> Thanks for your reply. >> >> This is used to solve the problem previously discussed in v3 [0]. >> >> Task ref may be released during iteration. >> >> [0]: >> https://lore.kernel.org/bpf/CAADnVQ+0LUXxmfm1YgyGDz=cciy3+dGGM-Zysq84fpAdaB74Qw@mail.gmail.com/ > > I think you misunderstood my comment. > > "If this object _was_ RCU protected ..." > > Adding rcu_read_lock doesn't make 'task' pointer RCU protected. > That's not how RCU works. > > So patch 1 doing: > > item->task = task; > > is not correct. > > See bpf_iter_task_vma_new(). It's doing: > kit->data->task = get_task_struct(task); > to make sure task stays valid while iterating. > > pw-bot: cr Thanks for your reply. Sorry for the misunderstanding. I will fix it in the next version.
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index cd8ecd39c3f3..ce1520c56b55 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -588,4 +588,11 @@ extern int bpf_iter_kmem_cache_new(struct bpf_iter_kmem_cache *it) __weak __ksym extern struct kmem_cache *bpf_iter_kmem_cache_next(struct bpf_iter_kmem_cache *it) __weak __ksym; extern void bpf_iter_kmem_cache_destroy(struct bpf_iter_kmem_cache *it) __weak __ksym; +struct bpf_iter_task_file; +struct bpf_iter_task_file_item; +extern int bpf_iter_task_file_new(struct bpf_iter_task_file *it, struct task_struct *task) __ksym; +extern struct bpf_iter_task_file_item * +bpf_iter_task_file_next(struct bpf_iter_task_file *it) __ksym; +extern void bpf_iter_task_file_destroy(struct bpf_iter_task_file *it) __ksym; + #endif diff --git a/tools/testing/selftests/bpf/prog_tests/iters.c b/tools/testing/selftests/bpf/prog_tests/iters.c index 3cea71f9c500..cfe5b56cc027 100644 --- a/tools/testing/selftests/bpf/prog_tests/iters.c +++ b/tools/testing/selftests/bpf/prog_tests/iters.c @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */ +#define _GNU_SOURCE +#include <sys/socket.h> #include <sys/syscall.h> #include <sys/mman.h> #include <sys/wait.h> @@ -16,11 +18,13 @@ #include "iters_num.skel.h" #include "iters_testmod.skel.h" #include "iters_testmod_seq.skel.h" +#include "iters_task_file.skel.h" #include "iters_task_vma.skel.h" #include "iters_task.skel.h" #include "iters_css_task.skel.h" #include "iters_css.skel.h" #include "iters_task_failure.skel.h" +#include "iters_task_file_failure.skel.h" static void subtest_num_iters(void) { @@ -291,6 +295,78 @@ static void subtest_css_iters(void) iters_css__destroy(skel); } +static int task_file_test_process(void *args) +{ + int pipefd[2], sockfd, err = 0; + + /* Create a clean file descriptor table for the test process */ + close_range(0, ~0U, 0); + + if (pipe(pipefd) < 0) + return 1; + + sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + if (sockfd < 0) { + err = 2; + goto cleanup_pipe; + } + + usleep(1); + + close(sockfd); +cleanup_pipe: + close(pipefd[0]); + close(pipefd[1]); + return err; +} + +static void subtest_task_file_iters(void) +{ + const int stack_size = 1024 * 1024; + struct iters_task_file *skel; + int child_pid, wstatus, err; + char *stack; + + skel = iters_task_file__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + return; + + if (!ASSERT_OK(skel->bss->err, "pre_test_err")) + goto cleanup_skel; + + skel->bss->parent_pid = getpid(); + skel->bss->count = 0; + + err = iters_task_file__attach(skel); + if (!ASSERT_OK(err, "skel_attach")) + goto cleanup_skel; + + stack = (char *)malloc(stack_size); + if (!ASSERT_OK_PTR(stack, "clone_stack")) + goto cleanup_attach; + + /* Note that there is no CLONE_FILES */ + child_pid = clone(task_file_test_process, stack + stack_size, CLONE_VM | SIGCHLD, NULL); + if (!ASSERT_GT(child_pid, -1, "child_pid")) + goto cleanup_stack; + + if (!ASSERT_GT(waitpid(child_pid, &wstatus, 0), -1, "waitpid")) + goto cleanup_stack; + + if (!ASSERT_OK(WEXITSTATUS(wstatus), "run_task_file_iters_test_err")) + goto cleanup_stack; + + ASSERT_EQ(skel->bss->count, 1, "run_task_file_iters_test_count_err"); + ASSERT_OK(skel->bss->err, "run_task_file_iters_test_failure"); + +cleanup_stack: + free(stack); +cleanup_attach: + iters_task_file__detach(skel); +cleanup_skel: + iters_task_file__destroy(skel); +} + void test_iters(void) { RUN_TESTS(iters_state_safety); @@ -315,5 +391,8 @@ void test_iters(void) subtest_css_task_iters(); if (test__start_subtest("css")) subtest_css_iters(); + if (test__start_subtest("task_file")) + subtest_task_file_iters(); RUN_TESTS(iters_task_failure); + RUN_TESTS(iters_task_file_failure); } diff --git a/tools/testing/selftests/bpf/progs/iters_task_file.c b/tools/testing/selftests/bpf/progs/iters_task_file.c new file mode 100644 index 000000000000..81bcd20041d8 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/iters_task_file.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "vmlinux.h" +#include <bpf/bpf_tracing.h> +#include <bpf/bpf_helpers.h> +#include "bpf_misc.h" +#include "bpf_experimental.h" +#include "task_kfunc_common.h" + +char _license[] SEC("license") = "GPL"; + +int err, parent_pid, count; + +extern const void pipefifo_fops __ksym; +extern const void socket_file_ops __ksym; + +SEC("fentry/" SYS_PREFIX "sys_nanosleep") +int test_bpf_iter_task_file(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct bpf_iter_task_file_item *item; + struct task_struct *task; + + task = bpf_get_current_task_btf(); + if (task->parent->pid != parent_pid) + return 0; + + count++; + + bpf_rcu_read_lock(); + bpf_iter_task_file_new(&task_file_it, task); + + item = bpf_iter_task_file_next(&task_file_it); + if (item == NULL) { + err = 1; + goto cleanup; + } + + if (item->fd != 0) { + err = 2; + goto cleanup; + } + + if (item->file->f_op != &pipefifo_fops) { + err = 3; + goto cleanup; + } + + item = bpf_iter_task_file_next(&task_file_it); + if (item == NULL) { + err = 4; + goto cleanup; + } + + if (item->fd != 1) { + err = 5; + goto cleanup; + } + + if (item->file->f_op != &pipefifo_fops) { + err = 6; + goto cleanup; + } + + item = bpf_iter_task_file_next(&task_file_it); + if (item == NULL) { + err = 7; + goto cleanup; + } + + if (item->fd != 2) { + err = 8; + goto cleanup; + } + + if (item->file->f_op != &socket_file_ops) { + err = 9; + goto cleanup; + } + + item = bpf_iter_task_file_next(&task_file_it); + if (item != NULL) + err = 10; +cleanup: + bpf_iter_task_file_destroy(&task_file_it); + bpf_rcu_read_unlock(); + return 0; +} diff --git a/tools/testing/selftests/bpf/progs/iters_task_file_failure.c b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c new file mode 100644 index 000000000000..c3de9235b888 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/iters_task_file_failure.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "vmlinux.h" +#include <bpf/bpf_tracing.h> +#include <bpf/bpf_helpers.h> +#include "bpf_misc.h" +#include "bpf_experimental.h" +#include "task_kfunc_common.h" + +char _license[] SEC("license") = "GPL"; + +SEC("syscall") +__failure __msg("expected an RCU CS when using bpf_iter_task_file") +int bpf_iter_task_file_new_without_rcu_lock(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct task_struct *task; + + task = bpf_get_current_task_btf(); + + bpf_iter_task_file_new(&task_file_it, task); + + bpf_iter_task_file_destroy(&task_file_it); + return 0; +} + +SEC("syscall") +__failure __msg("expected uninitialized iter_task_file as arg #1") +int bpf_iter_task_file_new_inited_iter(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct task_struct *task; + + task = bpf_get_current_task_btf(); + + bpf_rcu_read_lock(); + bpf_iter_task_file_new(&task_file_it, task); + + bpf_iter_task_file_new(&task_file_it, task); + + bpf_iter_task_file_destroy(&task_file_it); + bpf_rcu_read_unlock(); + return 0; +} + +SEC("syscall") +__failure __msg("Possibly NULL pointer passed to trusted arg1") +int bpf_iter_task_file_new_null_task(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct task_struct *task = NULL; + + bpf_rcu_read_lock(); + bpf_iter_task_file_new(&task_file_it, task); + + bpf_iter_task_file_destroy(&task_file_it); + bpf_rcu_read_unlock(); + return 0; +} + +SEC("syscall") +__failure __msg("R2 must be referenced or trusted") +int bpf_iter_task_file_new_untrusted_task(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct task_struct *task; + + task = bpf_get_current_task_btf()->parent; + + bpf_rcu_read_lock(); + bpf_iter_task_file_new(&task_file_it, task); + + bpf_iter_task_file_destroy(&task_file_it); + bpf_rcu_read_unlock(); + return 0; +} + +SEC("syscall") +__failure __msg("Unreleased reference") +int bpf_iter_task_file_no_destory(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + struct task_struct *task; + + task = bpf_get_current_task_btf(); + + bpf_rcu_read_lock(); + bpf_iter_task_file_new(&task_file_it, task); + + bpf_rcu_read_unlock(); + return 0; +} + +SEC("syscall") +__failure __msg("expected an initialized iter_task_file as arg #1") +int bpf_iter_task_file_next_uninit_iter(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + + bpf_iter_task_file_next(&task_file_it); + + return 0; +} + +SEC("syscall") +__failure __msg("expected an initialized iter_task_file as arg #1") +int bpf_iter_task_file_destroy_uninit_iter(void *ctx) +{ + struct bpf_iter_task_file task_file_it; + + bpf_iter_task_file_destroy(&task_file_it); + + return 0; +}
This patch adds test cases for open-coded style process file iterator. Test cases related to process files are run in the newly created child process. Close all opened files inherited from the parent process in the child process to avoid the files opened by the parent process affecting the test results. In addition, this patch adds failure test cases where bpf programs cannot pass the verifier due to uninitialized or untrusted arguments, or not in RCU CS, etc. Signed-off-by: Juntong Deng <juntong.deng@outlook.com> --- .../testing/selftests/bpf/bpf_experimental.h | 7 ++ .../testing/selftests/bpf/prog_tests/iters.c | 79 ++++++++++++ .../selftests/bpf/progs/iters_task_file.c | 88 ++++++++++++++ .../bpf/progs/iters_task_file_failure.c | 114 ++++++++++++++++++ 4 files changed, 288 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file.c create mode 100644 tools/testing/selftests/bpf/progs/iters_task_file_failure.c