Skip to contentSkip to conversation

bpf: Fix WARNING in bpf_tracing_link_release

PATCH Superseded Leon Hwang 2 participants 7 messages Jul 21 – Jul 22

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Leon Hwang

author
#0 lore raw

cover bpf: Fix WARNING in bpf_tracing_link_release

The trampoline could be corrupted by the blindly 'tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX' in verifier.

1. A fexit attached to a tail_call_reachable prog.
2. Another fexit loaded with the same tail_call_reachable prog target.
3. Close the first fexit link.

[    3.410719] WARNING: kernel/bpf/syscall.c:3551 at bpf_tracing_link_release+0x53/0x60, CPU#1: test_progs/98
...
[    3.428793]  bpf_link_free+0x58/0x130
[    3.429293]  bpf_link_release+0x23/0x30

Fix the warning by updating 'tr->flags' with '|=' and lock.

Leon Hwang (2):
bpf: Fix WARNING in bpf_tracing_link_release selftests/bpf: Verify no warning when close fexit link

 include/linux/bpf.h                           |  2 +
 kernel/bpf/trampoline.c                       |  7 +++
 kernel/bpf/verifier.c                         |  2 +-
 .../selftests/bpf/prog_tests/tailcalls.c      | 51 +++++++++++++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)

--
2.55.0

author
#1 lore raw

PATCH 1/2 bpf: Fix WARNING in bpf_tracing_link_release

The trampoline could be corrupted by the blindly 'tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX' in verifier.

1. A fexit attached to a tail_call_reachable prog.
2. Another fexit loaded with the same tail_call_reachable prog target.
3. Close the first fexit link.

[    3.410719] WARNING: kernel/bpf/syscall.c:3551 at bpf_tracing_link_release+0x53/0x60, CPU#1: test_progs/98
...
[    3.428793]  bpf_link_free+0x58/0x130
[    3.429293]  bpf_link_release+0x23/0x30

Fix the warning by updating 'tr->flags' with '|=' and lock.

Fixes 2b5dcb31a19a ("bpf, x64: Fix tailcall infinite loop")
---
 include/linux/bpf.h     | 2 ++
 kernel/bpf/trampoline.c | 7 +++++++
 kernel/bpf/verifier.c   | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)
include/linux/bpf.h +2 -0
index d9542127dfdf..fc84f39967ae 100644
@@ -1523,6 +1523,7 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
 				struct bpf_tracing_multi_link *link);
 int bpf_trampoline_multi_detach(struct bpf_prog *prog,
 				struct bpf_tracing_multi_link *link);
+void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags);
 
 /*
  * When the architecture supports STATIC_CALL replace the bpf_dispatcher_fn
@@ -1646,6 +1647,7 @@ static inline int bpf_trampoline_multi_detach(struct bpf_prog *prog,
 {
 	return -ENOTSUPP;
 }
+static inline void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags) {}
 #endif
 
 struct bpf_func_info_aux {
kernel/bpf/trampoline.c +7 -0
index 6eadf64f7ec9..129d07db117e 100644
@@ -670,6 +670,13 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, int size)
 	return ERR_PTR(err);
 }
 
+void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags)
+{
+	trampoline_lock(tr);
+	tr->flags |= flags;
+	trampoline_unlock(tr);
+}
+
 static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex,
 				 const struct bpf_trampoline_ops *ops, void *data)
 {
kernel/bpf/verifier.c +1 -1
index 52be0a118cce..66d8d9eaec05 100644
@@ -19523,7 +19523,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return -ENOMEM;
 
 	if (tgt_prog && tgt_prog->aux->tail_call_reachable)
-		tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
+		bpf_trampoline_set_flags(tr, BPF_TRAMP_F_TAIL_CALL_CTX);
 
 	prog->aux->dst_trampoline = tr;
 	return 0;
Signature · 1 line
2.55.0
Acked-by on v1 1/2
#2 lore raw
Quoted 6 lines · Leon Hwang

The trampoline could be corrupted by the blindly 'tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX' in verifier.

1. A fexit attached to a tail_call_reachable prog.
2. Another fexit loaded with the same tail_call_reachable prog target.
3. Close the first fexit link.

had to ask ai for more details on how the warning was trigered ;-) would be nice to have that info in the changelog

jirka

Quoted 70 lines
[    3.410719] WARNING: kernel/bpf/syscall.c:3551 at bpf_tracing_link_release+0x53/0x60, CPU#1: test_progs/98
...
[    3.428793]  bpf_link_free+0x58/0x130
[    3.429293]  bpf_link_release+0x23/0x30

Fix the warning by updating 'tr->flags' with '|=' and lock.

Fixes 2b5dcb31a19a ("bpf, x64: Fix tailcall infinite loop")
---
 include/linux/bpf.h     | 2 ++
 kernel/bpf/trampoline.c | 7 +++++++
 kernel/bpf/verifier.c   | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)
include/linux/bpf.h +2 -0
index d9542127dfdf..fc84f39967ae 100644
@@ -1523,6 +1523,7 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
 				struct bpf_tracing_multi_link *link);
 int bpf_trampoline_multi_detach(struct bpf_prog *prog,
 				struct bpf_tracing_multi_link *link);
+void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags);
 
 /*
  * When the architecture supports STATIC_CALL replace the bpf_dispatcher_fn
@@ -1646,6 +1647,7 @@ static inline int bpf_trampoline_multi_detach(struct bpf_prog *prog,
 {
 	return -ENOTSUPP;
 }
+static inline void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags) {}
 #endif
 
 struct bpf_func_info_aux {
kernel/bpf/trampoline.c +7 -0
index 6eadf64f7ec9..129d07db117e 100644
@@ -670,6 +670,13 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, int size)
 	return ERR_PTR(err);
 }
 
+void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags)
+{
+	trampoline_lock(tr);
+	tr->flags |= flags;
+	trampoline_unlock(tr);
+}
+
 static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex,
 				 const struct bpf_trampoline_ops *ops, void *data)
 {
kernel/bpf/verifier.c +1 -1
index 52be0a118cce..66d8d9eaec05 100644
@@ -19523,7 +19523,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return -ENOMEM;
 
 	if (tgt_prog && tgt_prog->aux->tail_call_reachable)
-		tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
+		bpf_trampoline_set_flags(tr, BPF_TRAMP_F_TAIL_CALL_CTX);
 
 	prog->aux->dst_trampoline = tr;
 	return 0;
Signature · 1 line
2.55.0
author
#3 lore raw
Quoted 10 lines · Jiri Olsa
Quoted 6 lines · Leon Hwang

The trampoline could be corrupted by the blindly 'tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX' in verifier.

1. A fexit attached to a tail_call_reachable prog.
2. Another fexit loaded with the same tail_call_reachable prog target.
3. Close the first fexit link.

had to ask ai for more details on how the warning was trigered ;-) would be nice to have that info in the changelog

Ack.

Will add the change of tr->flags. So, the detach failure would be obvious due to the incorrect tr->flags.

Quoted 2 lines

Thanks for your review.

Leon

Quoted 1 line

[...]

author
#4 lore raw

PATCH 2/2 selftests/bpf: Verify no warning when close fexit link

In reply to Leon Hwang

Add a test to verify that there's no WARNING when detaching fexit link by following the repro steps of previous commit.

Without the fix, the WARNING could be triggered by this test.

---
 .../selftests/bpf/prog_tests/tailcalls.c      | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
tools/testing/selftests/bpf/prog_tests/tailcalls.c +51 -0
index c66037162da5..86d87d5817cf 100644
@@ -13,6 +13,8 @@
 #include "tailcall_cgrp_storage.skel.h"
 #include "tailcall_sleepable.skel.h"
 #include "tailcall_callback.skel.h"
+#include "tailcall_bpf2bpf2.skel.h"
+#include "tailcall_bpf2bpf_fexit.skel.h"
 
 /* test_tailcall_1 checks basic functionality by patching multiple locations
  * in a single program for a single tail call slot with nop->jmp, jmp->nop
@@ -1907,6 +1909,53 @@ static void test_tailcall_callback(void)
 	RUN_TESTS(tailcall_callback);
 }
 
+static void test_tailcall_bpf2bpf_fexit_links(void)
+{
+	struct tailcall_bpf2bpf_fexit *skel1 = NULL, *skel2 = NULL;
+	struct tailcall_bpf2bpf2 *skel_tc;
+	struct bpf_link *link;
+	int err, prog_fd;
+
+	skel_tc = tailcall_bpf2bpf2__open_and_load();
+	if (!ASSERT_OK_PTR(skel_tc, "tailcall_bpf2bpf2__open_and_load"))
+		return;
+
+	skel1 = tailcall_bpf2bpf_fexit__open();
+	if (!ASSERT_OK_PTR(skel1, "tailcall_bpf2bpf_fexit__open"))
+		goto out;
+
+	prog_fd = bpf_program__fd(skel_tc->progs.classifier_0);
+	err = bpf_program__set_attach_target(skel1->progs.fexit, prog_fd, "subprog_tail");
+	if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+		goto out;
+
+	err = tailcall_bpf2bpf_fexit__load(skel1);
+	if (!ASSERT_OK(err, "tailcall_bpf2bpf_fexit__load"))
+		goto out;
+
+	link = bpf_program__attach_trace(skel1->progs.fexit);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_trace"))
+		goto out;
+	skel1->links.fexit = link;
+
+	skel2 = tailcall_bpf2bpf_fexit__open();
+	if (!ASSERT_OK_PTR(skel2, "tailcall_bpf2bpf_fexit__open"))
+		goto out;
+
+	err = bpf_program__set_attach_target(skel2->progs.fexit, prog_fd, "subprog_tail");
+	if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+		goto out;
+
+	err = tailcall_bpf2bpf_fexit__load(skel2);
+	if (!ASSERT_OK(err, "tailcall_bpf2bpf_fexit__load"))
+		goto out;
+
+out:
+	tailcall_bpf2bpf_fexit__destroy(skel1);
+	tailcall_bpf2bpf_fexit__destroy(skel2);
+	tailcall_bpf2bpf2__destroy(skel_tc);
+}
+
 void test_tailcalls(void)
 {
 	if (test__start_subtest("tailcall_1"))
@@ -1974,4 +2023,6 @@ void test_tailcalls(void)
 	if (test__start_subtest("tailcall_cgrp_storage_no_storage_bridge"))
 		test_tailcall_cgrp_storage_no_storage_bridge();
 	test_tailcall_callback();
+	if (test__start_subtest("tailcall_bpf2bpf_fexit_links"))
+		test_tailcall_bpf2bpf_fexit_links();
 }
Signature · 1 line
2.55.0
Acked-by on v1 2/2
#5 lore raw

Re: [PATCH bpf-next 2/2] selftests/bpf: Verify no warning when close fexit link

Quoted 6 lines · Leon Hwang

Add a test to verify that there's no WARNING when detaching fexit link by following the repro steps of previous commit.

Without the fix, the WARNING could be triggered by this test.

left 2 nits below

jirka

Quoted 39 lines
---
 .../selftests/bpf/prog_tests/tailcalls.c      | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
tools/testing/selftests/bpf/prog_tests/tailcalls.c +20 -0
index c66037162da5..86d87d5817cf 100644
@@ -13,6 +13,8 @@
 #include "tailcall_cgrp_storage.skel.h"
 #include "tailcall_sleepable.skel.h"
 #include "tailcall_callback.skel.h"
+#include "tailcall_bpf2bpf2.skel.h"
+#include "tailcall_bpf2bpf_fexit.skel.h"
 
 /* test_tailcall_1 checks basic functionality by patching multiple locations
  * in a single program for a single tail call slot with nop->jmp, jmp->nop
@@ -1907,6 +1909,53 @@ static void test_tailcall_callback(void)
 	RUN_TESTS(tailcall_callback);
 }
 
+static void test_tailcall_bpf2bpf_fexit_links(void)
+{
+	struct tailcall_bpf2bpf_fexit *skel1 = NULL, *skel2 = NULL;
+	struct tailcall_bpf2bpf2 *skel_tc;
+	struct bpf_link *link;
+	int err, prog_fd;
+
+	skel_tc = tailcall_bpf2bpf2__open_and_load();
+	if (!ASSERT_OK_PTR(skel_tc, "tailcall_bpf2bpf2__open_and_load"))
+		return;
+
+	skel1 = tailcall_bpf2bpf_fexit__open();
+	if (!ASSERT_OK_PTR(skel1, "tailcall_bpf2bpf_fexit__open"))
+		goto out;
+
+	prog_fd = bpf_program__fd(skel_tc->progs.classifier_0);
+	err = bpf_program__set_attach_target(skel1->progs.fexit, prog_fd, "subprog_tail");
+	if (!ASSERT_OK(err, "bpf_program__set_attach_target"))

tools/testing/selftests/bpf/prog_tests/tailcalls.c static void test_tailcall_callback(void) · line 1939

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/tailcalls.c +10 -0
+		goto out;
+
+	err = tailcall_bpf2bpf_fexit__load(skel1);
+	if (!ASSERT_OK(err, "tailcall_bpf2bpf_fexit__load"))
+		goto out;
+
+	link = bpf_program__attach_trace(skel1->progs.fexit);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_trace"))
+		goto out;
+	skel1->links.fexit = link;

nit, you could use skel1->links.fexit directly and remove link

tools/testing/selftests/bpf/prog_tests/tailcalls.c static void test_tailcall_callback(void) · line 1951

Quoted 12 lines
tools/testing/selftests/bpf/prog_tests/tailcalls.c +12 -0
+
+	skel2 = tailcall_bpf2bpf_fexit__open();
+	if (!ASSERT_OK_PTR(skel2, "tailcall_bpf2bpf_fexit__open"))
+		goto out;
+
+	err = bpf_program__set_attach_target(skel2->progs.fexit, prog_fd, "subprog_tail");
+	if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+		goto out;
+
+	err = tailcall_bpf2bpf_fexit__load(skel2);
+	if (!ASSERT_OK(err, "tailcall_bpf2bpf_fexit__load"))
+		goto out;

nit, no need to if/goto, just ASSERT_OK would be enough

Quoted 20 lines
tools/testing/selftests/bpf/prog_tests/tailcalls.c +9 -0
+
+out:
+	tailcall_bpf2bpf_fexit__destroy(skel1);
+	tailcall_bpf2bpf_fexit__destroy(skel2);
+	tailcall_bpf2bpf2__destroy(skel_tc);
+}
+
 void test_tailcalls(void)
 {
 	if (test__start_subtest("tailcall_1"))
@@ -1974,4 +2023,6 @@ void test_tailcalls(void)
 	if (test__start_subtest("tailcall_cgrp_storage_no_storage_bridge"))
 		test_tailcall_cgrp_storage_no_storage_bridge();
 	test_tailcall_callback();
+	if (test__start_subtest("tailcall_bpf2bpf_fexit_links"))
+		test_tailcall_bpf2bpf_fexit_links();
 }
Signature · 1 line
2.55.0
author
#6 lore raw

Re: [PATCH bpf-next 2/2] selftests/bpf: Verify no warning when close fexit link

Quoted 9 lines · Jiri Olsa
Quoted 6 lines · Leon Hwang

Add a test to verify that there's no WARNING when detaching fexit link by following the repro steps of previous commit.

Without the fix, the WARNING could be triggered by this test.

left 2 nits below

Ack.

Will address them in the next revision.

Quoted 2 lines

Thanks,
Leon

Quoted 1 line

[...]