Message-ID: <20241210-zustehen-skilift-44ba2f53ceca@brauner>
Date: Tue, 10 Dec 2024 15:37:28 +0100
From: Christian Brauner <brauner@...nel.org>
To: Juntong Deng <juntong.deng@...look.com>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com, 
	andrii@...nel.org, martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, 
	yonghong.song@...ux.dev, kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, 
	jolsa@...nel.org, memxor@...il.com, snorcht@...il.com, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 2/5] selftests/bpf: Add tests for open-coded
 style process file iterator

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@...look.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
> 

Powered by blists - more mailing lists