[PATCHv2 bpf-next 0/5] bpf: Tracing and lsm programs re-attach

[PATCHv2 bpf-next 0/5] bpf: Tracing and lsm programs re-attach

[Jiri Olsa]

hi,
while adding test for pinning the module while there's
trampoline attach to it, I noticed that we don't allow
link detach and following re-attach for trampolines.
Adding that for tracing and lsm programs.

You need to have patch [1] from bpf tree for test module
attach test to pass.

v2 changes:
  - allow re-attach for TRACING and LSM programs
  - add lsm re-attach test

thanks,
jirka


[1] https://lore.kernel.org/bpf/20210326105900.151466-1-jolsa@kernel.org/
---
Jiri Olsa (5):
      bpf: Allow trampoline re-attach for tracing and lsm programs
      selftests/bpf: Add re-attach test to fentry_test
      selftests/bpf: Add re-attach test to fexit_test
      selftests/bpf: Add re-attach test to lsm test
      selftests/bpf: Test that module can't be unloaded with attached trampoline

 kernel/bpf/syscall.c                                   | 23 +++++++++++++++++------
 kernel/bpf/trampoline.c                                |  2 +-
 tools/testing/selftests/bpf/prog_tests/fentry_test.c   | 48 ++++++++++++++++++++++++++++++++++++++----------
 tools/testing/selftests/bpf/prog_tests/fexit_test.c    | 48 ++++++++++++++++++++++++++++++++++++++----------
 tools/testing/selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/test_lsm.c      | 48 ++++++++++++++++++++++++++++++++++++++----------
 6 files changed, 155 insertions(+), 37 deletions(-)

[PATCHv2 bpf-next 1/5] bpf: Allow trampoline re-attach for tracing and lsm programs

[Jiri Olsa]

Currently we don't allow re-attaching of trampolines. Once
it's detached, it can't be re-attach even when the program
is still loaded.

Adding the possibility to re-attach the loaded tracing and
lsm programs.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/syscall.c    | 23 +++++++++++++++++------
 kernel/bpf/trampoline.c |  2 +-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6428634da57e..f416735b20b3 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2645,14 +2645,25 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	 *   target_btf_id using the link_create API.
 	 *
 	 * - if tgt_prog == NULL when this function was called using the old
-         *   raw_tracepoint_open API, and we need a target from prog->aux
-         *
-         * The combination of no saved target in prog->aux, and no target
-         * specified on load is illegal, and we reject that here.
+	 *   raw_tracepoint_open API, and we need a target from prog->aux
+	 *
+	 * - if prog->aux->dst_trampoline and tgt_prog is NULL, the program
+	 *   was detached and is going for re-attachment.
 	 */
 	if (!prog->aux->dst_trampoline && !tgt_prog) {
-		err = -ENOENT;
-		goto out_unlock;
+		/*
+		 * Allow re-attach for TRACING and LSM programs. If it's
+		 * currently linked, bpf_trampoline_link_prog will fail.
+		 * EXT programs need to specify tgt_prog_fd, so they
+		 * re-attach in separate code path.
+		 */
+		if (prog->type != BPF_PROG_TYPE_TRACING &&
+		    prog->type != BPF_PROG_TYPE_LSM) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
+		btf_id = prog->aux->attach_btf_id;
+		key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
 	}
 
 	if (!prog->aux->dst_trampoline ||
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 4aa8b52adf25..0d937c63fc22 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -467,7 +467,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
 		tr->extension_prog = NULL;
 		goto out;
 	}
-	hlist_del(&prog->aux->tramp_hlist);
+	hlist_del_init(&prog->aux->tramp_hlist);
 	tr->progs_cnt[kind]--;
 	err = bpf_trampoline_update(tr);
 out:
-- 
2.30.2

[PATCHv2 bpf-next 2/5] selftests/bpf: Add re-attach test to fentry_test

[Jiri Olsa]

Adding the test to re-attach (detach/attach again) tracing
fentry programs, plus check that already linked program can't
be attached again.

Fixing the number of check-ed results, which should be 8.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/fentry_test.c    | 48 +++++++++++++++----
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_test.c b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
index 04ebbf1cb390..1f7566e772e9 100644
--- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
@@ -3,20 +3,24 @@
 #include <test_progs.h>
 #include "fentry_test.skel.h"
 
-void test_fentry_test(void)
+static __u32 duration;
+
+static int fentry_test(struct fentry_test *fentry_skel)
 {
-	struct fentry_test *fentry_skel = NULL;
+	struct bpf_link *link;
 	int err, prog_fd, i;
-	__u32 duration = 0, retval;
 	__u64 *result;
-
-	fentry_skel = fentry_test__open_and_load();
-	if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
-		goto cleanup;
+	__u32 retval;
 
 	err = fentry_test__attach(fentry_skel);
 	if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
-		goto cleanup;
+		return err;
+
+	/* Check that already linked program can't be attached again. */
+	link = bpf_program__attach(fentry_skel->progs.test1);
+	if (CHECK(!IS_ERR(link), "fentry_attach_link",
+		  "re-attach without detach should not succeed"))
+		return -1;
 
 	prog_fd = bpf_program__fd(fentry_skel->progs.test1);
 	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
@@ -26,12 +30,36 @@ void test_fentry_test(void)
 	      err, errno, retval, duration);
 
 	result = (__u64 *)fentry_skel->bss;
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < 8; i++) {
 		if (CHECK(result[i] != 1, "result",
 			  "fentry_test%d failed err %lld\n", i + 1, result[i]))
-			goto cleanup;
+			return -1;
 	}
 
+	fentry_test__detach(fentry_skel);
+
+	/* zero results for re-attach test */
+	for (i = 0; i < 8; i++)
+		result[i] = 0;
+	return 0;
+}
+
+void test_fentry_test(void)
+{
+	struct fentry_test *fentry_skel = NULL;
+	int err;
+
+	fentry_skel = fentry_test__open_and_load();
+	if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
+		goto cleanup;
+
+	err = fentry_test(fentry_skel);
+	if (CHECK(err, "fentry_test", "first attach failed\n"))
+		goto cleanup;
+
+	err = fentry_test(fentry_skel);
+	CHECK(err, "fentry_test", "second attach failed\n");
+
 cleanup:
 	fentry_test__destroy(fentry_skel);
 }
-- 
2.30.2

[PATCHv2 bpf-next 3/5] selftests/bpf: Add re-attach test to fexit_test

[Jiri Olsa]

Adding the test to re-attach (detach/attach again) tracing
fexit programs, plus check that already linked program can't
be attached again.

Fixing the number of check-ed results, which should be 8.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/fexit_test.c     | 48 +++++++++++++++----
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
index 78d7a2765c27..579e620e6612 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
@@ -3,20 +3,24 @@
 #include <test_progs.h>
 #include "fexit_test.skel.h"
 
-void test_fexit_test(void)
+static __u32 duration;
+
+static int fexit_test(struct fexit_test *fexit_skel)
 {
-	struct fexit_test *fexit_skel = NULL;
+	struct bpf_link *link;
 	int err, prog_fd, i;
-	__u32 duration = 0, retval;
 	__u64 *result;
-
-	fexit_skel = fexit_test__open_and_load();
-	if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
-		goto cleanup;
+	__u32 retval;
 
 	err = fexit_test__attach(fexit_skel);
 	if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
-		goto cleanup;
+		return err;
+
+	/* Check that already linked program can't be attached again. */
+	link = bpf_program__attach(fexit_skel->progs.test1);
+	if (CHECK(!IS_ERR(link), "fexit_attach_link",
+		  "re-attach without detach should not succeed"))
+		return -1;
 
 	prog_fd = bpf_program__fd(fexit_skel->progs.test1);
 	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
@@ -26,12 +30,36 @@ void test_fexit_test(void)
 	      err, errno, retval, duration);
 
 	result = (__u64 *)fexit_skel->bss;
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < 8; i++) {
 		if (CHECK(result[i] != 1, "result",
 			  "fexit_test%d failed err %lld\n", i + 1, result[i]))
-			goto cleanup;
+			return -1;
 	}
 
+	fexit_test__detach(fexit_skel);
+
+	/* zero results for re-attach test */
+	for (i = 0; i < 8; i++)
+		result[i] = 0;
+	return 0;
+}
+
+void test_fexit_test(void)
+{
+	struct fexit_test *fexit_skel = NULL;
+	int err;
+
+	fexit_skel = fexit_test__open_and_load();
+	if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
+		goto cleanup;
+
+	err = fexit_test(fexit_skel);
+	if (CHECK(err, "fexit_test", "first attach failed\n"))
+		goto cleanup;
+
+	err = fexit_test(fexit_skel);
+	CHECK(err, "fexit_test", "second attach failed\n");
+
 cleanup:
 	fexit_test__destroy(fexit_skel);
 }
-- 
2.30.2

[PATCHv2 bpf-next 4/5] selftests/bpf: Add re-attach test to lsm test

[Jiri Olsa]

Adding the test to re-attach (detach/attach again) lsm programs,
plus check that already linked program can't be attached again.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/test_lsm.c       | 48 +++++++++++++++----
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_lsm.c b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
index 2755e4f81499..d492e76e01cf 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_lsm.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
@@ -18,6 +18,8 @@ char *CMD_ARGS[] = {"true", NULL};
 #define GET_PAGE_ADDR(ADDR, PAGE_SIZE)					\
 	(char *)(((unsigned long) (ADDR + PAGE_SIZE)) & ~(PAGE_SIZE-1))
 
+static int duration = 0;
+
 int stack_mprotect(void)
 {
 	void *buf;
@@ -51,23 +53,25 @@ int exec_cmd(int *monitored_pid)
 	return -EINVAL;
 }
 
-void test_test_lsm(void)
+static int test_lsm(struct lsm *skel)
 {
-	struct lsm *skel = NULL;
-	int err, duration = 0;
+	struct bpf_link *link;
 	int buf = 1234;
-
-	skel = lsm__open_and_load();
-	if (CHECK(!skel, "skel_load", "lsm skeleton failed\n"))
-		goto close_prog;
+	int err;
 
 	err = lsm__attach(skel);
 	if (CHECK(err, "attach", "lsm attach failed: %d\n", err))
-		goto close_prog;
+		return err;
+
+	/* Check that already linked program can't be attached again. */
+	link = bpf_program__attach(skel->progs.test_int_hook);
+	if (CHECK(!IS_ERR(link), "attach_link",
+		  "re-attach without detach should not succeed"))
+		return -1;
 
 	err = exec_cmd(&skel->bss->monitored_pid);
 	if (CHECK(err < 0, "exec_cmd", "err %d errno %d\n", err, errno))
-		goto close_prog;
+		return err;
 
 	CHECK(skel->bss->bprm_count != 1, "bprm_count", "bprm_count = %d\n",
 	      skel->bss->bprm_count);
@@ -77,7 +81,7 @@ void test_test_lsm(void)
 	err = stack_mprotect();
 	if (CHECK(errno != EPERM, "stack_mprotect", "want err=EPERM, got %d\n",
 		  errno))
-		goto close_prog;
+		return err;
 
 	CHECK(skel->bss->mprotect_count != 1, "mprotect_count",
 	      "mprotect_count = %d\n", skel->bss->mprotect_count);
@@ -89,6 +93,30 @@ void test_test_lsm(void)
 	CHECK(skel->bss->copy_test != 3, "copy_test",
 	      "copy_test = %d\n", skel->bss->copy_test);
 
+	lsm__detach(skel);
+
+	skel->bss->copy_test = 0;
+	skel->bss->bprm_count = 0;
+	skel->bss->mprotect_count = 0;
+	return 0;
+}
+
+void test_test_lsm(void)
+{
+	struct lsm *skel = NULL;
+	int err;
+
+	skel = lsm__open_and_load();
+	if (CHECK(!skel, "lsm_skel_load", "lsm skeleton failed\n"))
+		goto close_prog;
+
+	err = test_lsm(skel);
+	if (CHECK(err, "test_lsm", "first attach failed\n"))
+		goto close_prog;
+
+	err = test_lsm(skel);
+	CHECK(err, "test_lsm", "second attach failed\n");
+
 close_prog:
 	lsm__destroy(skel);
 }
-- 
2.30.2

[PATCHv2 bpf-next 5/5] selftests/bpf: Test that module can't be unloaded with attached trampoline

[Jiri Olsa]

Adding test to verify that once we attach module's trampoline,
the module can't be unloaded.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/module_attach.c  | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index 5bc53d53d86e..d180b8c28287 100644
--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
@@ -45,12 +45,18 @@ static int trigger_module_test_write(int write_sz)
 	return 0;
 }
 
+static int delete_module(const char *name, int flags)
+{
+	return syscall(__NR_delete_module, name, flags);
+}
+
 void test_module_attach(void)
 {
 	const int READ_SZ = 456;
 	const int WRITE_SZ = 457;
 	struct test_module_attach* skel;
 	struct test_module_attach__bss *bss;
+	struct bpf_link *link;
 	int err;
 
 	skel = test_module_attach__open();
@@ -84,6 +90,23 @@ void test_module_attach(void)
 	ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet");
 	ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret");
 
+	test_module_attach__detach(skel);
+
+	/* attach fentry/fexit and make sure it get's module reference */
+	link = bpf_program__attach(skel->progs.handle_fentry);
+	if (CHECK(IS_ERR(link), "attach_fentry", "err: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+
+	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	bpf_link__destroy(link);
+
+	link = bpf_program__attach(skel->progs.handle_fexit);
+	if (CHECK(IS_ERR(link), "attach_fexit", "err: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+
+	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	bpf_link__destroy(link);
+
 cleanup:
 	test_module_attach__destroy(skel);
 }
-- 
2.30.2

Re: [PATCHv2 bpf-next 1/5] bpf: Allow trampoline re-attach for tracing and lsm programs

[Toke Høiland-Jørgensen]

Jiri Olsa <jolsa@kernel.org> writes:

> Currently we don't allow re-attaching of trampolines. Once
> it's detached, it can't be re-attach even when the program
> is still loaded.
>
> Adding the possibility to re-attach the loaded tracing and
> lsm programs.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCHv2 bpf-next 2/5] selftests/bpf: Add re-attach test to fentry_test

[Andrii Nakryiko]

On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding the test to re-attach (detach/attach again) tracing
> fentry programs, plus check that already linked program can't
> be attached again.
>
> Fixing the number of check-ed results, which should be 8.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/fentry_test.c    | 48 +++++++++++++++----
>  1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_test.c b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> index 04ebbf1cb390..1f7566e772e9 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> @@ -3,20 +3,24 @@
>  #include <test_progs.h>
>  #include "fentry_test.skel.h"
>
> -void test_fentry_test(void)
> +static __u32 duration;
> +
> +static int fentry_test(struct fentry_test *fentry_skel)
>  {
> -       struct fentry_test *fentry_skel = NULL;
> +       struct bpf_link *link;
>         int err, prog_fd, i;
> -       __u32 duration = 0, retval;
>         __u64 *result;
> -
> -       fentry_skel = fentry_test__open_and_load();
> -       if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
> -               goto cleanup;
> +       __u32 retval;
>
>         err = fentry_test__attach(fentry_skel);
>         if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
> -               goto cleanup;
> +               return err;
> +
> +       /* Check that already linked program can't be attached again. */
> +       link = bpf_program__attach(fentry_skel->progs.test1);
> +       if (CHECK(!IS_ERR(link), "fentry_attach_link",

if (!ASSERT_ERR_PTR(link, "fentry_attach_link")) ?

> +                 "re-attach without detach should not succeed"))
> +               return -1;
>
>         prog_fd = bpf_program__fd(fentry_skel->progs.test1);
>         err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> @@ -26,12 +30,36 @@ void test_fentry_test(void)
>               err, errno, retval, duration);
>
>         result = (__u64 *)fentry_skel->bss;
> -       for (i = 0; i < 6; i++) {
> +       for (i = 0; i < 8; i++) {

how about using sizeof(*fentry_skel->bss) / sizeof(__u64) ?

>                 if (CHECK(result[i] != 1, "result",
>                           "fentry_test%d failed err %lld\n", i + 1, result[i]))
> -                       goto cleanup;
> +                       return -1;
>         }
>
> +       fentry_test__detach(fentry_skel);
> +
> +       /* zero results for re-attach test */
> +       for (i = 0; i < 8; i++)
> +               result[i] = 0;
> +       return 0;
> +}
> +
> +void test_fentry_test(void)
> +{
> +       struct fentry_test *fentry_skel = NULL;
> +       int err;
> +
> +       fentry_skel = fentry_test__open_and_load();
> +       if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
> +               goto cleanup;
> +
> +       err = fentry_test(fentry_skel);
> +       if (CHECK(err, "fentry_test", "first attach failed\n"))
> +               goto cleanup;
> +
> +       err = fentry_test(fentry_skel);
> +       CHECK(err, "fentry_test", "second attach failed\n");

overall: please try to use ASSERT_xxx macros, they are easier to
follow and require less typing
> +
>  cleanup:
>         fentry_test__destroy(fentry_skel);
>  }
> --
> 2.30.2
>

Re: [PATCHv2 bpf-next 3/5] selftests/bpf: Add re-attach test to fexit_test

[Andrii Nakryiko]

On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding the test to re-attach (detach/attach again) tracing
> fexit programs, plus check that already linked program can't
> be attached again.
>
> Fixing the number of check-ed results, which should be 8.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/fexit_test.c     | 48 +++++++++++++++----
>  1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> index 78d7a2765c27..579e620e6612 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> @@ -3,20 +3,24 @@
>  #include <test_progs.h>
>  #include "fexit_test.skel.h"
>
> -void test_fexit_test(void)
> +static __u32 duration;
> +
> +static int fexit_test(struct fexit_test *fexit_skel)
>  {
> -       struct fexit_test *fexit_skel = NULL;
> +       struct bpf_link *link;
>         int err, prog_fd, i;
> -       __u32 duration = 0, retval;
>         __u64 *result;
> -
> -       fexit_skel = fexit_test__open_and_load();
> -       if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
> -               goto cleanup;
> +       __u32 retval;
>
>         err = fexit_test__attach(fexit_skel);
>         if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
> -               goto cleanup;
> +               return err;
> +
> +       /* Check that already linked program can't be attached again. */
> +       link = bpf_program__attach(fexit_skel->progs.test1);
> +       if (CHECK(!IS_ERR(link), "fexit_attach_link",
> +                 "re-attach without detach should not succeed"))
> +               return -1;
>
>         prog_fd = bpf_program__fd(fexit_skel->progs.test1);
>         err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> @@ -26,12 +30,36 @@ void test_fexit_test(void)
>               err, errno, retval, duration);
>
>         result = (__u64 *)fexit_skel->bss;
> -       for (i = 0; i < 6; i++) {
> +       for (i = 0; i < 8; i++) {
>                 if (CHECK(result[i] != 1, "result",
>                           "fexit_test%d failed err %lld\n", i + 1, result[i]))
> -                       goto cleanup;
> +                       return -1;
>         }
>
> +       fexit_test__detach(fexit_skel);
> +
> +       /* zero results for re-attach test */
> +       for (i = 0; i < 8; i++)
> +               result[i] = 0;

memset(fexit_skel->bss, 0, sizeof(*fexit_skel->bss)) ? ;)

and see my nits in previous patch about ASSERT over CHECK


> +       return 0;
> +}
> +
> +void test_fexit_test(void)
> +{
> +       struct fexit_test *fexit_skel = NULL;
> +       int err;
> +
> +       fexit_skel = fexit_test__open_and_load();
> +       if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
> +               goto cleanup;
> +
> +       err = fexit_test(fexit_skel);
> +       if (CHECK(err, "fexit_test", "first attach failed\n"))
> +               goto cleanup;
> +
> +       err = fexit_test(fexit_skel);
> +       CHECK(err, "fexit_test", "second attach failed\n");
> +
>  cleanup:
>         fexit_test__destroy(fexit_skel);
>  }
> --
> 2.30.2
>

Re: [PATCHv2 bpf-next 5/5] selftests/bpf: Test that module can't be unloaded with attached trampoline

[Andrii Nakryiko]

On Wed, Apr 7, 2021 at 4:22 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test to verify that once we attach module's trampoline,
> the module can't be unloaded.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

To be fair, to test that you are actually testing what you think you
are testing, you'd have to prove that you *can* detach when no program
is attached to bpf_testmod ;) You'd also need kern_sync_rcu() to wait
for all the async clean up to complete inside the kernel. But that
doesn't interact with other tests well, so I think it's fine.

grumpily due to CHECK() usage (please do consider updating to ASSERT):

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  .../selftests/bpf/prog_tests/module_attach.c  | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> index 5bc53d53d86e..d180b8c28287 100644
> --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> @@ -45,12 +45,18 @@ static int trigger_module_test_write(int write_sz)
>         return 0;
>  }
>
> +static int delete_module(const char *name, int flags)
> +{
> +       return syscall(__NR_delete_module, name, flags);
> +}
> +
>  void test_module_attach(void)
>  {
>         const int READ_SZ = 456;
>         const int WRITE_SZ = 457;
>         struct test_module_attach* skel;
>         struct test_module_attach__bss *bss;
> +       struct bpf_link *link;
>         int err;
>
>         skel = test_module_attach__open();
> @@ -84,6 +90,23 @@ void test_module_attach(void)
>         ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet");
>         ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret");
>
> +       test_module_attach__detach(skel);
> +
> +       /* attach fentry/fexit and make sure it get's module reference */
> +       link = bpf_program__attach(skel->progs.handle_fentry);
> +       if (CHECK(IS_ERR(link), "attach_fentry", "err: %ld\n", PTR_ERR(link)))
> +               goto cleanup;
> +
> +       ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +       bpf_link__destroy(link);
> +
> +       link = bpf_program__attach(skel->progs.handle_fexit);
> +       if (CHECK(IS_ERR(link), "attach_fexit", "err: %ld\n", PTR_ERR(link)))
> +               goto cleanup;
> +
> +       ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +       bpf_link__destroy(link);
> +
>  cleanup:
>         test_module_attach__destroy(skel);
>  }
> --
> 2.30.2
>

Re: [PATCHv2 bpf-next 3/5] selftests/bpf: Add re-attach test to fexit_test

[Jiri Olsa]

On Wed, Apr 07, 2021 at 03:51:46PM -0700, Andrii Nakryiko wrote:
> On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding the test to re-attach (detach/attach again) tracing
> > fexit programs, plus check that already linked program can't
> > be attached again.
> >
> > Fixing the number of check-ed results, which should be 8.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/fexit_test.c     | 48 +++++++++++++++----
> >  1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> > index 78d7a2765c27..579e620e6612 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
> > @@ -3,20 +3,24 @@
> >  #include <test_progs.h>
> >  #include "fexit_test.skel.h"
> >
> > -void test_fexit_test(void)
> > +static __u32 duration;
> > +
> > +static int fexit_test(struct fexit_test *fexit_skel)
> >  {
> > -       struct fexit_test *fexit_skel = NULL;
> > +       struct bpf_link *link;
> >         int err, prog_fd, i;
> > -       __u32 duration = 0, retval;
> >         __u64 *result;
> > -
> > -       fexit_skel = fexit_test__open_and_load();
> > -       if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
> > -               goto cleanup;
> > +       __u32 retval;
> >
> >         err = fexit_test__attach(fexit_skel);
> >         if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
> > -               goto cleanup;
> > +               return err;
> > +
> > +       /* Check that already linked program can't be attached again. */
> > +       link = bpf_program__attach(fexit_skel->progs.test1);
> > +       if (CHECK(!IS_ERR(link), "fexit_attach_link",
> > +                 "re-attach without detach should not succeed"))
> > +               return -1;
> >
> >         prog_fd = bpf_program__fd(fexit_skel->progs.test1);
> >         err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> > @@ -26,12 +30,36 @@ void test_fexit_test(void)
> >               err, errno, retval, duration);
> >
> >         result = (__u64 *)fexit_skel->bss;
> > -       for (i = 0; i < 6; i++) {
> > +       for (i = 0; i < 8; i++) {
> >                 if (CHECK(result[i] != 1, "result",
> >                           "fexit_test%d failed err %lld\n", i + 1, result[i]))
> > -                       goto cleanup;
> > +                       return -1;
> >         }
> >
> > +       fexit_test__detach(fexit_skel);
> > +
> > +       /* zero results for re-attach test */
> > +       for (i = 0; i < 8; i++)
> > +               result[i] = 0;
> 
> memset(fexit_skel->bss, 0, sizeof(*fexit_skel->bss)) ? ;)
> 
> and see my nits in previous patch about ASSERT over CHECK

sure ;-) thanks

jirka

Re: [PATCHv2 bpf-next 2/5] selftests/bpf: Add re-attach test to fentry_test

[Jiri Olsa]

On Wed, Apr 07, 2021 at 03:47:30PM -0700, Andrii Nakryiko wrote:
> On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding the test to re-attach (detach/attach again) tracing
> > fentry programs, plus check that already linked program can't
> > be attached again.
> >
> > Fixing the number of check-ed results, which should be 8.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/fentry_test.c    | 48 +++++++++++++++----
> >  1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_test.c b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> > index 04ebbf1cb390..1f7566e772e9 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> > @@ -3,20 +3,24 @@
> >  #include <test_progs.h>
> >  #include "fentry_test.skel.h"
> >
> > -void test_fentry_test(void)
> > +static __u32 duration;
> > +
> > +static int fentry_test(struct fentry_test *fentry_skel)
> >  {
> > -       struct fentry_test *fentry_skel = NULL;
> > +       struct bpf_link *link;
> >         int err, prog_fd, i;
> > -       __u32 duration = 0, retval;
> >         __u64 *result;
> > -
> > -       fentry_skel = fentry_test__open_and_load();
> > -       if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
> > -               goto cleanup;
> > +       __u32 retval;
> >
> >         err = fentry_test__attach(fentry_skel);
> >         if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
> > -               goto cleanup;
> > +               return err;
> > +
> > +       /* Check that already linked program can't be attached again. */
> > +       link = bpf_program__attach(fentry_skel->progs.test1);
> > +       if (CHECK(!IS_ERR(link), "fentry_attach_link",
> 
> if (!ASSERT_ERR_PTR(link, "fentry_attach_link")) ?

ok

> 
> > +                 "re-attach without detach should not succeed"))
> > +               return -1;
> >
> >         prog_fd = bpf_program__fd(fentry_skel->progs.test1);
> >         err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> > @@ -26,12 +30,36 @@ void test_fentry_test(void)
> >               err, errno, retval, duration);
> >
> >         result = (__u64 *)fentry_skel->bss;
> > -       for (i = 0; i < 6; i++) {
> > +       for (i = 0; i < 8; i++) {
> 
> how about using sizeof(*fentry_skel->bss) / sizeof(__u64) ?

ok

> 
> >                 if (CHECK(result[i] != 1, "result",
> >                           "fentry_test%d failed err %lld\n", i + 1, result[i]))
> > -                       goto cleanup;
> > +                       return -1;
> >         }
> >
> > +       fentry_test__detach(fentry_skel);
> > +
> > +       /* zero results for re-attach test */
> > +       for (i = 0; i < 8; i++)
> > +               result[i] = 0;
> > +       return 0;
> > +}
> > +
> > +void test_fentry_test(void)
> > +{
> > +       struct fentry_test *fentry_skel = NULL;
> > +       int err;
> > +
> > +       fentry_skel = fentry_test__open_and_load();
> > +       if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
> > +               goto cleanup;
> > +
> > +       err = fentry_test(fentry_skel);
> > +       if (CHECK(err, "fentry_test", "first attach failed\n"))
> > +               goto cleanup;
> > +
> > +       err = fentry_test(fentry_skel);
> > +       CHECK(err, "fentry_test", "second attach failed\n");
> 
> overall: please try to use ASSERT_xxx macros, they are easier to
> follow and require less typing

ok, will check

thanks,
jirka

Re: [PATCHv2 bpf-next 5/5] selftests/bpf: Test that module can't be unloaded with attached trampoline

[Jiri Olsa]

On Wed, Apr 07, 2021 at 04:04:48PM -0700, Andrii Nakryiko wrote:
> On Wed, Apr 7, 2021 at 4:22 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test to verify that once we attach module's trampoline,
> > the module can't be unloaded.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> To be fair, to test that you are actually testing what you think you
> are testing, you'd have to prove that you *can* detach when no program
> is attached to bpf_testmod ;) You'd also need kern_sync_rcu() to wait
> for all the async clean up to complete inside the kernel. But that
> doesn't interact with other tests well, so I think it's fine.

well without the kernel change the module gets unloaded
and the test below fails.. we could add module unload
test, but as you described it could probably interfere
with other tests

> 
> grumpily due to CHECK() usage (please do consider updating to ASSERT):

ok, will check

thanks,
jirka

> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  .../selftests/bpf/prog_tests/module_attach.c  | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> > index 5bc53d53d86e..d180b8c28287 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> > @@ -45,12 +45,18 @@ static int trigger_module_test_write(int write_sz)
> >         return 0;
> >  }
> >
> > +static int delete_module(const char *name, int flags)
> > +{
> > +       return syscall(__NR_delete_module, name, flags);
> > +}
> > +
> >  void test_module_attach(void)
> >  {
> >         const int READ_SZ = 456;
> >         const int WRITE_SZ = 457;
> >         struct test_module_attach* skel;
> >         struct test_module_attach__bss *bss;
> > +       struct bpf_link *link;
> >         int err;
> >
> >         skel = test_module_attach__open();
> > @@ -84,6 +90,23 @@ void test_module_attach(void)
> >         ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet");
> >         ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret");
> >
> > +       test_module_attach__detach(skel);
> > +
> > +       /* attach fentry/fexit and make sure it get's module reference */
> > +       link = bpf_program__attach(skel->progs.handle_fentry);
> > +       if (CHECK(IS_ERR(link), "attach_fentry", "err: %ld\n", PTR_ERR(link)))
> > +               goto cleanup;
> > +
> > +       ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> > +       bpf_link__destroy(link);
> > +
> > +       link = bpf_program__attach(skel->progs.handle_fexit);
> > +       if (CHECK(IS_ERR(link), "attach_fexit", "err: %ld\n", PTR_ERR(link)))
> > +               goto cleanup;
> > +
> > +       ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> > +       bpf_link__destroy(link);
> > +
> >  cleanup:
> >         test_module_attach__destroy(skel);
> >  }
> > --
> > 2.30.2
> >
>