diff mbox series

[bpf-next,v4,6/7] selftests/bpf: Add selftests for new cgroup local storage

Message ID 20221023180546.2863789-1-yhs@fb.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Implement cgroup local storage available to non-cgroup-attached bpf progs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_progs on s390x with gcc
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: sdf@google.com john.fastabend@gmail.com haoluo@google.com linux-kselftest@vger.kernel.org jolsa@kernel.org song@kernel.org shuah@kernel.org mykolal@fb.com martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 102 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yonghong Song Oct. 23, 2022, 6:05 p.m. UTC 
Add two tests for new cgroup local storage, one to test bpf program helpers
and user space map APIs, and the other to test recursive fentry
triggering won't deadlock.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 .../bpf/prog_tests/cgrp_local_storage.c       | 92 +++++++++++++++++++
 .../selftests/bpf/progs/cgrp_local_storage.c  | 88 ++++++++++++++++++
 .../selftests/bpf/progs/cgrp_ls_recursion.c   | 70 ++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgrp_local_storage.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c

Comments

David Vernet Oct. 23, 2022, 8:14 p.m. UTC | #1 
On Sun, Oct 23, 2022 at 11:05:46AM -0700, Yonghong Song wrote:
> Add two tests for new cgroup local storage, one to test bpf program helpers
> and user space map APIs, and the other to test recursive fentry
> triggering won't deadlock.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  .../bpf/prog_tests/cgrp_local_storage.c       | 92 +++++++++++++++++++
>  .../selftests/bpf/progs/cgrp_local_storage.c  | 88 ++++++++++++++++++
>  .../selftests/bpf/progs/cgrp_ls_recursion.c   | 70 ++++++++++++++
>  3 files changed, 250 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
>  create mode 100644 tools/testing/selftests/bpf/progs/cgrp_local_storage.c
>  create mode 100644 tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
> new file mode 100644
> index 000000000000..7ee21d598195
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates.*/
> +
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <test_progs.h>
> +#include "cgrp_local_storage.skel.h"
> +#include "cgrp_ls_recursion.skel.h"
> +
> +static void test_sys_enter_exit(int cgroup_fd)
> +{
> +	struct cgrp_local_storage *skel;
> +	long val1 = 1, val2 = 0;
> +	int err;
> +
> +	skel = cgrp_local_storage__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> +		return;
> +
> +	/* populate a value in cgrp_storage_2 */
> +	err = bpf_map_update_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val1, BPF_ANY);
> +	if (!ASSERT_OK(err, "map_update_elem"))
> +		goto out;
> +
> +	/* check value */
> +	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val2);
> +	if (!ASSERT_OK(err, "map_lookup_elem"))
> +		goto out;
> +	if (!ASSERT_EQ(val2, 1, "map_lookup_elem, invalid val"))
> +		goto out;
> +
> +	/* delete value */
> +	err = bpf_map_delete_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd);
> +	if (!ASSERT_OK(err, "map_delete_elem"))
> +		goto out;
> +
> +	skel->bss->target_pid = syscall(SYS_gettid);
> +
> +	err = cgrp_local_storage__attach(skel);
> +	if (!ASSERT_OK(err, "skel_attach"))
> +		goto out;
> +
> +	syscall(SYS_gettid);
> +	syscall(SYS_gettid);
> +
> +	skel->bss->target_pid = 0;
> +
> +	/* 3x syscalls: 1x attach and 2x gettid */
> +	ASSERT_EQ(skel->bss->enter_cnt, 3, "enter_cnt");
> +	ASSERT_EQ(skel->bss->exit_cnt, 3, "exit_cnt");
> +	ASSERT_EQ(skel->bss->mismatch_cnt, 0, "mismatch_cnt");
> +out:
> +	cgrp_local_storage__destroy(skel);
> +}
> +
> +static void test_recursion(int cgroup_fd)
> +{
> +	struct cgrp_ls_recursion *skel;
> +	int err;
> +
> +	skel = cgrp_ls_recursion__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> +		return;
> +
> +	err = cgrp_ls_recursion__attach(skel);
> +	if (!ASSERT_OK(err, "skel_attach"))
> +		goto out;
> +
> +	/* trigger sys_enter, make sure it does not cause deadlock */
> +	syscall(SYS_gettid);
> +
> +out:
> +	cgrp_ls_recursion__destroy(skel);
> +}
> +
> +void test_cgrp_local_storage(void)
> +{
> +	int cgroup_fd;
> +
> +	cgroup_fd = test__join_cgroup("/cgrp_local_storage");
> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup /cgrp_local_storage"))
> +		return;
> +
> +	if (test__start_subtest("sys_enter_exit"))
> +		test_sys_enter_exit(cgroup_fd);
> +	if (test__start_subtest("recursion"))
> +		test_recursion(cgroup_fd);
> +
> +	close(cgroup_fd);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cgrp_local_storage.c b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
> new file mode 100644
> index 000000000000..dee63d4c1512
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, long);
> +} cgrp_storage_1 SEC(".maps");
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, long);
> +} cgrp_storage_2 SEC(".maps");
> +
> +#define MAGIC_VALUE 0xabcd1234
> +
> +pid_t target_pid = 0;
> +int mismatch_cnt = 0;
> +int enter_cnt = 0;
> +int exit_cnt = 0;
> +
> +SEC("tp_btf/sys_enter")
> +int BPF_PROG(on_enter, struct pt_regs *regs, long id)
> +{
> +	struct task_struct *task;
> +	long *ptr;
> +	int err;
> +
> +	task = bpf_get_current_task_btf();
> +	if (task->pid != target_pid)
> +		return 0;
> +
> +	/* populate value 0 */
> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!ptr)
> +		return 0;
> +
> +	/* delete value 0 */
> +	err = bpf_cgrp_storage_delete(&cgrp_storage_1, task->cgroups->dfl_cgrp);
> +	if (err)
> +		return 0;
> +
> +	/* value is not available */
> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0, 0);
> +	if (ptr)
> +		return 0;
> +
> +	/* re-populate the value */
> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!ptr)
> +		return 0;

Should we also add a global int err variable to this program that we set
any time we can't fetch an instance of the local storage, etc  and then
check in the user space test progs logic?

> +	__sync_fetch_and_add(&enter_cnt, 1);
> +	*ptr = MAGIC_VALUE + enter_cnt;
> +
> +	return 0;
> +}
> +
> +SEC("tp_btf/sys_exit")
> +int BPF_PROG(on_exit, struct pt_regs *regs, long id)
> +{
> +	struct task_struct *task;
> +	long *ptr;
> +
> +	task = bpf_get_current_task_btf();
> +	if (task->pid != target_pid)
> +		return 0;
> +
> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!ptr)
> +		return 0;
> +
> +	__sync_fetch_and_add(&exit_cnt, 1);
> +	if (*ptr != MAGIC_VALUE + exit_cnt)
> +		__sync_fetch_and_add(&mismatch_cnt, 1);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
> new file mode 100644
> index 000000000000..a043d8fefdac
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, long);
> +} map_a SEC(".maps");
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, long);
> +} map_b SEC(".maps");
> +
> +SEC("fentry/bpf_local_storage_lookup")
> +int BPF_PROG(on_lookup)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +
> +	bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
> +	bpf_cgrp_storage_delete(&map_b, task->cgroups->dfl_cgrp);
> +	return 0;
> +}
> +
> +SEC("fentry/bpf_local_storage_update")
> +int BPF_PROG(on_update)
> +{
> +	struct task_struct *task = bpf_get_current_task_btf();
> +	long *ptr;
> +
> +	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (ptr)
> +		*ptr += 1;
> +
> +	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (ptr)
> +		*ptr += 1;
> +
> +	return 0;
> +}
> +
> +SEC("tp_btf/sys_enter")
> +int BPF_PROG(on_enter, struct pt_regs *regs, long id)
> +{
> +	struct task_struct *task;
> +	long *ptr;
> +
> +	task = bpf_get_current_task_btf();
> +	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (ptr)
> +		*ptr = 200;
> +
> +	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (ptr)
> +		*ptr = 100;
> +	return 0;
> +}
> -- 
> 2.30.2
> 


Looks reasonable overall. Should we also include any negative tests,
like e.g. trying to invoke bpf_cgrp_storage_get() with a struct other
than a cgroup?


Thanks,
David
Yonghong Song Oct. 24, 2022, 7:03 p.m. UTC | #2 
On 10/23/22 1:14 PM, David Vernet wrote:
> On Sun, Oct 23, 2022 at 11:05:46AM -0700, Yonghong Song wrote:
>> Add two tests for new cgroup local storage, one to test bpf program helpers
>> and user space map APIs, and the other to test recursive fentry
>> triggering won't deadlock.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../bpf/prog_tests/cgrp_local_storage.c       | 92 +++++++++++++++++++
>>   .../selftests/bpf/progs/cgrp_local_storage.c  | 88 ++++++++++++++++++
>>   .../selftests/bpf/progs/cgrp_ls_recursion.c   | 70 ++++++++++++++
>>   3 files changed, 250 insertions(+)
>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/cgrp_local_storage.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
>> new file mode 100644
>> index 000000000000..7ee21d598195
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates.*/
>> +
>> +#define _GNU_SOURCE
>> +#include <unistd.h>
>> +#include <sys/syscall.h>
>> +#include <sys/types.h>
>> +#include <test_progs.h>
>> +#include "cgrp_local_storage.skel.h"
>> +#include "cgrp_ls_recursion.skel.h"
>> +
>> +static void test_sys_enter_exit(int cgroup_fd)
>> +{
>> +	struct cgrp_local_storage *skel;
>> +	long val1 = 1, val2 = 0;
>> +	int err;
>> +
>> +	skel = cgrp_local_storage__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
>> +		return;
>> +
>> +	/* populate a value in cgrp_storage_2 */
>> +	err = bpf_map_update_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val1, BPF_ANY);
>> +	if (!ASSERT_OK(err, "map_update_elem"))
>> +		goto out;
>> +
>> +	/* check value */
>> +	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val2);
>> +	if (!ASSERT_OK(err, "map_lookup_elem"))
>> +		goto out;
>> +	if (!ASSERT_EQ(val2, 1, "map_lookup_elem, invalid val"))
>> +		goto out;
>> +
>> +	/* delete value */
>> +	err = bpf_map_delete_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd);
>> +	if (!ASSERT_OK(err, "map_delete_elem"))
>> +		goto out;
>> +
>> +	skel->bss->target_pid = syscall(SYS_gettid);
>> +
>> +	err = cgrp_local_storage__attach(skel);
>> +	if (!ASSERT_OK(err, "skel_attach"))
>> +		goto out;
>> +
>> +	syscall(SYS_gettid);
>> +	syscall(SYS_gettid);
>> +
>> +	skel->bss->target_pid = 0;
>> +
>> +	/* 3x syscalls: 1x attach and 2x gettid */
>> +	ASSERT_EQ(skel->bss->enter_cnt, 3, "enter_cnt");
>> +	ASSERT_EQ(skel->bss->exit_cnt, 3, "exit_cnt");
>> +	ASSERT_EQ(skel->bss->mismatch_cnt, 0, "mismatch_cnt");
>> +out:
>> +	cgrp_local_storage__destroy(skel);
>> +}
>> +
>> +static void test_recursion(int cgroup_fd)
>> +{
>> +	struct cgrp_ls_recursion *skel;
>> +	int err;
>> +
>> +	skel = cgrp_ls_recursion__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
>> +		return;
>> +
>> +	err = cgrp_ls_recursion__attach(skel);
>> +	if (!ASSERT_OK(err, "skel_attach"))
>> +		goto out;
>> +
>> +	/* trigger sys_enter, make sure it does not cause deadlock */
>> +	syscall(SYS_gettid);
>> +
>> +out:
>> +	cgrp_ls_recursion__destroy(skel);
>> +}
>> +
>> +void test_cgrp_local_storage(void)
>> +{
>> +	int cgroup_fd;
>> +
>> +	cgroup_fd = test__join_cgroup("/cgrp_local_storage");
>> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup /cgrp_local_storage"))
>> +		return;
>> +
>> +	if (test__start_subtest("sys_enter_exit"))
>> +		test_sys_enter_exit(cgroup_fd);
>> +	if (test__start_subtest("recursion"))
>> +		test_recursion(cgroup_fd);
>> +
>> +	close(cgroup_fd);
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/cgrp_local_storage.c b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
>> new file mode 100644
>> index 000000000000..dee63d4c1512
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
>> +	__uint(map_flags, BPF_F_NO_PREALLOC);
>> +	__type(key, int);
>> +	__type(value, long);
>> +} cgrp_storage_1 SEC(".maps");
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
>> +	__uint(map_flags, BPF_F_NO_PREALLOC);
>> +	__type(key, int);
>> +	__type(value, long);
>> +} cgrp_storage_2 SEC(".maps");
>> +
>> +#define MAGIC_VALUE 0xabcd1234
>> +
>> +pid_t target_pid = 0;
>> +int mismatch_cnt = 0;
>> +int enter_cnt = 0;
>> +int exit_cnt = 0;
>> +
>> +SEC("tp_btf/sys_enter")
>> +int BPF_PROG(on_enter, struct pt_regs *regs, long id)
>> +{
>> +	struct task_struct *task;
>> +	long *ptr;
>> +	int err;
>> +
>> +	task = bpf_get_current_task_btf();
>> +	if (task->pid != target_pid)
>> +		return 0;
>> +
>> +	/* populate value 0 */
>> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (!ptr)
>> +		return 0;
>> +
>> +	/* delete value 0 */
>> +	err = bpf_cgrp_storage_delete(&cgrp_storage_1, task->cgroups->dfl_cgrp);
>> +	if (err)
>> +		return 0;
>> +
>> +	/* value is not available */
>> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0, 0);
>> +	if (ptr)
>> +		return 0;
>> +
>> +	/* re-populate the value */
>> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (!ptr)
>> +		return 0;
> 
> Should we also add a global int err variable to this program that we set
> any time we can't fetch an instance of the local storage, etc  and then
> check in the user space test progs logic?

I think we are fine here. The enter_cnt below should ensure all above
should not prematurely return. Otherwise, the test will fail.

> 
>> +	__sync_fetch_and_add(&enter_cnt, 1);
>> +	*ptr = MAGIC_VALUE + enter_cnt;
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("tp_btf/sys_exit")
>> +int BPF_PROG(on_exit, struct pt_regs *regs, long id)
>> +{
>> +	struct task_struct *task;
>> +	long *ptr;
>> +
>> +	task = bpf_get_current_task_btf();
>> +	if (task->pid != target_pid)
>> +		return 0;
>> +
>> +	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (!ptr)
>> +		return 0;
>> +
>> +	__sync_fetch_and_add(&exit_cnt, 1);
>> +	if (*ptr != MAGIC_VALUE + exit_cnt)
>> +		__sync_fetch_and_add(&mismatch_cnt, 1);
>> +	return 0;
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
>> new file mode 100644
>> index 000000000000..a043d8fefdac
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
>> +	__uint(map_flags, BPF_F_NO_PREALLOC);
>> +	__type(key, int);
>> +	__type(value, long);
>> +} map_a SEC(".maps");
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
>> +	__uint(map_flags, BPF_F_NO_PREALLOC);
>> +	__type(key, int);
>> +	__type(value, long);
>> +} map_b SEC(".maps");
>> +
>> +SEC("fentry/bpf_local_storage_lookup")
>> +int BPF_PROG(on_lookup)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +
>> +	bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
>> +	bpf_cgrp_storage_delete(&map_b, task->cgroups->dfl_cgrp);
>> +	return 0;
>> +}
>> +
>> +SEC("fentry/bpf_local_storage_update")
>> +int BPF_PROG(on_update)
>> +{
>> +	struct task_struct *task = bpf_get_current_task_btf();
>> +	long *ptr;
>> +
>> +	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (ptr)
>> +		*ptr += 1;
>> +
>> +	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (ptr)
>> +		*ptr += 1;
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("tp_btf/sys_enter")
>> +int BPF_PROG(on_enter, struct pt_regs *regs, long id)
>> +{
>> +	struct task_struct *task;
>> +	long *ptr;
>> +
>> +	task = bpf_get_current_task_btf();
>> +	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (ptr)
>> +		*ptr = 200;
>> +
>> +	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
>> +				   BPF_LOCAL_STORAGE_GET_F_CREATE);
>> +	if (ptr)
>> +		*ptr = 100;
>> +	return 0;
>> +}
>> -- 
>> 2.30.2
>>
> 
> 
> Looks reasonable overall. Should we also include any negative tests,
> like e.g. trying to invoke bpf_cgrp_storage_get() with a struct other
> than a cgroup?

I can add one.

> 
> 
> Thanks,
> David
Martin KaFai Lau Oct. 24, 2022, 8:30 p.m. UTC | #3 
On 10/23/22 11:05 AM, Yonghong Song wrote:
> Add two tests for new cgroup local storage, one to test bpf program helpers
> and user space map APIs, and the other to test recursive fentry
> triggering won't deadlock.

Other than tracing, it will be very useful to add a bpf_cgrp_storage_get() usage 
in a cgroup-bpf prog.  Exercising this helper in the existing 
SEC(cgroup)/SEC(sockops) tests should be pretty easy.  eg. The 
SEC("cgroup/connect6") in socket_cookie_prog.c.
Yonghong Song Oct. 25, 2022, 2:26 a.m. UTC | #4 
On 10/24/22 1:30 PM, Martin KaFai Lau wrote:
> On 10/23/22 11:05 AM, Yonghong Song wrote:
>> Add two tests for new cgroup local storage, one to test bpf program 
>> helpers
>> and user space map APIs, and the other to test recursive fentry
>> triggering won't deadlock.
> 
> Other than tracing, it will be very useful to add a 
> bpf_cgrp_storage_get() usage in a cgroup-bpf prog.  Exercising this 
> helper in the existing SEC(cgroup)/SEC(sockops) tests should be pretty 
> easy.  eg. The SEC("cgroup/connect6") in socket_cookie_prog.c.

Will do.
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
new file mode 100644
index 000000000000..7ee21d598195
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
@@ -0,0 +1,92 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates.*/
+
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <test_progs.h>
+#include "cgrp_local_storage.skel.h"
+#include "cgrp_ls_recursion.skel.h"
+
+static void test_sys_enter_exit(int cgroup_fd)
+{
+	struct cgrp_local_storage *skel;
+	long val1 = 1, val2 = 0;
+	int err;
+
+	skel = cgrp_local_storage__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
+		return;
+
+	/* populate a value in cgrp_storage_2 */
+	err = bpf_map_update_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val1, BPF_ANY);
+	if (!ASSERT_OK(err, "map_update_elem"))
+		goto out;
+
+	/* check value */
+	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd, &val2);
+	if (!ASSERT_OK(err, "map_lookup_elem"))
+		goto out;
+	if (!ASSERT_EQ(val2, 1, "map_lookup_elem, invalid val"))
+		goto out;
+
+	/* delete value */
+	err = bpf_map_delete_elem(bpf_map__fd(skel->maps.cgrp_storage_2), &cgroup_fd);
+	if (!ASSERT_OK(err, "map_delete_elem"))
+		goto out;
+
+	skel->bss->target_pid = syscall(SYS_gettid);
+
+	err = cgrp_local_storage__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach"))
+		goto out;
+
+	syscall(SYS_gettid);
+	syscall(SYS_gettid);
+
+	skel->bss->target_pid = 0;
+
+	/* 3x syscalls: 1x attach and 2x gettid */
+	ASSERT_EQ(skel->bss->enter_cnt, 3, "enter_cnt");
+	ASSERT_EQ(skel->bss->exit_cnt, 3, "exit_cnt");
+	ASSERT_EQ(skel->bss->mismatch_cnt, 0, "mismatch_cnt");
+out:
+	cgrp_local_storage__destroy(skel);
+}
+
+static void test_recursion(int cgroup_fd)
+{
+	struct cgrp_ls_recursion *skel;
+	int err;
+
+	skel = cgrp_ls_recursion__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
+		return;
+
+	err = cgrp_ls_recursion__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach"))
+		goto out;
+
+	/* trigger sys_enter, make sure it does not cause deadlock */
+	syscall(SYS_gettid);
+
+out:
+	cgrp_ls_recursion__destroy(skel);
+}
+
+void test_cgrp_local_storage(void)
+{
+	int cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/cgrp_local_storage");
+	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup /cgrp_local_storage"))
+		return;
+
+	if (test__start_subtest("sys_enter_exit"))
+		test_sys_enter_exit(cgroup_fd);
+	if (test__start_subtest("recursion"))
+		test_recursion(cgroup_fd);
+
+	close(cgroup_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/cgrp_local_storage.c b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
new file mode 100644
index 000000000000..dee63d4c1512
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgrp_local_storage.c
@@ -0,0 +1,88 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, long);
+} cgrp_storage_1 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, long);
+} cgrp_storage_2 SEC(".maps");
+
+#define MAGIC_VALUE 0xabcd1234
+
+pid_t target_pid = 0;
+int mismatch_cnt = 0;
+int enter_cnt = 0;
+int exit_cnt = 0;
+
+SEC("tp_btf/sys_enter")
+int BPF_PROG(on_enter, struct pt_regs *regs, long id)
+{
+	struct task_struct *task;
+	long *ptr;
+	int err;
+
+	task = bpf_get_current_task_btf();
+	if (task->pid != target_pid)
+		return 0;
+
+	/* populate value 0 */
+	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (!ptr)
+		return 0;
+
+	/* delete value 0 */
+	err = bpf_cgrp_storage_delete(&cgrp_storage_1, task->cgroups->dfl_cgrp);
+	if (err)
+		return 0;
+
+	/* value is not available */
+	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0, 0);
+	if (ptr)
+		return 0;
+
+	/* re-populate the value */
+	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (!ptr)
+		return 0;
+	__sync_fetch_and_add(&enter_cnt, 1);
+	*ptr = MAGIC_VALUE + enter_cnt;
+
+	return 0;
+}
+
+SEC("tp_btf/sys_exit")
+int BPF_PROG(on_exit, struct pt_regs *regs, long id)
+{
+	struct task_struct *task;
+	long *ptr;
+
+	task = bpf_get_current_task_btf();
+	if (task->pid != target_pid)
+		return 0;
+
+	ptr = bpf_cgrp_storage_get(&cgrp_storage_1, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (!ptr)
+		return 0;
+
+	__sync_fetch_and_add(&exit_cnt, 1);
+	if (*ptr != MAGIC_VALUE + exit_cnt)
+		__sync_fetch_and_add(&mismatch_cnt, 1);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
new file mode 100644
index 000000000000..a043d8fefdac
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgrp_ls_recursion.c
@@ -0,0 +1,70 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, long);
+} map_a SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, long);
+} map_b SEC(".maps");
+
+SEC("fentry/bpf_local_storage_lookup")
+int BPF_PROG(on_lookup)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+
+	bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
+	bpf_cgrp_storage_delete(&map_b, task->cgroups->dfl_cgrp);
+	return 0;
+}
+
+SEC("fentry/bpf_local_storage_update")
+int BPF_PROG(on_update)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	long *ptr;
+
+	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (ptr)
+		*ptr += 1;
+
+	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (ptr)
+		*ptr += 1;
+
+	return 0;
+}
+
+SEC("tp_btf/sys_enter")
+int BPF_PROG(on_enter, struct pt_regs *regs, long id)
+{
+	struct task_struct *task;
+	long *ptr;
+
+	task = bpf_get_current_task_btf();
+	ptr = bpf_cgrp_storage_get(&map_a, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (ptr)
+		*ptr = 200;
+
+	ptr = bpf_cgrp_storage_get(&map_b, task->cgroups->dfl_cgrp, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE);
+	if (ptr)
+		*ptr = 100;
+	return 0;
+}