[PATCH bpf-next v7 0/4] Relax tracing prog recursive attach rules

[PATCH bpf-next v7 0/4] Relax tracing prog recursive attach rules

[Dmitrii Dolgov]

Currently, it's not allowed to attach an fentry/fexit prog to another
fentry/fexit. 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. But currently it seems impossible to load and attach tracing
programs in a way that will form such a cycle. Replace "no same type"
requirement with verification that no more than one level of attachment
nesting is allowed. In this way only one fentry/fexit program could be
attached to another fentry/fexit to cover profiling use case, and still
no cycle could be formed.

The series contains a test for recursive attachment, as well as a fix +
test for 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
  selftests/bpf: Test re-attachment fix for bpf_tracing_prog_attach

Jiri Olsa (1):
  bpf: Fix re-attachment branch in bpf_tracing_prog_attach

 include/linux/bpf.h                           |   1 +
 kernel/bpf/syscall.c                          |  16 +++
 kernel/bpf/verifier.c                         |  39 +++---
 .../bpf/prog_tests/recursive_attach.c         | 117 ++++++++++++++++++
 .../selftests/bpf/progs/fentry_recursive.c    |  19 +++
 .../bpf/progs/fentry_recursive_target.c       |  31 +++++
 6 files changed, 209 insertions(+), 14 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 v7 1/4] bpf: Relax tracing prog recursive attach rules

[Dmitrii Dolgov]

Currently, it's not allowed to attach an fentry/fexit prog to another
one fentry/fexit. 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. But currently it seems impossible to load and attach tracing
programs in a way that will form such a cycle. The limitation is 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.

Replace "no same type" requirement with verification that no more than
one level of attachment nesting is allowed. In this way only one
fentry/fexit program could be attached to another fentry/fexit to cover
profiling use case, and still no cycle could be formed. To implement,
add a new field into bpf_prog_aux to track nested attachment for tracing
programs.

[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/20231202191556.30997-1-9erthalion6@gmail.com/

Changes in v7:
    - Replace attach_depth with a boolean flag to indicate a program is
      already tracing an fentry/fexit.

Changes in v6:
    - Apply nesting level limitation only to tracing programs, otherwise
      it's possible to apply it in "fentry->extension" case and break it

Changes in v5:
    - Remove follower_cnt and drop unreachable cycle prevention condition
    - Allow only one level of attachment nesting
    - Do not display attach_depth in bpftool, as it doesn't make sense
      anymore

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   |  1 +
 kernel/bpf/syscall.c  |  7 +++++++
 kernel/bpf/verifier.c | 39 +++++++++++++++++++++++++--------------
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eb447b0a9423..e7393674ab94 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1414,6 +1414,7 @@ struct bpf_prog_aux {
 	bool dev_bound; /* Program is bound to the netdev. */
 	bool offload_requested; /* Program is bound and offloaded to the netdev. */
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
+	bool attach_tracing_prog; /* true if tracing another tracing program */
 	bool func_proto_unreliable;
 	bool sleepable;
 	bool tail_call_reachable;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5e43ddd1b83f..d5470a5c8c6d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3039,6 +3039,7 @@ static void bpf_tracing_link_release(struct bpf_link *link)
 
 	bpf_trampoline_put(tr_link->trampoline);
 
+	link->prog->aux->attach_tracing_prog = false;
 	/* tgt_prog is NULL if target is a kernel function */
 	if (tr_link->tgt_prog)
 		bpf_prog_put(tr_link->tgt_prog);
@@ -3243,6 +3244,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 		goto out_unlock;
 	}
 
+	/* Bookkeeping for managing the prog attachment chain */
+	if (tgt_prog &&
+		prog->type == BPF_PROG_TYPE_TRACING &&
+		tgt_prog->type == BPF_PROG_TYPE_TRACING)
+		prog->aux->attach_tracing_prog = true;
+
 	link->tgt_prog = tgt_prog;
 	link->trampoline = tr;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8e7b6072e3f4..f8c15ce8fd05 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20077,6 +20077,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    struct bpf_attach_target_info *tgt_info)
 {
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
+	bool prog_tracing = prog->type == BPF_PROG_TYPE_TRACING;
 	const char prefix[] = "btf_trace_";
 	int ret = 0, subprog = -1, i;
 	const struct btf_type *t;
@@ -20147,10 +20148,21 @@ 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.
-			 * It's ok to attach fentry/fexit to extension program.
+		if (prog_tracing) {
+			if (aux->attach_tracing_prog) {
+				/*
+				 * Target program is an fentry/fexit which is already attached
+				 * to another tracing program. More levels of nesting
+				 * attachment are not allowed.
+				 */
+				bpf_log(log, "Cannot nest tracing program attach more than once\n");
+				return -EINVAL;
+			}
+		} else if (tgt_prog->type == prog->type) {
+			/*
+			 * To avoid potential call chain cycles, prevent attaching of a
+			 * program extension to another extension. It's ok to attach
+			 * fentry/fexit to extension program.
 			 */
 			bpf_log(log, "Cannot recursively attach\n");
 			return -EINVAL;
@@ -20163,16 +20175,15 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			 * except fentry/fexit. The reason is the following.
 			 * The fentry/fexit programs are used for performance
 			 * analysis, stats and can be attached to any program
-			 * type except themselves. When extension program is
-			 * replacing XDP function it is necessary to allow
-			 * performance analysis of all functions. Both original
-			 * XDP program and its program extension. Hence
-			 * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
-			 * allowed. If extending of fentry/fexit was allowed it
-			 * would be possible to create long call chain
-			 * fentry->extension->fentry->extension beyond
-			 * reasonable stack size. Hence extending fentry is not
-			 * allowed.
+			 * type. When extension program is replacing XDP function
+			 * it is necessary to allow performance analysis of all
+			 * functions. Both original XDP program and its program
+			 * extension. Hence attaching fentry/fexit to
+			 * BPF_PROG_TYPE_EXT is allowed. If extending of
+			 * fentry/fexit was allowed it would be possible to create
+			 * long call chain fentry->extension->fentry->extension
+			 * beyond reasonable stack size. Hence extending fentry
+			 * is not allowed.
 			 */
 			bpf_log(log, "Cannot extend fentry/fexit\n");
 			return -EINVAL;
-- 
2.41.0

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

[Dmitrii Dolgov]

Verify the fact that only one fentry prog could be attached to another
fentry, building up an attachment chain of limited size. Use existing
bpf_testmod as a start of the chain.

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
Changes in v5:
    - Test only one level of attachment

 .../bpf/prog_tests/recursive_attach.c         | 69 +++++++++++++++++++
 .../selftests/bpf/progs/fentry_recursive.c    | 19 +++++
 .../bpf/progs/fentry_recursive_target.c       | 20 ++++++
 3 files changed, 108 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..7248d0661ee9
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
@@ -0,0 +1,69 @@
+// 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"
+
+/*
+ * Test following scenarios:
+ * - attach one fentry progs to another one
+ * - more than one nesting levels are not allowed
+ */
+void test_recursive_fentry_attach(void)
+{
+	struct fentry_recursive_target *target_skel = NULL;
+	struct fentry_recursive *tracing_chain[2] = {};
+	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;
+
+	/* Create an attachment chain with two fentry progs */
+	for (int i = 0; i < 2; i++) {
+		tracing_chain[i] = fentry_recursive__open();
+		if (!ASSERT_OK_PTR(tracing_chain[i], "fentry_recursive__open"))
+			goto close_prog;
+
+		/*
+		 * The first prog in the chain is going to be attached to the target
+		 * fentry program, the second one to the previous in the chain.
+		 */
+		if (i == 0) {
+			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");
+		} else {
+			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]);
+		/* The first attach should succeed, the second fail */
+		if (i == 0) {
+			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;
+		} else {
+			if (!ASSERT_ERR(err, "fentry_recursive__load"))
+				goto close_prog;
+		}
+	}
+
+close_prog:
+	fentry_recursive_target__destroy(target_skel);
+	for (int i = 0; i < 2; 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 v7 3/4] bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Dmitrii Dolgov]

From: Jiri Olsa <olsajiri@gmail.com>

The following case can cause a crash due to missing attach_btf:

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 the end 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 (the program was loaded with attach_prog_fd=X)
- the program was loaded for tgt_prog but we have no way to find out which one

    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

Return -EINVAL in this situation.

Signed-off-by: Jiri Olsa <olsajiri@gmail.com>
Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 kernel/bpf/syscall.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d5470a5c8c6d..2b111fa7637d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3181,6 +3181,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) {
 		/*
@@ -3194,6 +3198,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);
 	}
-- 
2.41.0

[PATCH bpf-next v7 4/4] selftests/bpf: Test re-attachment fix for bpf_tracing_prog_attach

[Dmitrii Dolgov]

Add a test case to verify the fix for "prog->aux->dst_trampoline and
tgt_prog is NULL" branch in bpf_tracing_prog_attach. The sequence of
events:

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

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 .../bpf/prog_tests/recursive_attach.c         | 48 +++++++++++++++++++
 .../bpf/progs/fentry_recursive_target.c       | 11 +++++
 2 files changed, 59 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
index 7248d0661ee9..6296bcf95481 100644
--- a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
@@ -67,3 +67,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 v7 1/4] bpf: Relax tracing prog recursive attach rules

[Jiri Olsa]

On Fri, Dec 08, 2023 at 07:55:53PM +0100, Dmitrii Dolgov wrote:

SNIP

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index eb447b0a9423..e7393674ab94 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1414,6 +1414,7 @@ struct bpf_prog_aux {
>  	bool dev_bound; /* Program is bound to the netdev. */
>  	bool offload_requested; /* Program is bound and offloaded to the netdev. */
>  	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
> +	bool attach_tracing_prog; /* true if tracing another tracing program */
>  	bool func_proto_unreliable;
>  	bool sleepable;
>  	bool tail_call_reachable;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 5e43ddd1b83f..d5470a5c8c6d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3039,6 +3039,7 @@ static void bpf_tracing_link_release(struct bpf_link *link)
>  
>  	bpf_trampoline_put(tr_link->trampoline);
>  
> +	link->prog->aux->attach_tracing_prog = false;

I think it'd be better to have this as part of the 'if (tr_link->tgt_prog)'
path below, because it's set only for that case

>  	/* tgt_prog is NULL if target is a kernel function */
>  	if (tr_link->tgt_prog)
>  		bpf_prog_put(tr_link->tgt_prog);
> @@ -3243,6 +3244,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>  		goto out_unlock;
>  	}
>  
> +	/* Bookkeeping for managing the prog attachment chain */
> +	if (tgt_prog &&
> +		prog->type == BPF_PROG_TYPE_TRACING &&
> +		tgt_prog->type == BPF_PROG_TYPE_TRACING)
> +		prog->aux->attach_tracing_prog = true;

wrong indentation in here, please check the if conditions around

thanks,
jirka

> +
>  	link->tgt_prog = tgt_prog;
>  	link->trampoline = tr;
>  
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8e7b6072e3f4..f8c15ce8fd05 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20077,6 +20077,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			    struct bpf_attach_target_info *tgt_info)
>  {
>  	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
> +	bool prog_tracing = prog->type == BPF_PROG_TYPE_TRACING;
>  	const char prefix[] = "btf_trace_";
>  	int ret = 0, subprog = -1, i;
>  	const struct btf_type *t;
> @@ -20147,10 +20148,21 @@ 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.
> -			 * It's ok to attach fentry/fexit to extension program.
> +		if (prog_tracing) {
> +			if (aux->attach_tracing_prog) {
> +				/*
> +				 * Target program is an fentry/fexit which is already attached
> +				 * to another tracing program. More levels of nesting
> +				 * attachment are not allowed.
> +				 */
> +				bpf_log(log, "Cannot nest tracing program attach more than once\n");
> +				return -EINVAL;
> +			}
> +		} else if (tgt_prog->type == prog->type) {
> +			/*
> +			 * To avoid potential call chain cycles, prevent attaching of a
> +			 * program extension to another extension. It's ok to attach
> +			 * fentry/fexit to extension program.
>  			 */
>  			bpf_log(log, "Cannot recursively attach\n");
>  			return -EINVAL;
> @@ -20163,16 +20175,15 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			 * except fentry/fexit. The reason is the following.
>  			 * The fentry/fexit programs are used for performance
>  			 * analysis, stats and can be attached to any program
> -			 * type except themselves. When extension program is
> -			 * replacing XDP function it is necessary to allow
> -			 * performance analysis of all functions. Both original
> -			 * XDP program and its program extension. Hence
> -			 * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
> -			 * allowed. If extending of fentry/fexit was allowed it
> -			 * would be possible to create long call chain
> -			 * fentry->extension->fentry->extension beyond
> -			 * reasonable stack size. Hence extending fentry is not
> -			 * allowed.
> +			 * type. When extension program is replacing XDP function
> +			 * it is necessary to allow performance analysis of all
> +			 * functions. Both original XDP program and its program
> +			 * extension. Hence attaching fentry/fexit to
> +			 * BPF_PROG_TYPE_EXT is allowed. If extending of
> +			 * fentry/fexit was allowed it would be possible to create
> +			 * long call chain fentry->extension->fentry->extension
> +			 * beyond reasonable stack size. Hence extending fentry
> +			 * is not allowed.
>  			 */
>  			bpf_log(log, "Cannot extend fentry/fexit\n");
>  			return -EINVAL;
> -- 
> 2.41.0
> 

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

[Jiri Olsa]

On Fri, Dec 08, 2023 at 07:55:54PM +0100, Dmitrii Dolgov wrote:
> Verify the fact that only one fentry prog could be attached to another
> fentry, building up an attachment chain of limited size. Use existing
> bpf_testmod as a start of the chain.
> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> Changes in v5:
>     - Test only one level of attachment
> 
>  .../bpf/prog_tests/recursive_attach.c         | 69 +++++++++++++++++++
>  .../selftests/bpf/progs/fentry_recursive.c    | 19 +++++
>  .../bpf/progs/fentry_recursive_target.c       | 20 ++++++
>  3 files changed, 108 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..7248d0661ee9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> @@ -0,0 +1,69 @@
> +// 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"
> +
> +/*
> + * Test following scenarios:
> + * - attach one fentry progs to another one
> + * - more than one nesting levels are not allowed
> + */
> +void test_recursive_fentry_attach(void)
> +{
> +	struct fentry_recursive_target *target_skel = NULL;
> +	struct fentry_recursive *tracing_chain[2] = {};
> +	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;
> +
> +	/* Create an attachment chain with two fentry progs */
> +	for (int i = 0; i < 2; i++) {
> +		tracing_chain[i] = fentry_recursive__open();
> +		if (!ASSERT_OK_PTR(tracing_chain[i], "fentry_recursive__open"))
> +			goto close_prog;
> +
> +		/*
> +		 * The first prog in the chain is going to be attached to the target
> +		 * fentry program, the second one to the previous in the chain.
> +		 */
> +		if (i == 0) {
> +			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");
> +		} else {
> +			prog = tracing_chain[i]->progs.recursive_attach;

nit, common line, could be placed before the loop

perhaps also the bpf_program__set_attach_target call does not need to be
in the if path, I think it should work with NULL for attach_func_name as
long as we provide attach_prog_fd

I wonder now with just 2 skels the test might be easier to read
without the loop, but I dont have strong opinion about that

> +			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]);
> +		/* The first attach should succeed, the second fail */
> +		if (i == 0) {
> +			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;
> +		} else {
> +			if (!ASSERT_ERR(err, "fentry_recursive__load"))
> +				goto close_prog;
> +		}
> +	}
> +
> +close_prog:
> +	fentry_recursive_target__destroy(target_skel);
> +	for (int i = 0; i < 2; 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;

there's no reason to keep test1_result in here, please remove

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

ditto

thanks,
jirka

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

Re: [PATCH bpf-next v7 4/4] selftests/bpf: Test re-attachment fix for bpf_tracing_prog_attach

[Jiri Olsa]

On Fri, Dec 08, 2023 at 07:55:56PM +0100, Dmitrii Dolgov wrote:
> Add a test case to verify the fix for "prog->aux->dst_trampoline and
> tgt_prog is NULL" branch in bpf_tracing_prog_attach. The sequence of
> events:
> 
> 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
> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
>  .../bpf/prog_tests/recursive_attach.c         | 48 +++++++++++++++++++
>  .../bpf/progs/fentry_recursive_target.c       | 11 +++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> index 7248d0661ee9..6296bcf95481 100644
> --- a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> @@ -67,3 +67,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);

we don't need link_opts, you can pass NULL to below bpf_link_create call

> +
> +	link_fd = bpf_link_create(bpf_program__fd(tracing_skel->progs.recursive_attach),
> +							  0, BPF_TRACE_FENTRY, &link_opts);

wrong indentation

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

no need to call goto in here, let's just have ASSERT_ERR without the if

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

please remove test1_result

thanks,
jirka

> +	return 0;
> +}
> -- 
> 2.41.0
> 

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

[Dmitry Dolgov]

> On Mon, Dec 11, 2023 at 01:30:24PM +0100, Jiri Olsa wrote:
> > +	/* Bookkeeping for managing the prog attachment chain */
> > +	if (tgt_prog &&
> > +		prog->type == BPF_PROG_TYPE_TRACING &&
> > +		tgt_prog->type == BPF_PROG_TYPE_TRACING)
> > +		prog->aux->attach_tracing_prog = true;
>
> wrong indentation in here, please check the if conditions around

I'm a bit confused here, why is the indentation here wrong? IIUC "if"
predicates have to be aligned on the same column, with padding where
needed. Or is it always necessary to expand the last tab with spaces?

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

[Dmitry Dolgov]

> On Mon, Dec 11, 2023 at 01:30:33PM +0100, Jiri Olsa wrote:
> > +		/*
> > +		 * The first prog in the chain is going to be attached to the target
> > +		 * fentry program, the second one to the previous in the chain.
> > +		 */
> > +		if (i == 0) {
> > +			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");
> > +		} else {
> > +			prog = tracing_chain[i]->progs.recursive_attach;

> perhaps also the bpf_program__set_attach_target call does not need to be
> in the if path, I think it should work with NULL for attach_func_name as
> long as we provide attach_prog_fd

Just have tried that, but it didn't work -- when loading, libbpf tried
to search for XXX in BTF.

> I wonder now with just 2 skels the test might be easier to read
> without the loop, but I dont have strong opinion about that

Yeah, I was pondering that too. But eventually I came to the conclusion
that the difference for readability would be marginal, and went with
keeping the loop.

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

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:49:08PM +0100, Dmitry Dolgov wrote:
> > On Mon, Dec 11, 2023 at 01:30:24PM +0100, Jiri Olsa wrote:
> > > +	/* Bookkeeping for managing the prog attachment chain */
> > > +	if (tgt_prog &&
> > > +		prog->type == BPF_PROG_TYPE_TRACING &&
> > > +		tgt_prog->type == BPF_PROG_TYPE_TRACING)
> > > +		prog->aux->attach_tracing_prog = true;
> >
> > wrong indentation in here, please check the if conditions around
> 
> I'm a bit confused here, why is the indentation here wrong? IIUC "if"
> predicates have to be aligned on the same column, with padding where
> needed. Or is it always necessary to expand the last tab with spaces?

I meant it should have looked something like this:

	if (tgt_prog &&
	    prog->type == BPF_PROG_TYPE_TRACING &&
	    tgt_prog->type == BPF_PROG_TYPE_TRACING)
		prog->aux->attach_tracing_prog = true;

jirka