[RFC PATCH bpf-next 0/4] bpf: Tracing programs re-attach

[RFC PATCH bpf-next 0/4] bpf: Tracing 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.

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(-)

[RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[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
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

[RFC PATCH bpf-next 2/4] 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    | 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

[RFC PATCH bpf-next 3/4] 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     | 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

[RFC PATCH bpf-next 4/4] 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: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Song Liu]

> 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>

[...]

Re: [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test

[Song Liu]

> 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
> 

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

[Song Liu]

> 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>

[...]

Re: [RFC PATCH bpf-next 2/4] selftests/bpf: Add re-attach test to fentry_test

[Jiri Olsa]

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

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[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
> 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

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Alexei Starovoitov]

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?

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Jiri Olsa]

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

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Jiri Olsa]

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

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Toke Høiland-Jørgensen]

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

Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

[Jiri Olsa]

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