BPF Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach
@ 2021-03-28 11:26 Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-28 11:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen

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.

I'm not sure this was on purpose, but seems as natural
use of the interface, although the only user is the test
for now.

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

thanks,
jirka


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

 kernel/bpf/syscall.c                                   | 25 +++++++++++++++++++------
 kernel/bpf/trampoline.c                                |  2 +-
 tools/testing/selftests/bpf/prog_tests/fentry_test.c   | 58 +++++++++++++++++++++++++++++++++++++++++++++-------------
 tools/testing/selftests/bpf/prog_tests/fexit_test.c    | 58 +++++++++++++++++++++++++++++++++++++++++++++-------------
 tools/testing/selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++++++
 5 files changed, 133 insertions(+), 33 deletions(-)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-03-28 11:26 [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach Jiri Olsa
@ 2021-03-28 11:26 ` Jiri Olsa
  2021-03-30  1:18   ` Song Liu
  2021-04-03 11:24   ` Toke Høiland-Jørgensen
  2021-03-28 11:26 ` [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test Jiri Olsa
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-28 11:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen

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
kernel program.

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

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 9603de81811a..e14926b2e95a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2645,14 +2645,27 @@ 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
+	 *
+	 * The combination of no saved target in prog->aux, and no target
+	 * specified on is legal only for tracing programs re-attach, rest
+	 * is illegal, and we reject that here.
 	 */
 	if (!prog->aux->dst_trampoline && !tgt_prog) {
-		err = -ENOENT;
-		goto out_unlock;
+		/*
+		 * Allow re-attach for tracing programs, if it's currently
+		 * linked, bpf_trampoline_link_prog will fail.
+		 */
+		if (prog->type != BPF_PROG_TYPE_TRACING) {
+			err = -ENOENT;
+			goto out_unlock;
+		}
+		if (!prog->aux->attach_btf) {
+			err = -EINVAL;
+			goto out_unlock;
+		}
+		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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test
  2021-03-28 11:26 [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
@ 2021-03-28 11:26 ` Jiri Olsa
  2021-03-30  1:23   ` Song Liu
  2021-03-28 11:26 ` [RFC PATCH bpf-next 3/4] selftests/bpf: Add re-attach test to fexit_test Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline Jiri Olsa
  3 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2021-03-28 11:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen

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    | 58 ++++++++++++++-----
 1 file changed, 45 insertions(+), 13 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..fa7a9c719659 100644
--- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
@@ -3,20 +3,13 @@
 #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;
 	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;
-
-	err = fentry_test__attach(fentry_skel);
-	if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
-		goto cleanup;
+	__u32 retval;
 
 	prog_fd = bpf_program__fd(fentry_skel->progs.test1);
 	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
@@ -26,12 +19,51 @@ 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;
 	}
 
+	/* 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;
+	struct bpf_link *link;
+	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__attach(fentry_skel);
+	if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
+		goto cleanup;
+
+	err = fentry_test(fentry_skel);
+	if (CHECK(err, "fentry_test", "fentry test failed: %d\n", err))
+		goto cleanup;
+
+	fentry_test__detach(fentry_skel);
+
+	/* Re-attach and test again */
+	err = fentry_test__attach(fentry_skel);
+	if (CHECK(err, "fentry_attach", "fentry re-attach failed: %d\n", err))
+		goto cleanup;
+
+	link = bpf_program__attach(fentry_skel->progs.test1);
+	if (CHECK(!IS_ERR(link), "attach_fentry re-attach without detach",
+		  "err: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+
+	err = fentry_test(fentry_skel);
+	CHECK(err, "fentry_test", "fentry test failed: %d\n", err);
+
 cleanup:
 	fentry_test__destroy(fentry_skel);
 }
-- 
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH bpf-next 3/4] selftests/bpf: Add re-attach test to fexit_test
  2021-03-28 11:26 [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test Jiri Olsa
@ 2021-03-28 11:26 ` Jiri Olsa
  2021-03-28 11:26 ` [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline Jiri Olsa
  3 siblings, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-28 11:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen

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     | 58 ++++++++++++++-----
 1 file changed, 45 insertions(+), 13 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..6666c5105f1e 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
@@ -3,20 +3,13 @@
 #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;
 	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;
-
-	err = fexit_test__attach(fexit_skel);
-	if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
-		goto cleanup;
+	__u32 retval;
 
 	prog_fd = bpf_program__fd(fexit_skel->progs.test1);
 	err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
@@ -26,12 +19,51 @@ 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;
 	}
 
+	/* 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;
+	struct bpf_link *link;
+	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__attach(fexit_skel);
+	if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
+		goto cleanup;
+
+	err = fexit_test(fexit_skel);
+	if (CHECK(err, "fexit_test", "exit test failed: %d\n", err))
+		goto cleanup;
+
+	fexit_test__detach(fexit_skel);
+
+	/* Re-attach and test again */
+	err = fexit_test__attach(fexit_skel);
+	if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
+		goto cleanup;
+
+	link = bpf_program__attach(fexit_skel->progs.test1);
+	if (CHECK(!IS_ERR(link), "attach_fexit re-attach without detach",
+		"err: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+
+	err = fexit_test(fexit_skel);
+	CHECK(err, "fexit_test", "fexit test failed: %d\n", err);
+
 cleanup:
 	fexit_test__destroy(fexit_skel);
 }
-- 
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline
  2021-03-28 11:26 [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach Jiri Olsa
                   ` (2 preceding siblings ...)
  2021-03-28 11:26 ` [RFC PATCH bpf-next 3/4] selftests/bpf: Add re-attach test to fexit_test Jiri Olsa
@ 2021-03-28 11:26 ` Jiri Olsa
  2021-03-30  6:12   ` Song Liu
  3 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2021-03-28 11:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Toke Høiland-Jørgensen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
@ 2021-03-30  1:18   ` Song Liu
  2021-04-03 11:24   ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 15+ messages in thread
From: Song Liu @ 2021-03-30  1:18 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
	bpf, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen



> On Mar 28, 2021, at 4:26 AM, Jiri Olsa <jolsa@kernel.org> wrote:
> 
> 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
> kernel program.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

LGTM. 

Acked-by: Song Liu <songliubraving@fb.com>

[...]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test
  2021-03-28 11:26 ` [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test Jiri Olsa
@ 2021-03-30  1:23   ` Song Liu
  2021-03-30 20:02     ` Jiri Olsa
  0 siblings, 1 reply; 15+ messages in thread
From: Song Liu @ 2021-03-30  1:23 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
	bpf, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen



> On Mar 28, 2021, at 4:26 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>
> 
[...]
> +
> +void test_fentry_test(void)
> +{
> +	struct fentry_test *fentry_skel = NULL;
> +	struct bpf_link *link;
> +	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__attach(fentry_skel);
> +	if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
> +		goto cleanup;
> +
> +	err = fentry_test(fentry_skel);
> +	if (CHECK(err, "fentry_test", "fentry test failed: %d\n", err))
> +		goto cleanup;
> +
> +	fentry_test__detach(fentry_skel);
> +
> +	/* Re-attach and test again */
> +	err = fentry_test__attach(fentry_skel);
> +	if (CHECK(err, "fentry_attach", "fentry re-attach failed: %d\n", err))
> +		goto cleanup;
> +
> +	link = bpf_program__attach(fentry_skel->progs.test1);
> +	if (CHECK(!IS_ERR(link), "attach_fentry re-attach without detach",
> +		  "err: %ld\n", PTR_ERR(link)))

nit: I guess we shouldn't print PTR_ERR(link) when link is not an error code?
This shouldn't break though. 

Thanks,
Song

> +		goto cleanup;
> +
> +	err = fentry_test(fentry_skel);
> +	CHECK(err, "fentry_test", "fentry test failed: %d\n", err);
> +
> cleanup:
> 	fentry_test__destroy(fentry_skel);
> }
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline
  2021-03-28 11:26 ` [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline Jiri Olsa
@ 2021-03-30  6:12   ` Song Liu
  0 siblings, 0 replies; 15+ messages in thread
From: Song Liu @ 2021-03-30  6:12 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev,
	bpf, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen



> On Mar 28, 2021, at 4:26 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>

Acked-by: Song Liu <songliubraving@fb.com>

[...]


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test
  2021-03-30  1:23   ` Song Liu
@ 2021-03-30 20:02     ` Jiri Olsa
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-03-30 20:02 UTC (permalink / raw)
  To: Song Liu
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	netdev, bpf, Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Toke Høiland-Jørgensen

On Tue, Mar 30, 2021 at 01:23:15AM +0000, Song Liu wrote:
> 
> 
> > On Mar 28, 2021, at 4:26 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>
> > 
> [...]
> > +
> > +void test_fentry_test(void)
> > +{
> > +	struct fentry_test *fentry_skel = NULL;
> > +	struct bpf_link *link;
> > +	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__attach(fentry_skel);
> > +	if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
> > +		goto cleanup;
> > +
> > +	err = fentry_test(fentry_skel);
> > +	if (CHECK(err, "fentry_test", "fentry test failed: %d\n", err))
> > +		goto cleanup;
> > +
> > +	fentry_test__detach(fentry_skel);
> > +
> > +	/* Re-attach and test again */
> > +	err = fentry_test__attach(fentry_skel);
> > +	if (CHECK(err, "fentry_attach", "fentry re-attach failed: %d\n", err))
> > +		goto cleanup;
> > +
> > +	link = bpf_program__attach(fentry_skel->progs.test1);
> > +	if (CHECK(!IS_ERR(link), "attach_fentry re-attach without detach",
> > +		  "err: %ld\n", PTR_ERR(link)))
> 
> nit: I guess we shouldn't print PTR_ERR(link) when link is not an error code?
> This shouldn't break though. 

true, makes no sense.. I'll remove it

thanks,
jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
  2021-03-30  1:18   ` Song Liu
@ 2021-04-03 11:24   ` Toke Høiland-Jørgensen
  2021-04-03 18:21     ` Alexei Starovoitov
  2021-04-05 14:06     ` Jiri Olsa
  1 sibling, 2 replies; 15+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-04-03 11:24 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh

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
> kernel program.

Hmm, yeah, didn't really consider this case when I added the original
disallow. But don't see why not, so (with one nit below):

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

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/syscall.c    | 25 +++++++++++++++++++------
>  kernel/bpf/trampoline.c |  2 +-
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 9603de81811a..e14926b2e95a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2645,14 +2645,27 @@ 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
> +	 *
> +	 * The combination of no saved target in prog->aux, and no target
> +	 * specified on is legal only for tracing programs re-attach, rest
> +	 * is illegal, and we reject that here.
>  	 */
>  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> -		err = -ENOENT;
> -		goto out_unlock;
> +		/*
> +		 * Allow re-attach for tracing programs, if it's currently
> +		 * linked, bpf_trampoline_link_prog will fail.
> +		 */
> +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> +			err = -ENOENT;
> +			goto out_unlock;
> +		}
> +		if (!prog->aux->attach_btf) {
> +			err = -EINVAL;
> +			goto out_unlock;
> +		}

I'm wondering about the two different return codes here. Under what
circumstances will aux->attach_btf be NULL, and why is that not an
ENOENT error? :)

-Toke


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-04-03 11:24   ` Toke Høiland-Jørgensen
@ 2021-04-03 18:21     ` Alexei Starovoitov
  2021-04-05 14:08       ` Jiri Olsa
  2021-04-05 14:06     ` Jiri Olsa
  1 sibling, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2021-04-03 18:21 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh

On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote:
> >  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> > -		err = -ENOENT;
> > -		goto out_unlock;
> > +		/*
> > +		 * Allow re-attach for tracing programs, if it's currently
> > +		 * linked, bpf_trampoline_link_prog will fail.
> > +		 */
> > +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> > +			err = -ENOENT;
> > +			goto out_unlock;
> > +		}
> > +		if (!prog->aux->attach_btf) {
> > +			err = -EINVAL;
> > +			goto out_unlock;
> > +		}
> 
> I'm wondering about the two different return codes here. Under what
> circumstances will aux->attach_btf be NULL, and why is that not an
> ENOENT error? :)

The feature makes sense to me as well.
I don't quite see how it would get here with attach_btf == NULL.
Maybe WARN_ON then?
Also if we're allowing re-attach this way why exclude PROG_EXT and LSM?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-04-03 11:24   ` Toke Høiland-Jørgensen
  2021-04-03 18:21     ` Alexei Starovoitov
@ 2021-04-05 14:06     ` Jiri Olsa
  1 sibling, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-04-05 14:06 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh

On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote:
> 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
> > kernel program.
> 
> Hmm, yeah, didn't really consider this case when I added the original
> disallow. But don't see why not, so (with one nit below):
> 
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/syscall.c    | 25 +++++++++++++++++++------
> >  kernel/bpf/trampoline.c |  2 +-
> >  2 files changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 9603de81811a..e14926b2e95a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2645,14 +2645,27 @@ 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
> > +	 *
> > +	 * The combination of no saved target in prog->aux, and no target
> > +	 * specified on is legal only for tracing programs re-attach, rest
> > +	 * is illegal, and we reject that here.
> >  	 */
> >  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> > -		err = -ENOENT;
> > -		goto out_unlock;
> > +		/*
> > +		 * Allow re-attach for tracing programs, if it's currently
> > +		 * linked, bpf_trampoline_link_prog will fail.
> > +		 */
> > +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> > +			err = -ENOENT;
> > +			goto out_unlock;
> > +		}
> > +		if (!prog->aux->attach_btf) {
> > +			err = -EINVAL;
> > +			goto out_unlock;
> > +		}
> 
> I'm wondering about the two different return codes here. Under what
> circumstances will aux->attach_btf be NULL, and why is that not an
> ENOENT error? :)

right, that should be always there.. I'll remove it

thanks,
jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-04-03 18:21     ` Alexei Starovoitov
@ 2021-04-05 14:08       ` Jiri Olsa
  2021-04-05 14:15         ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2021-04-05 14:08 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Toke Høiland-Jørgensen, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, netdev, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh

On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote:
> On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote:
> > >  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> > > -		err = -ENOENT;
> > > -		goto out_unlock;
> > > +		/*
> > > +		 * Allow re-attach for tracing programs, if it's currently
> > > +		 * linked, bpf_trampoline_link_prog will fail.
> > > +		 */
> > > +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> > > +			err = -ENOENT;
> > > +			goto out_unlock;
> > > +		}
> > > +		if (!prog->aux->attach_btf) {
> > > +			err = -EINVAL;
> > > +			goto out_unlock;
> > > +		}
> > 
> > I'm wondering about the two different return codes here. Under what
> > circumstances will aux->attach_btf be NULL, and why is that not an
> > ENOENT error? :)
> 
> The feature makes sense to me as well.
> I don't quite see how it would get here with attach_btf == NULL.
> Maybe WARN_ON then?

right, that should be always there

> Also if we're allowing re-attach this way why exclude PROG_EXT and LSM?
> 

I was enabling just what I needed for the test, which is so far
the only use case.. I'll see if I can enable that for all of them

jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-04-05 14:08       ` Jiri Olsa
@ 2021-04-05 14:15         ` Toke Høiland-Jørgensen
  2021-04-05 21:58           ` Jiri Olsa
  0 siblings, 1 reply; 15+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-04-05 14:15 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh

Jiri Olsa <jolsa@redhat.com> writes:

> On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote:
>> On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote:
>> > >  	if (!prog->aux->dst_trampoline && !tgt_prog) {
>> > > -		err = -ENOENT;
>> > > -		goto out_unlock;
>> > > +		/*
>> > > +		 * Allow re-attach for tracing programs, if it's currently
>> > > +		 * linked, bpf_trampoline_link_prog will fail.
>> > > +		 */
>> > > +		if (prog->type != BPF_PROG_TYPE_TRACING) {
>> > > +			err = -ENOENT;
>> > > +			goto out_unlock;
>> > > +		}
>> > > +		if (!prog->aux->attach_btf) {
>> > > +			err = -EINVAL;
>> > > +			goto out_unlock;
>> > > +		}
>> > 
>> > I'm wondering about the two different return codes here. Under what
>> > circumstances will aux->attach_btf be NULL, and why is that not an
>> > ENOENT error? :)
>> 
>> The feature makes sense to me as well.
>> I don't quite see how it would get here with attach_btf == NULL.
>> Maybe WARN_ON then?
>
> right, that should be always there
>
>> Also if we're allowing re-attach this way why exclude PROG_EXT and LSM?
>> 
>
> I was enabling just what I needed for the test, which is so far
> the only use case.. I'll see if I can enable that for all of them

How would that work? For PROG_EXT we clear the destination on the first
attach (to avoid keeping a ref on it), so re-attach can only be done
with an explicit target (which already works just fine)...

-Toke


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach
  2021-04-05 14:15         ` Toke Høiland-Jørgensen
@ 2021-04-05 21:58           ` Jiri Olsa
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Olsa @ 2021-04-05 21:58 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Alexei Starovoitov, Jiri Olsa, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, netdev, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh

On Mon, Apr 05, 2021 at 04:15:54PM +0200, Toke Høiland-Jørgensen wrote:
> Jiri Olsa <jolsa@redhat.com> writes:
> 
> > On Sat, Apr 03, 2021 at 11:21:55AM -0700, Alexei Starovoitov wrote:
> >> On Sat, Apr 03, 2021 at 01:24:12PM +0200, Toke Høiland-Jørgensen wrote:
> >> > >  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> >> > > -		err = -ENOENT;
> >> > > -		goto out_unlock;
> >> > > +		/*
> >> > > +		 * Allow re-attach for tracing programs, if it's currently
> >> > > +		 * linked, bpf_trampoline_link_prog will fail.
> >> > > +		 */
> >> > > +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> >> > > +			err = -ENOENT;
> >> > > +			goto out_unlock;
> >> > > +		}
> >> > > +		if (!prog->aux->attach_btf) {
> >> > > +			err = -EINVAL;
> >> > > +			goto out_unlock;
> >> > > +		}
> >> > 
> >> > I'm wondering about the two different return codes here. Under what
> >> > circumstances will aux->attach_btf be NULL, and why is that not an
> >> > ENOENT error? :)
> >> 
> >> The feature makes sense to me as well.
> >> I don't quite see how it would get here with attach_btf == NULL.
> >> Maybe WARN_ON then?
> >
> > right, that should be always there
> >
> >> Also if we're allowing re-attach this way why exclude PROG_EXT and LSM?
> >> 
> >
> > I was enabling just what I needed for the test, which is so far
> > the only use case.. I'll see if I can enable that for all of them
> 
> How would that work? For PROG_EXT we clear the destination on the first
> attach (to avoid keeping a ref on it), so re-attach can only be done
> with an explicit target (which already works just fine)...

right, I'm just looking on it ;-) extensions already seem allow for that,
I'll check LSM

jirka


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-28 11:26 [RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach Jiri Olsa
2021-03-28 11:26 ` [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach Jiri Olsa
2021-03-30  1:18   ` Song Liu
2021-04-03 11:24   ` Toke Høiland-Jørgensen
2021-04-03 18:21     ` Alexei Starovoitov
2021-04-05 14:08       ` Jiri Olsa
2021-04-05 14:15         ` Toke Høiland-Jørgensen
2021-04-05 21:58           ` Jiri Olsa
2021-04-05 14:06     ` Jiri Olsa
2021-03-28 11:26 ` [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test Jiri Olsa
2021-03-30  1:23   ` Song Liu
2021-03-30 20:02     ` Jiri Olsa
2021-03-28 11:26 ` [RFC PATCH bpf-next 3/4] selftests/bpf: Add re-attach test to fexit_test Jiri Olsa
2021-03-28 11:26 ` [RFC PATCH bpf-next 4/4] selftests/bpf: Test that module can't be unloaded with attached trampoline Jiri Olsa
2021-03-30  6:12   ` Song Liu

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git