[PATCH bpf-next v4 0/3] Relax tracing prog recursive attach rules

[PATCH bpf-next v4 0/3] Relax tracing prog recursive attach rules

[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 for
offloading certain part of logic into tail-called programs, but the
use-case is still generic enough -- a tracing program could be
complicated and heavy enough to warrant its profiling, yet frustratingly
it's not possible to do so use best tooling for that.

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.

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.

The series contains a test for recursive attachment, as well as a fix +
test for what looks like an issue in re-attachment branch of
bpf_tracing_prog_attach. When preparing the test for the main change
set, I've stumbled upon the possibility to construct a sequence of
events when attach_btf would be NULL while computing a trampoline key.
It doesn't look like this issue is triggered by the main change,
because the reproduces doesn't actually need to have an fentry
attachment chain.

[1]: https://lore.kernel.org/bpf/20191108064039.2041889-16-ast@kernel.org/

Dmitrii Dolgov (3):
  bpf: Relax tracing prog recursive attach rules
  selftests/bpf: Add test for recursive attachment of tracing progs
  bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

 include/linux/bpf.h                           |   2 +
 include/uapi/linux/bpf.h                      |   1 +
 kernel/bpf/syscall.c                          |  16 ++-
 kernel/bpf/verifier.c                         |  19 ++-
 tools/bpf/bpftool/prog.c                      |   3 +
 tools/include/uapi/linux/bpf.h                |   1 +
 .../bpf/prog_tests/recursive_attach.c         | 133 ++++++++++++++++++
 .../selftests/bpf/progs/fentry_recursive.c    |  19 +++
 .../bpf/progs/fentry_recursive_target.c       |  31 ++++
 9 files changed, 220 insertions(+), 5 deletions(-)
 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


base-commit: 40d0eb0259ae77ace3e81d7454d1068c38bc95c2
-- 
2.41.0

[PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[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>
---
 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 eb447b0a9423..d7ace97d8e4b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1400,6 +1400,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 e88746ba7d21..9cf45ad914f1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6468,6 +6468,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 5e43ddd1b83f..a595d7a62dbc 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3039,9 +3039,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)
@@ -3243,6 +3246,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;
 
@@ -4510,6 +4519,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 8e7b6072e3f4..31ffcffb7198 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20109,6 +20109,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");
@@ -20147,9 +20153,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 feb8e305804f..83f999f5505d 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -558,6 +558,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd, bool orphaned)
 	if (orphaned)
 		printf("  orphaned");
 
+	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 e88746ba7d21..9cf45ad914f1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6468,6 +6468,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

[PATCH bpf-next v4 2/3] selftests/bpf: Add test for recursive attachment of tracing progs

[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

[PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[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 a595d7a62dbc..e01a949dfed7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3197,7 +3197,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

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Song Liu]

On Wed, Nov 29, 2023 at 11:56 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote:
>
> 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>

We discussed this in earlier version:

"
> If prog B attached to prog A, and prog C attached to prog B, then we
> detach B. At this point, can we re-attach B to A?

Nope, with the proposed changes it still wouldn't be possible to
reattach B to A (if we're talking about tracing progs of course),
because this time B is an attachment target on its own.
"

I think this can be problematic for some users. Basically, doing
profiling on prog B can cause it to not work (cannot re-attach).

Given it is not possible to create a call circle, shall we remove
this issue?

Thanks,
Song

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Dmitry Dolgov]

> On Wed, Nov 29, 2023 at 03:58:02PM -0800, Song Liu wrote:
> We discussed this in earlier version:
>
> "
> > If prog B attached to prog A, and prog C attached to prog B, then we
> > detach B. At this point, can we re-attach B to A?
>
> Nope, with the proposed changes it still wouldn't be possible to
> reattach B to A (if we're talking about tracing progs of course),
> because this time B is an attachment target on its own.
> "
>
> I think this can be problematic for some users. Basically, doing
> profiling on prog B can cause it to not work (cannot re-attach).

Sorry, I was probably not clear enough about this first time. Let me
elaborate:

* The patch affects only tracing programs (only they can reach the
  corresponding verifier change), so I assume in your example at least B
  and A are fentry/fexit.

* The patch is less restrictive than the current kernel implementation.
  Currently, no attach of a tracing program to another tracing program is
  possible, thus IIUC the case you describe (A, B: tracing, C -> B -> A,
  then re-attach B -> A) is not possible without the patch (the first B
  -> A is going to return a verifier error).

* I've also tried to reproduce this use case with the patch, and noticed
  that link_detach is not supported for tracing progs. Which means the
  re-attach part in (C -> B -> A) has to be done via unloading of prog B
  and C, then reattaching them one-by-one back. This is another
  limitation why the case above doesn't seem to be possible (attaching
  one-by-one back would of course work without any issues even with the
  patch).

Does it all make sense to you, or am I missing something about the
problem you describe?

> Given it is not possible to create a call circle, shall we remove
> this issue?

I was originally thinking about this when preparing the patch, even
independently of the question above, simply remove verifier limitation
for an impossible situation sounds interesting. I see the following
pros/cons:

* Just remove the verifier limitation on recursive attachment is of
  course easier.

* At the same time it makes the implementation less "defensive" against
  future changes.

* Tracking attachment depth & followers might be useful in some other
  contexts.

All in all I've decided that more elaborated approach is slightly
better. But if everyone in the community agrees that less
"defensiveness" is not an issue and verifier could be simply made less
restrictive, I'm fine with that. What do you think?

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Jiri Olsa]

On Wed, Nov 29, 2023 at 08:52:36PM +0100, Dmitrii Dolgov wrote:
> 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.

SNIP

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8e7b6072e3f4..31ffcffb7198 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20109,6 +20109,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;
> +		}

IIUC the use case you have is to be able to attach fentry program on
another fentry program.. do you have use case that needs more than
that?

could we allow just single nesting? that might perhaps endup in easier
code while still allowing your use case?


> +
>  		if (bpf_prog_is_dev_bound(prog->aux) &&
>  		    !bpf_prog_dev_bound_match(prog, tgt_prog)) {
>  			bpf_log(log, "Target program bound device mismatch");
> @@ -20147,9 +20153,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.

next condition below denies extension on fentry/fexit and the reason
is the possibility:
  "... to create long call chain * fentry->extension->fentry->extension
  beyond reasonable stack size ..."

that might be problem also here with 32 allowed nesting

also the the comment mentions that it's not possible to attach fentry/fexit
on themselfs, so it should be updated

jirka

>  			 */
>  			bpf_log(log, "Cannot recursively attach\n");
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index feb8e305804f..83f999f5505d 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -558,6 +558,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd, bool orphaned)
>  	if (orphaned)
>  		printf("  orphaned");
>  
> +	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 e88746ba7d21..9cf45ad914f1 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -6468,6 +6468,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
> 

Re: [PATCH bpf-next v4 2/3] selftests/bpf: Add test for recursive attachment of tracing progs

[Jiri Olsa]

On Wed, Nov 29, 2023 at 08:52:37PM +0100, Dmitrii Dolgov wrote:

SNIP

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

should you call fentry_recursive__attach in here as well?

> +
> +	/* 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);

or maybe better brach here for (i == 0) and call

  bpf_program__set_attach_target(prog, prev_fd, "test1");


> +		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++) {

i = 0 ?

jirka

> +		if (tracing_chain[i])
> +			fentry_recursive__destroy(tracing_chain[i]);
> +	}
> +}

SNIP

Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Jiri Olsa]

On Wed, Nov 29, 2023 at 08:52:38PM +0100, Dmitrii Dolgov wrote:
> 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 a595d7a62dbc..e01a949dfed7 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3197,7 +3197,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);
>  	}

nice catch.. I'd think dst_trampoline would caught it, because the
program is loaded with attach_prog_fd=x and check_attach_btf_id should
create dst_trampoline.. hum

jirka

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

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Dmitry Dolgov]

> On Thu, Nov 30, 2023 at 03:30:38PM +0100, Jiri Olsa wrote:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8e7b6072e3f4..31ffcffb7198 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -20109,6 +20109,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;
> > +		}
>
> IIUC the use case you have is to be able to attach fentry program on
> another fentry program.. do you have use case that needs more than
> that?
>
> could we allow just single nesting? that might perhaps endup in easier
> code while still allowing your use case?

Right, there is no use case beyond attaching an fentry to another one,
so having just a single nesting should be fine.

> > +			/*
> > +			 * 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.
>
> next condition below denies extension on fentry/fexit and the reason
> is the possibility:
>   "... to create long call chain * fentry->extension->fentry->extension
>   beyond reasonable stack size ..."
>
> that might be problem also here with 32 allowed nesting

Reducing nesting to only one level probably will lift this question, but
for posterity, what kind of problem similar to "fentry->extension->fentry->..."
do you have in mind?

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Song Liu]

On Thu, Nov 30, 2023 at 2:08 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Wed, Nov 29, 2023 at 03:58:02PM -0800, Song Liu wrote:
> > We discussed this in earlier version:
> >
> > "
> > > If prog B attached to prog A, and prog C attached to prog B, then we
> > > detach B. At this point, can we re-attach B to A?
> >
> > Nope, with the proposed changes it still wouldn't be possible to
> > reattach B to A (if we're talking about tracing progs of course),
> > because this time B is an attachment target on its own.
> > "
> >
> > I think this can be problematic for some users. Basically, doing
> > profiling on prog B can cause it to not work (cannot re-attach).
>
> Sorry, I was probably not clear enough about this first time. Let me
> elaborate:
>
> * The patch affects only tracing programs (only they can reach the
>   corresponding verifier change), so I assume in your example at least B
>   and A are fentry/fexit.
>
> * The patch is less restrictive than the current kernel implementation.
>   Currently, no attach of a tracing program to another tracing program is
>   possible, thus IIUC the case you describe (A, B: tracing, C -> B -> A,
>   then re-attach B -> A) is not possible without the patch (the first B
>   -> A is going to return a verifier error).

Yes, I was aware this is less restrictive than current rules, and I think
this can be very useful.

> * I've also tried to reproduce this use case with the patch, and noticed
>   that link_detach is not supported for tracing progs. Which means the
>   re-attach part in (C -> B -> A) has to be done via unloading of prog B
>   and C, then reattaching them one-by-one back. This is another
>   limitation why the case above doesn't seem to be possible (attaching
>   one-by-one back would of course work without any issues even with the
>   patch).

I think there is an issue without re-attach:
1. Load program A, B, C;
2. Attach C to B;
3. Attach B to A will fail.

> Does it all make sense to you, or am I missing something about the
> problem you describe?
>
> > Given it is not possible to create a call circle, shall we remove
> > this issue?
>
> I was originally thinking about this when preparing the patch, even
> independently of the question above, simply remove verifier limitation
> for an impossible situation sounds interesting. I see the following
> pros/cons:
>
> * Just remove the verifier limitation on recursive attachment is of
>   course easier.
>
> * At the same time it makes the implementation less "defensive" against
>   future changes.
>
> * Tracking attachment depth & followers might be useful in some other
>   contexts.
>
> All in all I've decided that more elaborated approach is slightly
> better. But if everyone in the community agrees that less
> "defensiveness" is not an issue and verifier could be simply made less
> restrictive, I'm fine with that. What do you think?

I think the follower_cnt check is not necessary, and may cause confusions.
For tracing programs, we are very specific on "which function(s) are we
tracing". So I don't think circular attachment can be a real issue. Do we
have potential use cases that make the circular attach possible?

Thanks,
Song

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Dmitry Dolgov]

> On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > All in all I've decided that more elaborated approach is slightly
> > better. But if everyone in the community agrees that less
> > "defensiveness" is not an issue and verifier could be simply made less
> > restrictive, I'm fine with that. What do you think?
>
> I think the follower_cnt check is not necessary, and may cause confusions.
> For tracing programs, we are very specific on "which function(s) are we
> tracing". So I don't think circular attachment can be a real issue. Do we
> have potential use cases that make the circular attach possible?

At the moment no, nothing like that in sight. Ok, you've convinced me --
plus since nobody has yet actively mentioned that potential cycle
prevention is nice to have, I can drop follower_cnt and the
corresponding check in the verifier.

Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Jiri Olsa]

On Thu, Nov 30, 2023 at 04:14:55PM +0100, Jiri Olsa wrote:
> On Wed, Nov 29, 2023 at 08:52:38PM +0100, Dmitrii Dolgov wrote:
> > 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 a595d7a62dbc..e01a949dfed7 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3197,7 +3197,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);
> >  	}
> 
> nice catch.. I'd think dst_trampoline would caught it, because the
> program is loaded with attach_prog_fd=x and check_attach_btf_id should
> create dst_trampoline.. hum

looks like we don't handle case like this one:

  1) load rawtp program
  2) load fentry program with rawtp as target_fd
  3) create tracing link for fentry program with target_fd = 0
  4) repeat 3

in 3 we will use prog->aux->dst_trampoline and prog->aux->dst_prog
(set from fentry loading) to attach the link, and then set both to NULL

in 4 we have:

  - prog->aux->dst_trampoline == NULL
  - tgt_prog == NULL (because we did not provide target_fd to link_create)
  - prog->aux->attach_btf == NULL (becase program was loaded with attach_prog_fd=X)

AFAICS we can't do anything here, because program was loaded for tgt_prog but we
have no way to find out which one.. so return -EINVAL, like in the patch below

jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5e43ddd1b83f..558ce7bdd781 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3180,6 +3180,10 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	 *
 	 * - if prog->aux->dst_trampoline and tgt_prog is NULL, the program
 	 *   was detached and is going for re-attachment.
+	 *
+	 * - if prog->aux->dst_trampoline is NULL and tgt_prog and prog->aux->attach_btf
+	 *   are NULL, then program was already attached and user did not provide
+	 *   tgt_prog_fd so we have no way to find out or create trampoline
 	 */
 	if (!prog->aux->dst_trampoline && !tgt_prog) {
 		/*
@@ -3193,6 +3197,11 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 			err = -EINVAL;
 			goto out_unlock;
 		}
+		/* We can allow re-attach only if we have valid attach_btf. */
+		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);
 	}

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Jiri Olsa]

On Thu, Nov 30, 2023 at 07:57:52PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 03:30:38PM +0100, Jiri Olsa wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 8e7b6072e3f4..31ffcffb7198 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -20109,6 +20109,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;
> > > +		}
> >
> > IIUC the use case you have is to be able to attach fentry program on
> > another fentry program.. do you have use case that needs more than
> > that?
> >
> > could we allow just single nesting? that might perhaps endup in easier
> > code while still allowing your use case?
> 
> Right, there is no use case beyond attaching an fentry to another one,
> so having just a single nesting should be fine.
> 
> > > +			/*
> > > +			 * 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.
> >
> > next condition below denies extension on fentry/fexit and the reason
> > is the possibility:
> >   "... to create long call chain * fentry->extension->fentry->extension
> >   beyond reasonable stack size ..."
> >
> > that might be problem also here with 32 allowed nesting
> 
> Reducing nesting to only one level probably will lift this question, but
> for posterity, what kind of problem similar to "fentry->extension->fentry->..."
> do you have in mind?

I meant that allowing that amount of nesting will make it easier to get
to a point where we use all the available stack size

jirka

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Artem Savkov]

On Thu, Nov 30, 2023 at 09:41:34PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > > All in all I've decided that more elaborated approach is slightly
> > > better. But if everyone in the community agrees that less
> > > "defensiveness" is not an issue and verifier could be simply made less
> > > restrictive, I'm fine with that. What do you think?
> >
> > I think the follower_cnt check is not necessary, and may cause confusions.
> > For tracing programs, we are very specific on "which function(s) are we
> > tracing". So I don't think circular attachment can be a real issue. Do we
> > have potential use cases that make the circular attach possible?
> 
> At the moment no, nothing like that in sight. Ok, you've convinced me --
> plus since nobody has yet actively mentioned that potential cycle
> prevention is nice to have, I can drop follower_cnt and the
> corresponding check in the verifier.

If you are worried about potential future situations where cyclic
attaches are possible would it make sense to add a test that checks if
this fails?

-- 
Regards,
  Artem

Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Dmitry Dolgov]

> On Thu, Nov 30, 2023 at 11:30:29PM +0100, Jiri Olsa wrote:
> AFAICS we can't do anything here, because program was loaded for tgt_prog but we
> have no way to find out which one.. so return -EINVAL, like in the patch below

Yep, makes sense. Is that fine if I include this patch into the series
with you as an author, with signed-off and everything?

Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

[Dmitry Dolgov]

> On Fri, Dec 01, 2023 at 10:55:09AM +0100, Artem Savkov wrote:
> On Thu, Nov 30, 2023 at 09:41:34PM +0100, Dmitry Dolgov wrote:
> > > On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > > > All in all I've decided that more elaborated approach is slightly
> > > > better. But if everyone in the community agrees that less
> > > > "defensiveness" is not an issue and verifier could be simply made less
> > > > restrictive, I'm fine with that. What do you think?
> > >
> > > I think the follower_cnt check is not necessary, and may cause confusions.
> > > For tracing programs, we are very specific on "which function(s) are we
> > > tracing". So I don't think circular attachment can be a real issue. Do we
> > > have potential use cases that make the circular attach possible?
> >
> > At the moment no, nothing like that in sight. Ok, you've convinced me --
> > plus since nobody has yet actively mentioned that potential cycle
> > prevention is nice to have, I can drop follower_cnt and the
> > corresponding check in the verifier.
>
> If you are worried about potential future situations where cyclic
> attaches are possible would it make sense to add a test that checks if
> this fails?

Do you mean a test that cyclic attachment doesn't work due to the
current limitations (not the one in verifier)? Sounds interesting, but
I'm hesitant to add such a test -- it would verify a property that is
more like a side effect of e.g. having attach_prog_fd at prog load, and
most likely will be either incomplete or flaky.

At the moment I'm pretty convinced that even if the future changes will
make cycles possible, it's something that has to be discussed at the
point when such change will land, and it's fine for now to simplify
things.

Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Jiri Olsa]

On Fri, Dec 01, 2023 at 03:21:43PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 11:30:29PM +0100, Jiri Olsa wrote:
> > AFAICS we can't do anything here, because program was loaded for tgt_prog but we
> > have no way to find out which one.. so return -EINVAL, like in the patch below
> 
> Yep, makes sense. Is that fine if I include this patch into the series
> with you as an author, with signed-off and everything?

sure, thanks

jirka