2022-05-16 03:18:10

by Yuntao Wang

[permalink] [raw]
Subject: [PATCH bpf-next] selftests/bpf: Add missing trampoline program type to trampoline_count test

Currently the trampoline_count test doesn't include any fmod_ret bpf
programs, fix it to make the test cover all possible trampoline program
types.

Since fmod_ret bpf programs can't be attached to __set_task_comm function,
as it's neither whitelisted for error injection nor a security hook, change
it to bpf_modify_return_test.

This patch also does some other cleanups such as removing duplicate code,
dropping inconsistent comments, etc.

Signed-off-by: Yuntao Wang <[email protected]>
---
include/linux/bpf.h | 2 +-
.../bpf/prog_tests/trampoline_count.c | 121 ++++++------------
.../bpf/progs/test_trampoline_count.c | 16 ++-
3 files changed, 47 insertions(+), 92 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c107392b0ba7..cb01f247e05f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -724,7 +724,7 @@ struct btf_func_model {
#define BPF_TRAMP_F_RET_FENTRY_RET BIT(4)

/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
- * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
+ * bytes on x86.
*/
#define BPF_MAX_TRAMP_LINKS 38

diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
index 9c795ee52b7b..b1d25998cc48 100644
--- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
+++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
@@ -1,126 +1,79 @@
// SPDX-License-Identifier: GPL-2.0-only
#define _GNU_SOURCE
-#include <sched.h>
-#include <sys/prctl.h>
#include <test_progs.h>

#define MAX_TRAMP_PROGS 38

struct inst {
struct bpf_object *obj;
- struct bpf_link *link_fentry;
- struct bpf_link *link_fexit;
+ struct bpf_link *link;
};

-static int test_task_rename(void)
-{
- int fd, duration = 0, err;
- char buf[] = "test_overhead";
-
- fd = open("/proc/self/comm", O_WRONLY|O_TRUNC);
- if (CHECK(fd < 0, "open /proc", "err %d", errno))
- return -1;
- err = write(fd, buf, sizeof(buf));
- if (err < 0) {
- CHECK(err < 0, "task rename", "err %d", errno);
- close(fd);
- return -1;
- }
- close(fd);
- return 0;
-}
-
-static struct bpf_link *load(struct bpf_object *obj, const char *name)
+static struct bpf_program *load_prog(char *file, char *name, struct inst *inst)
{
+ struct bpf_object *obj;
struct bpf_program *prog;
- int duration = 0;
+ int err;
+
+ obj = bpf_object__open_file(file, NULL);
+ if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ return NULL;
+
+ inst->obj = obj;
+
+ err = bpf_object__load(obj);
+ if (!ASSERT_OK(err, "obj_load"))
+ return NULL;

prog = bpf_object__find_program_by_name(obj, name);
- if (CHECK(!prog, "find_probe", "prog '%s' not found\n", name))
- return ERR_PTR(-EINVAL);
- return bpf_program__attach_trace(prog);
+ if (!ASSERT_OK_PTR(prog, "obj_find_prog"))
+ return NULL;
+
+ return prog;
}

/* TODO: use different target function to run in concurrent mode */
void serial_test_trampoline_count(void)
{
- const char *fentry_name = "prog1";
- const char *fexit_name = "prog2";
- const char *object = "test_trampoline_count.o";
- struct inst inst[MAX_TRAMP_PROGS] = {};
- int err, i = 0, duration = 0;
- struct bpf_object *obj;
+ char *file = "test_trampoline_count.o";
+ char *const progs[] = { "fentry_test", "fmod_ret_test", "fexit_test" };
+ struct inst inst[MAX_TRAMP_PROGS + 1] = {};
+ struct bpf_program *prog;
struct bpf_link *link;
- char comm[16] = {};
+ int i;

/* attach 'allowed' trampoline programs */
for (i = 0; i < MAX_TRAMP_PROGS; i++) {
- obj = bpf_object__open_file(object, NULL);
- if (!ASSERT_OK_PTR(obj, "obj_open_file")) {
- obj = NULL;
+ prog = load_prog(file, progs[i % ARRAY_SIZE(progs)], &inst[i]);
+ if (!prog)
goto cleanup;
- }

- err = bpf_object__load(obj);
- if (CHECK(err, "obj_load", "err %d\n", err))
+ link = bpf_program__attach(prog);
+ if (!ASSERT_OK_PTR(link, "attach_prog"))
goto cleanup;
- inst[i].obj = obj;
- obj = NULL;
-
- if (rand() % 2) {
- link = load(inst[i].obj, fentry_name);
- if (!ASSERT_OK_PTR(link, "attach_prog")) {
- link = NULL;
- goto cleanup;
- }
- inst[i].link_fentry = link;
- } else {
- link = load(inst[i].obj, fexit_name);
- if (!ASSERT_OK_PTR(link, "attach_prog")) {
- link = NULL;
- goto cleanup;
- }
- inst[i].link_fexit = link;
- }
+
+ inst[i].link = link;
}

/* and try 1 extra.. */
- obj = bpf_object__open_file(object, NULL);
- if (!ASSERT_OK_PTR(obj, "obj_open_file")) {
- obj = NULL;
+ prog = load_prog(file, progs[0], &inst[i]);
+ if (!prog)
goto cleanup;
- }
-
- err = bpf_object__load(obj);
- if (CHECK(err, "obj_load", "err %d\n", err))
- goto cleanup_extra;

/* ..that needs to fail */
- link = load(obj, fentry_name);
- err = libbpf_get_error(link);
- if (!ASSERT_ERR_PTR(link, "cannot attach over the limit")) {
- bpf_link__destroy(link);
- goto cleanup_extra;
+ link = bpf_program__attach(prog);
+ if (!ASSERT_ERR_PTR(link, "attach_prog")) {
+ inst[i].link = link;
+ goto cleanup;
}

/* with E2BIG error */
- ASSERT_EQ(err, -E2BIG, "proper error check");
+ ASSERT_EQ(libbpf_get_error(link), -E2BIG, "E2BIG");
ASSERT_EQ(link, NULL, "ptr_is_null");

- /* and finaly execute the probe */
- if (CHECK_FAIL(prctl(PR_GET_NAME, comm, 0L, 0L, 0L)))
- goto cleanup_extra;
- CHECK_FAIL(test_task_rename());
- CHECK_FAIL(prctl(PR_SET_NAME, comm, 0L, 0L, 0L));
-
-cleanup_extra:
- bpf_object__close(obj);
cleanup:
- if (i >= MAX_TRAMP_PROGS)
- i = MAX_TRAMP_PROGS - 1;
for (; i >= 0; i--) {
- bpf_link__destroy(inst[i].link_fentry);
- bpf_link__destroy(inst[i].link_fexit);
+ bpf_link__destroy(inst[i].link);
bpf_object__close(inst[i].obj);
}
}
diff --git a/tools/testing/selftests/bpf/progs/test_trampoline_count.c b/tools/testing/selftests/bpf/progs/test_trampoline_count.c
index f030e469d05b..7765720da7d5 100644
--- a/tools/testing/selftests/bpf/progs/test_trampoline_count.c
+++ b/tools/testing/selftests/bpf/progs/test_trampoline_count.c
@@ -1,20 +1,22 @@
// SPDX-License-Identifier: GPL-2.0
-#include <stdbool.h>
-#include <stddef.h>
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>

-struct task_struct;
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(fentry_test, int a, int *b)
+{
+ return 0;
+}

-SEC("fentry/__set_task_comm")
-int BPF_PROG(prog1, struct task_struct *tsk, const char *buf, bool exec)
+SEC("fmod_ret/bpf_modify_return_test")
+int BPF_PROG(fmod_ret_test, int a, int *b, int ret)
{
return 0;
}

-SEC("fexit/__set_task_comm")
-int BPF_PROG(prog2, struct task_struct *tsk, const char *buf, bool exec)
+SEC("fexit/bpf_modify_return_test")
+int BPF_PROG(fexit_test, int a, int *b, int ret)
{
return 0;
}
--
2.36.1



2022-05-17 09:44:19

by Yonghong Song

[permalink] [raw]
Subject: Re: [PATCH bpf-next] selftests/bpf: Add missing trampoline program type to trampoline_count test



On 5/14/22 11:31 PM, Yuntao Wang wrote:
> Currently the trampoline_count test doesn't include any fmod_ret bpf
> programs, fix it to make the test cover all possible trampoline program
> types.
>
> Since fmod_ret bpf programs can't be attached to __set_task_comm function,
> as it's neither whitelisted for error injection nor a security hook, change
> it to bpf_modify_return_test.
>
> This patch also does some other cleanups such as removing duplicate code,
> dropping inconsistent comments, etc.
>
> Signed-off-by: Yuntao Wang <[email protected]>

Acked-by: Yonghong Song <[email protected]>

2022-05-19 10:23:16

by Andrii Nakryiko

[permalink] [raw]
Subject: Re: [PATCH bpf-next] selftests/bpf: Add missing trampoline program type to trampoline_count test

On Sat, May 14, 2022 at 11:31 PM Yuntao Wang <[email protected]> wrote:
>
> Currently the trampoline_count test doesn't include any fmod_ret bpf
> programs, fix it to make the test cover all possible trampoline program
> types.
>
> Since fmod_ret bpf programs can't be attached to __set_task_comm function,
> as it's neither whitelisted for error injection nor a security hook, change
> it to bpf_modify_return_test.
>
> This patch also does some other cleanups such as removing duplicate code,
> dropping inconsistent comments, etc.
>
> Signed-off-by: Yuntao Wang <[email protected]>
> ---
> include/linux/bpf.h | 2 +-
> .../bpf/prog_tests/trampoline_count.c | 121 ++++++------------
> .../bpf/progs/test_trampoline_count.c | 16 ++-
> 3 files changed, 47 insertions(+), 92 deletions(-)
>

[...]

>
> /* with E2BIG error */
> - ASSERT_EQ(err, -E2BIG, "proper error check");
> + ASSERT_EQ(libbpf_get_error(link), -E2BIG, "E2BIG");
> ASSERT_EQ(link, NULL, "ptr_is_null");
>
> - /* and finaly execute the probe */
> - if (CHECK_FAIL(prctl(PR_GET_NAME, comm, 0L, 0L, 0L)))
> - goto cleanup_extra;
> - CHECK_FAIL(test_task_rename());

we stopped testing that kernel function actually can be called
properly, why don't you do bpf_prog_test_run() here to trigger
bpf_modify_return_test in kernel?

> - CHECK_FAIL(prctl(PR_SET_NAME, comm, 0L, 0L, 0L));
> -
> -cleanup_extra:
> - bpf_object__close(obj);
> cleanup:
> - if (i >= MAX_TRAMP_PROGS)
> - i = MAX_TRAMP_PROGS - 1;
> for (; i >= 0; i--) {
> - bpf_link__destroy(inst[i].link_fentry);
> - bpf_link__destroy(inst[i].link_fexit);
> + bpf_link__destroy(inst[i].link);
> bpf_object__close(inst[i].obj);
> }
> }

[...]

2022-05-20 18:37:14

by Yuntao Wang

[permalink] [raw]
Subject: [PATCH bpf-next v2] selftests/bpf: Add missing trampoline program type to trampoline_count test

Currently the trampoline_count test doesn't include any fmod_ret bpf
programs, fix it to make the test cover all possible trampoline program
types.

Since fmod_ret bpf programs can't be attached to __set_task_comm function,
as it's neither whitelisted for error injection nor a security hook, change
it to bpf_modify_return_test.

This patch also does some other cleanups such as removing duplicate code,
dropping inconsistent comments, etc.

Signed-off-by: Yuntao Wang <[email protected]>
Acked-by: Yonghong Song <[email protected]>
---
v1->v2: call bpf_prog_test_run() to trigger bpf_modify_return_test in kernel

include/linux/bpf.h | 2 +-
.../bpf/prog_tests/trampoline_count.c | 134 +++++++-----------
.../bpf/progs/test_trampoline_count.c | 16 ++-
3 files changed, 61 insertions(+), 91 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c107392b0ba7..cb01f247e05f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -724,7 +724,7 @@ struct btf_func_model {
#define BPF_TRAMP_F_RET_FENTRY_RET BIT(4)

/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
- * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
+ * bytes on x86.
*/
#define BPF_MAX_TRAMP_LINKS 38

diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
index 9c795ee52b7b..b0acbda6dbf5 100644
--- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
+++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
@@ -1,126 +1,94 @@
// SPDX-License-Identifier: GPL-2.0-only
#define _GNU_SOURCE
-#include <sched.h>
-#include <sys/prctl.h>
#include <test_progs.h>

#define MAX_TRAMP_PROGS 38

struct inst {
struct bpf_object *obj;
- struct bpf_link *link_fentry;
- struct bpf_link *link_fexit;
+ struct bpf_link *link;
};

-static int test_task_rename(void)
-{
- int fd, duration = 0, err;
- char buf[] = "test_overhead";
-
- fd = open("/proc/self/comm", O_WRONLY|O_TRUNC);
- if (CHECK(fd < 0, "open /proc", "err %d", errno))
- return -1;
- err = write(fd, buf, sizeof(buf));
- if (err < 0) {
- CHECK(err < 0, "task rename", "err %d", errno);
- close(fd);
- return -1;
- }
- close(fd);
- return 0;
-}
-
-static struct bpf_link *load(struct bpf_object *obj, const char *name)
+static struct bpf_program *load_prog(char *file, char *name, struct inst *inst)
{
+ struct bpf_object *obj;
struct bpf_program *prog;
- int duration = 0;
+ int err;
+
+ obj = bpf_object__open_file(file, NULL);
+ if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ return NULL;
+
+ inst->obj = obj;
+
+ err = bpf_object__load(obj);
+ if (!ASSERT_OK(err, "obj_load"))
+ return NULL;

prog = bpf_object__find_program_by_name(obj, name);
- if (CHECK(!prog, "find_probe", "prog '%s' not found\n", name))
- return ERR_PTR(-EINVAL);
- return bpf_program__attach_trace(prog);
+ if (!ASSERT_OK_PTR(prog, "obj_find_prog"))
+ return NULL;
+
+ return prog;
}

/* TODO: use different target function to run in concurrent mode */
void serial_test_trampoline_count(void)
{
- const char *fentry_name = "prog1";
- const char *fexit_name = "prog2";
- const char *object = "test_trampoline_count.o";
- struct inst inst[MAX_TRAMP_PROGS] = {};
- int err, i = 0, duration = 0;
- struct bpf_object *obj;
+ char *file = "test_trampoline_count.o";
+ char *const progs[] = { "fentry_test", "fmod_ret_test", "fexit_test" };
+ struct inst inst[MAX_TRAMP_PROGS + 1] = {};
+ struct bpf_program *prog;
struct bpf_link *link;
- char comm[16] = {};
+ int prog_fd, err, i;
+ LIBBPF_OPTS(bpf_test_run_opts, opts);

/* attach 'allowed' trampoline programs */
for (i = 0; i < MAX_TRAMP_PROGS; i++) {
- obj = bpf_object__open_file(object, NULL);
- if (!ASSERT_OK_PTR(obj, "obj_open_file")) {
- obj = NULL;
+ prog = load_prog(file, progs[i % ARRAY_SIZE(progs)], &inst[i]);
+ if (!prog)
goto cleanup;
- }

- err = bpf_object__load(obj);
- if (CHECK(err, "obj_load", "err %d\n", err))
+ link = bpf_program__attach(prog);
+ if (!ASSERT_OK_PTR(link, "attach_prog"))
goto cleanup;
- inst[i].obj = obj;
- obj = NULL;
-
- if (rand() % 2) {
- link = load(inst[i].obj, fentry_name);
- if (!ASSERT_OK_PTR(link, "attach_prog")) {
- link = NULL;
- goto cleanup;
- }
- inst[i].link_fentry = link;
- } else {
- link = load(inst[i].obj, fexit_name);
- if (!ASSERT_OK_PTR(link, "attach_prog")) {
- link = NULL;
- goto cleanup;
- }
- inst[i].link_fexit = link;
- }
+
+ inst[i].link = link;
}

/* and try 1 extra.. */
- obj = bpf_object__open_file(object, NULL);
- if (!ASSERT_OK_PTR(obj, "obj_open_file")) {
- obj = NULL;
+ prog = load_prog(file, "fmod_ret_test", &inst[i]);
+ if (!prog)
goto cleanup;
- }
-
- err = bpf_object__load(obj);
- if (CHECK(err, "obj_load", "err %d\n", err))
- goto cleanup_extra;

/* ..that needs to fail */
- link = load(obj, fentry_name);
- err = libbpf_get_error(link);
- if (!ASSERT_ERR_PTR(link, "cannot attach over the limit")) {
- bpf_link__destroy(link);
- goto cleanup_extra;
+ link = bpf_program__attach(prog);
+ if (!ASSERT_ERR_PTR(link, "attach_prog")) {
+ inst[i].link = link;
+ goto cleanup;
}

/* with E2BIG error */
- ASSERT_EQ(err, -E2BIG, "proper error check");
- ASSERT_EQ(link, NULL, "ptr_is_null");
+ if (!ASSERT_EQ(libbpf_get_error(link), -E2BIG, "E2BIG"))
+ goto cleanup;
+ if (!ASSERT_EQ(link, NULL, "ptr_is_null"))
+ goto cleanup;

/* and finaly execute the probe */
- if (CHECK_FAIL(prctl(PR_GET_NAME, comm, 0L, 0L, 0L)))
- goto cleanup_extra;
- CHECK_FAIL(test_task_rename());
- CHECK_FAIL(prctl(PR_SET_NAME, comm, 0L, 0L, 0L));
+ prog_fd = bpf_program__fd(prog);
+ if (!ASSERT_GE(prog_fd, 0, "bpf_program__fd"))
+ goto cleanup;
+
+ err = bpf_prog_test_run_opts(prog_fd, &opts);
+ if (!ASSERT_OK(err, "bpf_prog_test_run_opts"))
+ goto cleanup;
+
+ ASSERT_EQ(opts.retval & 0xffff, 4, "bpf_modify_return_test.result");
+ ASSERT_EQ(opts.retval >> 16, 1, "bpf_modify_return_test.side_effect");

-cleanup_extra:
- bpf_object__close(obj);
cleanup:
- if (i >= MAX_TRAMP_PROGS)
- i = MAX_TRAMP_PROGS - 1;
for (; i >= 0; i--) {
- bpf_link__destroy(inst[i].link_fentry);
- bpf_link__destroy(inst[i].link_fexit);
+ bpf_link__destroy(inst[i].link);
bpf_object__close(inst[i].obj);
}
}
diff --git a/tools/testing/selftests/bpf/progs/test_trampoline_count.c b/tools/testing/selftests/bpf/progs/test_trampoline_count.c
index f030e469d05b..7765720da7d5 100644
--- a/tools/testing/selftests/bpf/progs/test_trampoline_count.c
+++ b/tools/testing/selftests/bpf/progs/test_trampoline_count.c
@@ -1,20 +1,22 @@
// SPDX-License-Identifier: GPL-2.0
-#include <stdbool.h>
-#include <stddef.h>
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>

-struct task_struct;
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(fentry_test, int a, int *b)
+{
+ return 0;
+}

-SEC("fentry/__set_task_comm")
-int BPF_PROG(prog1, struct task_struct *tsk, const char *buf, bool exec)
+SEC("fmod_ret/bpf_modify_return_test")
+int BPF_PROG(fmod_ret_test, int a, int *b, int ret)
{
return 0;
}

-SEC("fexit/__set_task_comm")
-int BPF_PROG(prog2, struct task_struct *tsk, const char *buf, bool exec)
+SEC("fexit/bpf_modify_return_test")
+int BPF_PROG(fexit_test, int a, int *b, int ret)
{
return 0;
}
--
2.36.1


2022-05-22 08:59:12

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [PATCH bpf-next v2] selftests/bpf: Add missing trampoline program type to trampoline_count test

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[email protected]>:

On Thu, 19 May 2022 23:06:10 +0800 you wrote:
> Currently the trampoline_count test doesn't include any fmod_ret bpf
> programs, fix it to make the test cover all possible trampoline program
> types.
>
> Since fmod_ret bpf programs can't be attached to __set_task_comm function,
> as it's neither whitelisted for error injection nor a security hook, change
> it to bpf_modify_return_test.
>
> [...]

Here is the summary with links:
- [bpf-next,v2] selftests/bpf: Add missing trampoline program type to trampoline_count test
https://git.kernel.org/bpf/bpf-next/c/b23316aabffa

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html