* [PATCH bpf-next v3 1/3] bpf: Relax tracing prog recursive attach rules
2023-11-29 19:16 [PATCH bpf-next v3 0/3] Relax tracing prog recursive attach rules Dmitrii Dolgov
@ 2023-11-29 19:16 ` Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add test for recursive attachment of tracing progs Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach Dmitrii Dolgov
2 siblings, 0 replies; 4+ messages in thread
From: Dmitrii Dolgov @ 2023-11-29 19:16 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yonghong.song,
dan.carpenter, olsajiri, Dmitrii Dolgov
Currently, it's not allowed to attach an fentry/fexit prog to another
one of the same type. At the same time it's not uncommon to see a
tracing program with lots of logic in use, and the attachment limitation
prevents usage of fentry/fexit for performance analysis (e.g. with
"bpftool prog profile" command) in this case. An example could be
falcosecurity libs project that uses tp_btf tracing programs.
Following the corresponding discussion [1], the reason for that is to
avoid tracing progs call cycles without introducing more complex
solutions. Relax "no same type" requirement to "no progs that are
already an attach target themselves" for the tracing type. In this way
only a standalone tracing program (without any other progs attached to
it) could be attached to another one, and no cycle could be formed. To
implement, add a new field into bpf_prog_aux to track the number of
attachments to the target prog.
As a side effect of this change alone, one could create an unbounded
chain of tracing progs attached to each other. Similar issues between
fentry/fexit and extend progs are addressed via forbidding certain
combinations that could lead to similar chains. Introduce an
attach_depth field to limit the attachment chain, and display it in
bpftool.
Note, that currently, due to various limitations, it's actually not
possible to form such an attachment cycle the original implementation
was prohibiting. It seems that the idea was to make this part robust
even in the view of potential future changes.
[1]: https://lore.kernel.org/bpf/20191108064039.2041889-16-ast@kernel.org/
Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
Previous discussion: https://lore.kernel.org/bpf/20231122191816.5572-1-9erthalion6@gmail.com/
Changes in v3:
- Fix incorrect decreasing of attach_depth, setting to 0 instead
- Place bookkeeping later, to not miss a cleanup if needed
- Display attach_depth in bpftool only if the value is not 0
Changes in v2:
- Verify tgt_prog is not null
- Replace boolean followed with number of followers, to handle
multiple progs attaching/detaching
include/linux/bpf.h | 2 ++
include/uapi/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 12 +++++++++++-
kernel/bpf/verifier.c | 19 ++++++++++++++++---
tools/bpf/bpftool/prog.c | 3 +++
tools/include/uapi/linux/bpf.h | 1 +
6 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4001d11be151..1b890f65ac39 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1394,6 +1394,8 @@ struct bpf_prog_aux {
u32 real_func_cnt; /* includes hidden progs, only used for JIT and freeing progs */
u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
u32 attach_btf_id; /* in-kernel BTF type id to attach to */
+ u32 attach_depth; /* position of the prog in the attachment chain */
+ u32 follower_cnt; /* number of programs attached to it */
u32 ctx_arg_info_size;
u32 max_rdonly_access;
u32 max_rdwr_access;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 7cf8bcf9f6a2..aa6614547ef6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6465,6 +6465,7 @@ struct bpf_prog_info {
__u32 verified_insns;
__u32 attach_btf_obj_id;
__u32 attach_btf_id;
+ __u32 attach_depth;
} __attribute__((aligned(8)));
struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0ed286b8a0f0..15a47308bbdd 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3038,9 +3038,12 @@ static void bpf_tracing_link_release(struct bpf_link *link)
bpf_trampoline_put(tr_link->trampoline);
+ link->prog->aux->attach_depth = 0;
/* tgt_prog is NULL if target is a kernel function */
- if (tr_link->tgt_prog)
+ if (tr_link->tgt_prog) {
+ tr_link->tgt_prog->aux->follower_cnt--;
bpf_prog_put(tr_link->tgt_prog);
+ }
}
static void bpf_tracing_link_dealloc(struct bpf_link *link)
@@ -3242,6 +3245,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
goto out_unlock;
}
+ if (tgt_prog) {
+ /* Bookkeeping for managing the prog attachment chain. */
+ tgt_prog->aux->follower_cnt++;
+ prog->aux->attach_depth = tgt_prog->aux->attach_depth + 1;
+ }
+
link->tgt_prog = tgt_prog;
link->trampoline = tr;
@@ -4509,6 +4518,7 @@ static int bpf_prog_get_info_by_fd(struct file *file,
if (prog->aux->btf)
info.btf_id = btf_obj_id(prog->aux->btf);
info.attach_btf_id = prog->aux->attach_btf_id;
+ info.attach_depth = prog->aux->attach_depth;
if (attach_btf)
info.attach_btf_obj_id = btf_obj_id(attach_btf);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9ae6eae13471..de058a83d769 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20329,6 +20329,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
if (tgt_prog) {
struct bpf_prog_aux *aux = tgt_prog->aux;
+ if (aux->attach_depth >= 32) {
+ bpf_log(log, "Target program attach depth is %d. Too large\n",
+ aux->attach_depth);
+ return -EINVAL;
+ }
+
if (bpf_prog_is_dev_bound(prog->aux) &&
!bpf_prog_dev_bound_match(prog, tgt_prog)) {
bpf_log(log, "Target program bound device mismatch");
@@ -20367,9 +20373,16 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
bpf_log(log, "Can attach to only JITed progs\n");
return -EINVAL;
}
- if (tgt_prog->type == prog->type) {
- /* Cannot fentry/fexit another fentry/fexit program.
- * Cannot attach program extension to another extension.
+ if (tgt_prog->type == prog->type &&
+ (prog_extension || prog->aux->follower_cnt > 0)) {
+ /*
+ * To avoid potential call chain cycles, prevent attaching programs
+ * of the same type. The only exception is standalone fentry/fexit
+ * programs that themselves are not attachment targets.
+ * That means:
+ * - Cannot attach followed fentry/fexit to another
+ * fentry/fexit program.
+ * - Cannot attach program extension to another extension.
* It's ok to attach fentry/fexit to extension program.
*/
bpf_log(log, "Cannot recursively attach\n");
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 7ec4f5671e7a..f5cebd0a4a64 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -554,6 +554,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
printf(" memlock %sB", memlock);
free(memlock);
+ if (info->attach_depth)
+ printf(" attach depth %d", info->attach_depth);
+
if (info->nr_map_ids)
show_prog_maps(fd, info->nr_map_ids);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 7cf8bcf9f6a2..aa6614547ef6 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6465,6 +6465,7 @@ struct bpf_prog_info {
__u32 verified_insns;
__u32 attach_btf_obj_id;
__u32 attach_btf_id;
+ __u32 attach_depth;
} __attribute__((aligned(8)));
struct bpf_map_info {
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH bpf-next v3 2/3] selftests/bpf: Add test for recursive attachment of tracing progs
2023-11-29 19:16 [PATCH bpf-next v3 0/3] Relax tracing prog recursive attach rules Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 1/3] bpf: " Dmitrii Dolgov
@ 2023-11-29 19:16 ` Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach Dmitrii Dolgov
2 siblings, 0 replies; 4+ messages in thread
From: Dmitrii Dolgov @ 2023-11-29 19:16 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yonghong.song,
dan.carpenter, olsajiri, Dmitrii Dolgov
Verify the fact that an fentry prog could be attached to another one,
building up an attachment chain of limited size. Use existing
bpf_testmod as a start of the chain.
Note, that currently it's not possible to:
* form a cycle within an attachment chain
* splice two attachment chains
Those limitations are coming from the fact that attach_prog_fd is
specified at the prog load (thus making it impossible to attach to a
program loaded after it in this way), as well as tracing progs not
implementing link_detach. This is the reason there is only one test for
the chain length.
Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
.../bpf/prog_tests/recursive_attach.c | 85 +++++++++++++++++++
.../selftests/bpf/progs/fentry_recursive.c | 19 +++++
.../bpf/progs/fentry_recursive_target.c | 20 +++++
3 files changed, 124 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/recursive_attach.c
create mode 100644 tools/testing/selftests/bpf/progs/fentry_recursive.c
create mode 100644 tools/testing/selftests/bpf/progs/fentry_recursive_target.c
diff --git a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
new file mode 100644
index 000000000000..9c422dd92c4e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Red Hat, Inc. */
+#include <test_progs.h>
+#include "fentry_recursive.skel.h"
+#include "fentry_recursive_target.skel.h"
+#include <bpf/btf.h>
+#include "bpf/libbpf_internal.h"
+
+#define ATTACH_DEPTH 33
+
+/*
+ * Test following scenarios:
+ * - one can attach fentry progs recursively, one fentry to another one
+ * - an attachment chain generated this way is limited, after 32 attachment no
+ * more progs could be added
+ */
+void test_recursive_fentry_attach(void)
+{
+ struct fentry_recursive_target *target_skel = NULL;
+ struct fentry_recursive *tracing_chain[ATTACH_DEPTH + 1] = {};
+ struct bpf_program *prog;
+ int prev_fd, err;
+
+ target_skel = fentry_recursive_target__open_and_load();
+ if (!ASSERT_OK_PTR(target_skel, "fentry_recursive_target__open_and_load"))
+ goto close_prog;
+
+ /* This is going to be the start of the chain */
+ tracing_chain[0] = fentry_recursive__open();
+ if (!ASSERT_OK_PTR(tracing_chain[0], "fentry_recursive__open"))
+ goto close_prog;
+
+ prog = tracing_chain[0]->progs.recursive_attach;
+ prev_fd = bpf_program__fd(target_skel->progs.test1);
+ err = bpf_program__set_attach_target(prog, prev_fd, "test1");
+ if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+ goto close_prog;
+
+ err = fentry_recursive__load(tracing_chain[0]);
+ if (!ASSERT_OK(err, "fentry_recursive__load"))
+ goto close_prog;
+
+ /* Create an attachment chain to exhaust the limit */
+ for (int i = 1; i < ATTACH_DEPTH; i++) {
+ tracing_chain[i] = fentry_recursive__open();
+ if (!ASSERT_OK_PTR(tracing_chain[i], "fentry_recursive__open"))
+ goto close_prog;
+
+ prog = tracing_chain[i]->progs.recursive_attach;
+ prev_fd = bpf_program__fd(tracing_chain[i-1]->progs.recursive_attach);
+ err = bpf_program__set_attach_target(prog, prev_fd, "recursive_attach");
+ if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+ goto close_prog;
+
+ err = fentry_recursive__load(tracing_chain[i]);
+ if (!ASSERT_OK(err, "fentry_recursive__load"))
+ goto close_prog;
+
+ err = fentry_recursive__attach(tracing_chain[i]);
+ if (!ASSERT_OK(err, "fentry_recursive__attach"))
+ goto close_prog;
+ }
+
+ /* The next attachment would fail */
+ tracing_chain[ATTACH_DEPTH] = fentry_recursive__open();
+ if (!ASSERT_OK_PTR(tracing_chain[ATTACH_DEPTH], "last fentry_recursive__open"))
+ goto close_prog;
+
+ prog = tracing_chain[ATTACH_DEPTH]->progs.recursive_attach;
+ prev_fd = bpf_program__fd(tracing_chain[ATTACH_DEPTH - 1]->progs.recursive_attach);
+ err = bpf_program__set_attach_target(prog, prev_fd, "recursive_attach");
+ if (!ASSERT_OK(err, "last bpf_program__set_attach_target"))
+ goto close_prog;
+
+ err = fentry_recursive__load(tracing_chain[ATTACH_DEPTH]);
+ if (!ASSERT_ERR(err, "last fentry_recursive__load"))
+ goto close_prog;
+
+close_prog:
+ fentry_recursive_target__destroy(target_skel);
+ for (int i = 1; i < ATTACH_DEPTH + 1; i++) {
+ if (tracing_chain[i])
+ fentry_recursive__destroy(tracing_chain[i]);
+ }
+}
diff --git a/tools/testing/selftests/bpf/progs/fentry_recursive.c b/tools/testing/selftests/bpf/progs/fentry_recursive.c
new file mode 100644
index 000000000000..1df490230344
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/fentry_recursive.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Red Hat, Inc. */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 test1_result = 0;
+
+/*
+ * Dummy fentry bpf prog for testing fentry attachment chains
+ */
+SEC("fentry/XXX")
+int BPF_PROG(recursive_attach, int a)
+{
+ test1_result = a == 1;
+ return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/fentry_recursive_target.c b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
new file mode 100644
index 000000000000..b6fb8ebd598d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Red Hat, Inc. */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 test1_result = 0;
+
+/*
+ * Dummy fentry bpf prog for testing fentry attachment chains. It's going to be
+ * a start of the chain.
+ */
+SEC("fentry/bpf_testmod_fentry_test1")
+int BPF_PROG(test1, int a)
+{
+ test1_result = a == 1;
+ return 0;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH bpf-next v3 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach
2023-11-29 19:16 [PATCH bpf-next v3 0/3] Relax tracing prog recursive attach rules Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 1/3] bpf: " Dmitrii Dolgov
2023-11-29 19:16 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add test for recursive attachment of tracing progs Dmitrii Dolgov
@ 2023-11-29 19:16 ` Dmitrii Dolgov
2 siblings, 0 replies; 4+ messages in thread
From: Dmitrii Dolgov @ 2023-11-29 19:16 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yonghong.song,
dan.carpenter, olsajiri, Dmitrii Dolgov
It looks like there is an issue in bpf_tracing_prog_attach, in the
"prog->aux->dst_trampoline and tgt_prog is NULL" case. One can construct
a sequence of events when prog->aux->attach_btf will be NULL, and
bpf_trampoline_compute_key will fail.
BUG: kernel NULL pointer dereference, address: 0000000000000058
Call Trace:
<TASK>
? __die+0x20/0x70
? page_fault_oops+0x15b/0x430
? fixup_exception+0x22/0x330
? exc_page_fault+0x6f/0x170
? asm_exc_page_fault+0x22/0x30
? bpf_tracing_prog_attach+0x279/0x560
? btf_obj_id+0x5/0x10
bpf_tracing_prog_attach+0x439/0x560
__sys_bpf+0x1cf4/0x2de0
__x64_sys_bpf+0x1c/0x30
do_syscall_64+0x41/0xf0
entry_SYSCALL_64_after_hwframe+0x6e/0x76
The issue seems to be not relevant to the previous changes with
recursive tracing prog attach, because the reproducing test doesn't
actually include recursive fentry attaching.
Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
kernel/bpf/syscall.c | 4 +-
.../bpf/prog_tests/recursive_attach.c | 48 +++++++++++++++++++
.../bpf/progs/fentry_recursive_target.c | 11 +++++
3 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 15a47308bbdd..5cd4a7a39a03 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3196,7 +3196,9 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
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->attach_btf)
+ key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf,
+ btf_id);
}
if (!prog->aux->dst_trampoline ||
diff --git a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
index 9c422dd92c4e..a4abf1745e62 100644
--- a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
@@ -83,3 +83,51 @@ void test_recursive_fentry_attach(void)
fentry_recursive__destroy(tracing_chain[i]);
}
}
+
+/*
+ * Test that a tracing prog reattachment (when we land in
+ * "prog->aux->dst_trampoline and tgt_prog is NULL" branch in
+ * bpf_tracing_prog_attach) does not lead to a crash due to missing attach_btf
+ */
+void test_fentry_attach_btf_presence(void)
+{
+ struct fentry_recursive_target *target_skel = NULL;
+ struct fentry_recursive *tracing_skel = NULL;
+ struct bpf_program *prog;
+ int err, link_fd, tgt_prog_fd;
+
+ target_skel = fentry_recursive_target__open_and_load();
+ if (!ASSERT_OK_PTR(target_skel, "fentry_recursive_target__open_and_load"))
+ goto close_prog;
+
+ tracing_skel = fentry_recursive__open();
+ if (!ASSERT_OK_PTR(tracing_skel, "fentry_recursive__open"))
+ goto close_prog;
+
+ prog = tracing_skel->progs.recursive_attach;
+ tgt_prog_fd = bpf_program__fd(target_skel->progs.fentry_target);
+ err = bpf_program__set_attach_target(prog, tgt_prog_fd, "fentry_target");
+ if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+ goto close_prog;
+
+ err = fentry_recursive__load(tracing_skel);
+ if (!ASSERT_OK(err, "fentry_recursive__load"))
+ goto close_prog;
+
+ LIBBPF_OPTS(bpf_link_create_opts, link_opts);
+
+ link_fd = bpf_link_create(bpf_program__fd(tracing_skel->progs.recursive_attach),
+ 0, BPF_TRACE_FENTRY, &link_opts);
+ if (!ASSERT_GE(link_fd, 0, "link_fd"))
+ goto close_prog;
+
+ fentry_recursive__detach(tracing_skel);
+
+ err = fentry_recursive__attach(tracing_skel);
+ if (!ASSERT_ERR(err, "fentry_recursive__attach"))
+ goto close_prog;
+
+close_prog:
+ fentry_recursive_target__destroy(target_skel);
+ fentry_recursive__destroy(tracing_skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/fentry_recursive_target.c b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
index b6fb8ebd598d..f812d2de0c3c 100644
--- a/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
+++ b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
@@ -18,3 +18,14 @@ int BPF_PROG(test1, int a)
test1_result = a == 1;
return 0;
}
+
+/*
+ * Dummy bpf prog for testing attach_btf presence when attaching an fentry
+ * program.
+ */
+SEC("raw_tp/sys_enter")
+int BPF_PROG(fentry_target, struct pt_regs *regs, long id)
+{
+ test1_result = id == 1;
+ return 0;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread