fentry/fexit attach to EXT type XDP program does not work

fentry/fexit attach to EXT type XDP program does not work

[Eelco Chaudron]

I'm trying for a while to do a fentry/fexit trace an EXT program
attached to an XDP program. To make it easier to explain I've
created a test case (see patch below) to show the issue.

Without the changes to test_xdp_bpf2bpf.c I'll get the following error:

  libbpf: -- BEGIN DUMP LOG ---
  libbpf:
  arg#0 type is not a struct
  Unrecognized arg#0 type PTR
  ; int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
  0: (79) r6 = *(u64 *)(r1 +0)
  invalid bpf_context access off=0 size=8
  processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

  libbpf: -- END LOG --
  libbpf: failed to load program 'fentry/FUNC'
  libbpf: failed to load object 'test_xdp_bpf2bpf'
  libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
  test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
  #91 xdp_fentry_ext:FAIL
  Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

With the change I get the following (but I do feel this change
should not be needed):

  libbpf: -- BEGIN DUMP LOG ---
  libbpf:
  Unrecognized arg#0 type PTR
  ; int trace_on_entry(struct xdp_buff *xdp)
  0: (bf) r6 = r1
  ; void *data = (void *)(long)xdp->data;
  1: (79) r1 = *(u64 *)(r6 +0)
  invalid bpf_context access off=0 size=8
  processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

  libbpf: -- END LOG --
  libbpf: failed to load program 'fentry/FUNC'
  libbpf: failed to load object 'test_xdp_bpf2bpf'
  libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
  test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
  #91 xdp_fentry_ext:FAIL
  Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

Any idea what could be the case here? The same fentry/fexit attach
code works fine in the xdp_bpf2bpf.c tests case.


Cheers,


Eelco
---
 .../selftests/bpf/prog_tests/xdp_fentry_ext.c      |   95 ++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c b/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c
new file mode 100644
index 000000000000..68cd83fad632
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include <network_helpers.h>
+
+#include "test_xdp_ext.skel.h"
+#include "test_xdp_bpf2bpf.skel.h"
+#include "xdp_dummy.skel.h"
+
+void test_xdp_fentry_ext(void)
+{
+        /* Using the skeleton framework does not work, as it does not like
+         * like the prog_replace() function to be noinline
+         */
+
+	__u32 duration = 0, retval, size;
+        const char *file = "./test_xdp_ext.o";
+        const char *ext_file = "./xdp_dummy.o";
+        struct bpf_program *prog;
+        struct bpf_program *ext_prog;
+	struct bpf_object *xdp_obj, *ext_obj = NULL;
+        struct test_xdp_bpf2bpf *ftrace_skel = NULL;
+        int err, xdp_fd, ext_fd;
+	char buf[128];
+
+        err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &xdp_obj, &xdp_fd);
+	if (CHECK_FAIL(err))
+		return;
+
+        /* Load the EXT program to attach to replace existing function */
+        ext_obj = bpf_object__open_file(ext_file, NULL);
+        if (CHECK(IS_ERR_OR_NULL(ext_obj), "obj_open",
+                  "failed to open %s: %ld\n",
+                  ext_file, PTR_ERR(ext_obj)))
+                goto out;
+
+        ext_prog = bpf_object__find_program_by_title(ext_obj, "xdp_dummy");
+        if (CHECK(!ext_prog, "find_prog", "xdp_dummy_prog not found\n"))
+                goto out;
+
+        err = bpf_program__set_attach_target(ext_prog, xdp_fd, "prog_replace");
+	if (CHECK(err, "set_attach", "err %d, errno %d\n", err, errno))
+                goto out;
+
+        bpf_program__set_type(ext_prog, BPF_PROG_TYPE_EXT);
+
+        err = bpf_object__load(ext_obj);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto out;
+
+        bpf_program__attach_trace(ext_prog);
+
+        ext_fd = bpf_program__fd(ext_prog);
+
+        /* Now try to attach an fentry trace to the EXT program above */
+
+	ftrace_skel = test_xdp_bpf2bpf__open();
+	if (CHECK(!ftrace_skel, "__open", "ftrace skeleton failed\n"))
+		goto out;
+
+        prog = ftrace_skel->progs.trace_on_entry;
+	bpf_program__set_expected_attach_type(prog, BPF_TRACE_FENTRY);
+	bpf_program__set_attach_target(prog, ext_fd, "xdp_dummy_prog");
+
+	prog = ftrace_skel->progs.trace_on_exit;
+	bpf_program__set_expected_attach_type(prog, BPF_TRACE_FEXIT);
+	bpf_program__set_attach_target(prog, ext_fd, "xdp_dummy_prog");
+
+	err = test_xdp_bpf2bpf__load(ftrace_skel);
+	if (CHECK(err, "__load", "ftrace skeleton failed\n"))
+		goto out;
+
+	err = test_xdp_bpf2bpf__attach(ftrace_skel);
+	if (CHECK(err, "ftrace_attach", "ftrace attach failed: %d\n", err))
+		goto out;
+
+
+	/* Execute the xdp program by sending a dummy packet */
+	err = bpf_prog_test_run(xdp_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				buf, &size, &retval, &duration);
+
+	if (CHECK(err || retval != XDP_PASS , "packet",
+		  "err %d, errno %d, retval %d %c= %d\n",
+                  err, errno, retval, retval != XDP_PASS ? '!' : '=', XDP_PASS))
+		goto out;
+
+out:
+	bpf_object__close(xdp_obj);
+        if (ext_obj)
+                bpf_object__close(ext_obj);
+        if (ftrace_skel)
+                test_xdp_bpf2bpf__destroy(ftrace_skel);
+
+}
+
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c b/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
index a038e827f850..41b2a103c7ca 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
@@ -42,7 +42,8 @@ struct {
 
 __u64 test_result_fentry = 0;
 SEC("fentry/FUNC")
-int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
+//int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
+int trace_on_entry(struct xdp_buff *xdp)
 {
 	struct meta meta;
 	void *data_end = (void *)(long)xdp->data_end;
@@ -61,7 +62,8 @@ int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
 
 __u64 test_result_fexit = 0;
 SEC("fexit/FUNC")
-int BPF_PROG(trace_on_exit, struct xdp_buff *xdp, int ret)
+//int BPF_PROG(trace_on_exit, struct xdp_buff *xdp, int ret)
+int trace_on_exit(struct xdp_buff *xdp, int ret)
 {
 	test_result_fexit = ret;
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_ext.c b/tools/testing/selftests/bpf/progs/test_xdp_ext.c
new file mode 100644
index 000000000000..37bd16c95b36
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_xdp_ext.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__u64 xdp_ext_called = 0;
+
+__attribute__ ((noinline))
+int prog_replace(struct xdp_md *ctx) {
+        volatile int ret = XDP_ABORTED;
+
+        return ret;
+}
+
+SEC("xdp_ext_call")
+int xdp_ext_call_func(struct xdp_md *ctx)
+{
+
+        return prog_replace(ctx);
+}
+
+char _license[] SEC("license") = "GPL";

Re: fentry/fexit attach to EXT type XDP program does not work

[Yonghong Song]

On 6/8/20 7:11 AM, Eelco Chaudron wrote:
> I'm trying for a while to do a fentry/fexit trace an EXT program
> attached to an XDP program. To make it easier to explain I've
> created a test case (see patch below) to show the issue.
>
> Without the changes to test_xdp_bpf2bpf.c I'll get the following error:
>
>    libbpf: -- BEGIN DUMP LOG ---
>    libbpf:
>    arg#0 type is not a struct
>    Unrecognized arg#0 type PTR
>    ; int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
>    0: (79) r6 = *(u64 *)(r1 +0)
>    invalid bpf_context access off=0 size=8
>    processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>
>    libbpf: -- END LOG --
>    libbpf: failed to load program 'fentry/FUNC'
>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>    #91 xdp_fentry_ext:FAIL
>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> With the change I get the following (but I do feel this change
> should not be needed):
>
>    libbpf: -- BEGIN DUMP LOG ---
>    libbpf:
>    Unrecognized arg#0 type PTR
>    ; int trace_on_entry(struct xdp_buff *xdp)
>    0: (bf) r6 = r1
>    ; void *data = (void *)(long)xdp->data;
>    1: (79) r1 = *(u64 *)(r6 +0)
>    invalid bpf_context access off=0 size=8
>    processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>
>    libbpf: -- END LOG --
>    libbpf: failed to load program 'fentry/FUNC'
>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>    #91 xdp_fentry_ext:FAIL
>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> Any idea what could be the case here? The same fentry/fexit attach
> code works fine in the xdp_bpf2bpf.c tests case.
>
>
> Cheers,
>
>
> Eelco
> ---
>   .../selftests/bpf/prog_tests/xdp_fentry_ext.c      |   95 ++++++++++++++++++++
>   1 file changed, 95 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c b/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c
> new file mode 100644
> index 000000000000..68cd83fad632
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_fentry_ext.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +#include "test_xdp_ext.skel.h"
> +#include "test_xdp_bpf2bpf.skel.h"
> +#include "xdp_dummy.skel.h"
> +
> +void test_xdp_fentry_ext(void)
> +{
> +        /* Using the skeleton framework does not work, as it does not like
> +         * like the prog_replace() function to be noinline
> +         */
> +
> +	__u32 duration = 0, retval, size;
> +        const char *file = "./test_xdp_ext.o";
> +        const char *ext_file = "./xdp_dummy.o";
> +        struct bpf_program *prog;
> +        struct bpf_program *ext_prog;
> +	struct bpf_object *xdp_obj, *ext_obj = NULL;
> +        struct test_xdp_bpf2bpf *ftrace_skel = NULL;
> +        int err, xdp_fd, ext_fd;
> +	char buf[128];
> +
> +        err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &xdp_obj, &xdp_fd);
> +	if (CHECK_FAIL(err))
> +		return;
> +
> +        /* Load the EXT program to attach to replace existing function */
> +        ext_obj = bpf_object__open_file(ext_file, NULL);
> +        if (CHECK(IS_ERR_OR_NULL(ext_obj), "obj_open",
> +                  "failed to open %s: %ld\n",
> +                  ext_file, PTR_ERR(ext_obj)))
> +                goto out;
> +
> +        ext_prog = bpf_object__find_program_by_title(ext_obj, "xdp_dummy");
> +        if (CHECK(!ext_prog, "find_prog", "xdp_dummy_prog not found\n"))
> +                goto out;
> +
> +        err = bpf_program__set_attach_target(ext_prog, xdp_fd, "prog_replace");
> +	if (CHECK(err, "set_attach", "err %d, errno %d\n", err, errno))
> +                goto out;
> +
> +        bpf_program__set_type(ext_prog, BPF_PROG_TYPE_EXT);
> +
> +        err = bpf_object__load(ext_obj);
> +	if (CHECK(err, "obj_load", "err %d\n", err))
> +		goto out;
> +
> +        bpf_program__attach_trace(ext_prog);
> +
> +        ext_fd = bpf_program__fd(ext_prog);
> +
> +        /* Now try to attach an fentry trace to the EXT program above */
> +
> +	ftrace_skel = test_xdp_bpf2bpf__open();
> +	if (CHECK(!ftrace_skel, "__open", "ftrace skeleton failed\n"))
> +		goto out;

I think this is not supported now. That is, you cannot attach a fentry trace

to the EXT program. The current implementation for fentry program simply

trying to find and match the signature of freplace program which by default

is a pointer to void.


It is doable in that in kernel we could recognize to-be-attached program is

a freplace and further trace down to find the real signature. The related

kernel function is btf_get_prog_ctx_type(). You can try to implement by 
yourself

or I can have a patch for this once bpf-next opens.


> +
> +        prog = ftrace_skel->progs.trace_on_entry;
> +	bpf_program__set_expected_attach_type(prog, BPF_TRACE_FENTRY);
> +	bpf_program__set_attach_target(prog, ext_fd, "xdp_dummy_prog");
> +
> +	prog = ftrace_skel->progs.trace_on_exit;
> +	bpf_program__set_expected_attach_type(prog, BPF_TRACE_FEXIT);
> +	bpf_program__set_attach_target(prog, ext_fd, "xdp_dummy_prog");
> +
> +	err = test_xdp_bpf2bpf__load(ftrace_skel);
> +	if (CHECK(err, "__load", "ftrace skeleton failed\n"))
> +		goto out;
> +
> +	err = test_xdp_bpf2bpf__attach(ftrace_skel);
> +	if (CHECK(err, "ftrace_attach", "ftrace attach failed: %d\n", err))
> +		goto out;
> +
> +
> +	/* Execute the xdp program by sending a dummy packet */
> +	err = bpf_prog_test_run(xdp_fd, 1, &pkt_v4, sizeof(pkt_v4),
> +				buf, &size, &retval, &duration);
> +
> +	if (CHECK(err || retval != XDP_PASS , "packet",
> +		  "err %d, errno %d, retval %d %c= %d\n",
> +                  err, errno, retval, retval != XDP_PASS ? '!' : '=', XDP_PASS))
> +		goto out;
> +
> +out:
> +	bpf_object__close(xdp_obj);
> +        if (ext_obj)
> +                bpf_object__close(ext_obj);
> +        if (ftrace_skel)
> +                test_xdp_bpf2bpf__destroy(ftrace_skel);
> +
> +}
> +
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c b/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
> index a038e827f850..41b2a103c7ca 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c
> @@ -42,7 +42,8 @@ struct {
>   
>   __u64 test_result_fentry = 0;
>   SEC("fentry/FUNC")
> -int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
> +//int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
> +int trace_on_entry(struct xdp_buff *xdp)
>   {
>   	struct meta meta;
>   	void *data_end = (void *)(long)xdp->data_end;
> @@ -61,7 +62,8 @@ int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
>   
>   __u64 test_result_fexit = 0;
>   SEC("fexit/FUNC")
> -int BPF_PROG(trace_on_exit, struct xdp_buff *xdp, int ret)
> +//int BPF_PROG(trace_on_exit, struct xdp_buff *xdp, int ret)
> +int trace_on_exit(struct xdp_buff *xdp, int ret)
>   {
>   	test_result_fexit = ret;
>   	return 0;
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_ext.c b/tools/testing/selftests/bpf/progs/test_xdp_ext.c
> new file mode 100644
> index 000000000000..37bd16c95b36
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_ext.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +__u64 xdp_ext_called = 0;
> +
> +__attribute__ ((noinline))
> +int prog_replace(struct xdp_md *ctx) {
> +        volatile int ret = XDP_ABORTED;
> +
> +        return ret;
> +}
> +
> +SEC("xdp_ext_call")
> +int xdp_ext_call_func(struct xdp_md *ctx)
> +{
> +
> +        return prog_replace(ctx);
> +}
> +
> +char _license[] SEC("license") = "GPL";
>

Re: fentry/fexit attach to EXT type XDP program does not work

[Eelco Chaudron]

On 8 Jun 2020, at 18:58, Yonghong Song wrote:

> On 6/8/20 7:11 AM, Eelco Chaudron wrote:
>> I'm trying for a while to do a fentry/fexit trace an EXT program
>> attached to an XDP program. To make it easier to explain I've
>> created a test case (see patch below) to show the issue.
>>
>> Without the changes to test_xdp_bpf2bpf.c I'll get the following 
>> error:
>>
>>    libbpf: -- BEGIN DUMP LOG ---
>>    libbpf:
>>    arg#0 type is not a struct
>>    Unrecognized arg#0 type PTR
>>    ; int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
>>    0: (79) r6 = *(u64 *)(r1 +0)
>>    invalid bpf_context access off=0 size=8
>>    processed 1 insns (limit 1000000) max_states_per_insn 0 
>> total_states 0 peak_states 0 mark_read 0
>>
>>    libbpf: -- END LOG --
>>    libbpf: failed to load program 'fentry/FUNC'
>>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>>    #91 xdp_fentry_ext:FAIL
>>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>
>> With the change I get the following (but I do feel this change
>> should not be needed):
>>
>>    libbpf: -- BEGIN DUMP LOG ---
>>    libbpf:
>>    Unrecognized arg#0 type PTR
>>    ; int trace_on_entry(struct xdp_buff *xdp)
>>    0: (bf) r6 = r1
>>    ; void *data = (void *)(long)xdp->data;
>>    1: (79) r1 = *(u64 *)(r6 +0)
>>    invalid bpf_context access off=0 size=8
>>    processed 2 insns (limit 1000000) max_states_per_insn 0 
>> total_states 0 peak_states 0 mark_read 0
>>
>>    libbpf: -- END LOG --
>>    libbpf: failed to load program 'fentry/FUNC'
>>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>>    #91 xdp_fentry_ext:FAIL
>>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>
>> Any idea what could be the case here? The same fentry/fexit attach
>> code works fine in the xdp_bpf2bpf.c tests case.

<SNIP>
>
> I think this is not supported now. That is, you cannot attach a fentry 
> trace
> to the EXT program. The current implementation for fentry program 
> simply
> trying to find and match the signature of freplace program which by 
> default
> is a pointer to void.
>
> It is doable in that in kernel we could recognize to-be-attached 
> program is
> a freplace and further trace down to find the real signature. The 
> related
> kernel function is btf_get_prog_ctx_type(). You can try to implement 
> by yourself
> or I can have a patch for this once bpf-next opens.

I’m not familiar with this area of the code, so if you could prepare a 
patch that would nice.
You can also send it to me before bpf-next opens and I can verify it, 
and clean up the self-test so it can be included as well.

Re: fentry/fexit attach to EXT type XDP program does not work

[Jiri Olsa]

On Tue, Jun 09, 2020 at 10:52:34AM +0200, Eelco Chaudron wrote:

SNIP

> > >    libbpf: failed to load object 'test_xdp_bpf2bpf'
> > >    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
> > >    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
> > >    #91 xdp_fentry_ext:FAIL
> > >    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
> > > 
> > > Any idea what could be the case here? The same fentry/fexit attach
> > > code works fine in the xdp_bpf2bpf.c tests case.
> 
> <SNIP>
> > 
> > I think this is not supported now. That is, you cannot attach a fentry
> > trace
> > to the EXT program. The current implementation for fentry program simply
> > trying to find and match the signature of freplace program which by
> > default
> > is a pointer to void.
> > 
> > It is doable in that in kernel we could recognize to-be-attached program
> > is
> > a freplace and further trace down to find the real signature. The
> > related
> > kernel function is btf_get_prog_ctx_type(). You can try to implement by
> > yourself
> > or I can have a patch for this once bpf-next opens.
> 
> I’m not familiar with this area of the code, so if you could prepare a patch
> that would nice.
> You can also send it to me before bpf-next opens and I can verify it, and
> clean up the self-test so it can be included as well.
> 

hi,
it seems that you cannot exten fentry/fexit programs,
but it's possible to attach fentry/fexit to ext program.

   /* Program extensions can extend all program types
    * 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.
    */

I changed fexit_bpf2bpf.c test just to do a quick check
and it seems to work:

  # echo > /sys/kernel/debug/tracing/trace
  #  ./test_progs -t fexit_bpf2bpf 
  #25 fexit_bpf2bpf:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
  # cat /sys/kernel/debug/tracing/trace | tail -2
           <...>-75365 [012] d... 313932.416780: bpf_trace_printk: ENTRY val 123
           <...>-75365 [012] d... 313932.416784: bpf_trace_printk: EXIT  val 123, ret 1

jirka


---
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index a895bfed55db..4b6c26ac2362 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -17,6 +17,7 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
 	struct bpf_map *data_map;
 	const int zero = 0;
 	u64 *result = NULL;
+	int ext_fd;
 
 	err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
 			    &pkt_obj, &pkt_fd);
@@ -51,11 +52,50 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
 		link[i] = bpf_program__attach_trace(prog[i]);
 		if (CHECK(IS_ERR(link[i]), "attach_trace", "failed to link\n"))
 			goto close_prog;
+
+		if (!strcmp(prog_name[i], "freplace/get_constant"))
+			ext_fd = bpf_program__fd(prog[i]);
 	}
 
 	if (!run_prog)
 		goto close_prog;
 
+{
+	struct bpf_object *obj_trace = NULL;
+	struct bpf_program *prog_trace = NULL;
+	struct bpf_link *link_trace = NULL;
+
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts_trace,
+			    .attach_prog_fd = ext_fd,
+			   );
+
+	obj_trace = bpf_object__open_file("trace_ext.o", &opts_trace);
+	if (CHECK(IS_ERR_OR_NULL(obj_trace), "obj_open",
+		  "failed to open %s: %ld\n", obj_file,
+		  PTR_ERR(obj_trace)))
+		goto close_prog;
+
+	err = bpf_object__load(obj_trace);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto close_prog;
+
+	prog_trace = bpf_object__find_program_by_title(obj_trace, "fexit/new_get_constant");
+	if (CHECK(!prog_trace, "find_prog", "prog %s not found\n", "fexit/new_get_constant"))
+		goto close_prog;
+
+	link_trace = bpf_program__attach_trace(prog_trace);
+	if (CHECK(IS_ERR(link_trace), "attach_trace", "failed to link\n"))
+		goto close_prog;
+
+	prog_trace = bpf_object__find_program_by_title(obj_trace, "fentry/new_get_constant");
+	if (CHECK(!prog_trace, "find_prog", "prog %s not found\n", "fentry/new_get_constant"))
+		goto close_prog;
+
+	link_trace = bpf_program__attach_trace(prog_trace);
+	if (CHECK(IS_ERR(link_trace), "attach_trace", "failed to link\n"))
+		goto close_prog;
+}
+
 	data_map = bpf_object__find_map_by_name(obj, "fexit_bp.bss");
 	if (CHECK(!data_map, "find_data_map", "data map not found\n"))
 		goto close_prog;
@@ -88,6 +128,7 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
 	free(result);
 }
 
+#if 0
 static void test_target_no_callees(void)
 {
 	const char *prog_name[] = {
@@ -112,6 +153,7 @@ static void test_target_yes_callees(void)
 				  ARRAY_SIZE(prog_name),
 				  prog_name, true);
 }
+#endif
 
 static void test_func_replace(void)
 {
@@ -130,6 +172,7 @@ static void test_func_replace(void)
 				  prog_name, true);
 }
 
+#if 0
 static void test_func_replace_verify(void)
 {
 	const char *prog_name[] = {
@@ -140,11 +183,9 @@ static void test_func_replace_verify(void)
 				  ARRAY_SIZE(prog_name),
 				  prog_name, false);
 }
+#endif
 
 void test_fexit_bpf2bpf(void)
 {
-	test_target_no_callees();
-	test_target_yes_callees();
 	test_func_replace();
-	test_func_replace_verify();
 }
diff --git a/tools/testing/selftests/bpf/progs/trace_ext.c b/tools/testing/selftests/bpf/progs/trace_ext.c
new file mode 100644
index 000000000000..c7eaf30bf5ba
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/trace_ext.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <stddef.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+
+SEC("fentry/new_get_constant")
+int BPF_PROG(test_fentry_new_get_constant, long val)
+{
+        bpf_printk("ENTRY val %ld", val);
+	return 0;
+}
+
+SEC("fexit/new_get_constant")
+int BPF_PROG(test_fexit_new_get_constant, long val, int ret)
+{
+        bpf_printk("EXIT  val %ld, ret %d", val, ret);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";

Re: fentry/fexit attach to EXT type XDP program does not work

[Eelco Chaudron]

On 26 Jul 2020, at 14:24, Jiri Olsa wrote:

> On Tue, Jun 09, 2020 at 10:52:34AM +0200, Eelco Chaudron wrote:
>
> SNIP
>
>>>>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>>>>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>>>>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>>>>    #91 xdp_fentry_ext:FAIL
>>>>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>>>
>>>> Any idea what could be the case here? The same fentry/fexit attach
>>>> code works fine in the xdp_bpf2bpf.c tests case.
>>
>> <SNIP>
>>>
>>> I think this is not supported now. That is, you cannot attach a 
>>> fentry
>>> trace
>>> to the EXT program. The current implementation for fentry program 
>>> simply
>>> trying to find and match the signature of freplace program which by
>>> default
>>> is a pointer to void.
>>>
>>> It is doable in that in kernel we could recognize to-be-attached 
>>> program
>>> is
>>> a freplace and further trace down to find the real signature. The
>>> related
>>> kernel function is btf_get_prog_ctx_type(). You can try to implement 
>>> by
>>> yourself
>>> or I can have a patch for this once bpf-next opens.
>>
>> I’m not familiar with this area of the code, so if you could 
>> prepare a patch
>> that would nice.
>> You can also send it to me before bpf-next opens and I can verify it, 
>> and
>> clean up the self-test so it can be included as well.
>>
>
> hi,
> it seems that you cannot exten fentry/fexit programs,
> but it's possible to attach fentry/fexit to ext program.
>
>    /* Program extensions can extend all program types
>     * 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.
>     */
>
> I changed fexit_bpf2bpf.c test just to do a quick check
> and it seems to work:

Hi Jiri this is exactly what I’m trying, however when you do this 
where the first argument is a pointer to some context data which you are 
accessing it’s failing in the verifier.
This is a link to the original email, which has a test patch attached 
that will show the failure when trying to load/attach the fentry 
function and access the context:

https://lore.kernel.org/bpf/159162546868.10791.12432342618156330247.stgit@ebuild/

//Eelco

>
>   # echo > /sys/kernel/debug/tracing/trace
>   #  ./test_progs -t fexit_bpf2bpf
>   #25 fexit_bpf2bpf:OK
>   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>   # cat /sys/kernel/debug/tracing/trace | tail -2
>            <...>-75365 [012] d... 313932.416780: bpf_trace_printk: 
> ENTRY val 123
>            <...>-75365 [012] d... 313932.416784: bpf_trace_printk: 
> EXIT  val 123, ret 1
>
< SNIP>

Re: fentry/fexit attach to EXT type XDP program does not work

[Jiri Olsa]

On Mon, Jul 27, 2020 at 09:59:14AM +0200, Eelco Chaudron wrote:
> 
> 
> On 26 Jul 2020, at 14:24, Jiri Olsa wrote:
> 
> > On Tue, Jun 09, 2020 at 10:52:34AM +0200, Eelco Chaudron wrote:
> > 
> > SNIP
> > 
> > > > >    libbpf: failed to load object 'test_xdp_bpf2bpf'
> > > > >    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
> > > > >    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
> > > > >    #91 xdp_fentry_ext:FAIL
> > > > >    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
> > > > > 
> > > > > Any idea what could be the case here? The same fentry/fexit attach
> > > > > code works fine in the xdp_bpf2bpf.c tests case.
> > > 
> > > <SNIP>
> > > > 
> > > > I think this is not supported now. That is, you cannot attach a
> > > > fentry
> > > > trace
> > > > to the EXT program. The current implementation for fentry
> > > > program simply
> > > > trying to find and match the signature of freplace program which by
> > > > default
> > > > is a pointer to void.
> > > > 
> > > > It is doable in that in kernel we could recognize to-be-attached
> > > > program
> > > > is
> > > > a freplace and further trace down to find the real signature. The
> > > > related
> > > > kernel function is btf_get_prog_ctx_type(). You can try to
> > > > implement by
> > > > yourself
> > > > or I can have a patch for this once bpf-next opens.
> > > 
> > > I’m not familiar with this area of the code, so if you could prepare
> > > a patch
> > > that would nice.
> > > You can also send it to me before bpf-next opens and I can verify
> > > it, and
> > > clean up the self-test so it can be included as well.
> > > 
> > 
> > hi,
> > it seems that you cannot exten fentry/fexit programs,
> > but it's possible to attach fentry/fexit to ext program.
> > 
> >    /* Program extensions can extend all program types
> >     * 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.
> >     */
> > 
> > I changed fexit_bpf2bpf.c test just to do a quick check
> > and it seems to work:
> 
> Hi Jiri this is exactly what I’m trying, however when you do this where the
> first argument is a pointer to some context data which you are accessing
> it’s failing in the verifier.
> This is a link to the original email, which has a test patch attached that
> will show the failure when trying to load/attach the fentry function and
> access the context:
> 
> https://lore.kernel.org/bpf/159162546868.10791.12432342618156330247.stgit@ebuild/

ok, I tried to trace ext program with __sk_buff argument and I can see
the issue as well.. can't acess the skb argument

patch below fixes it for me, I can access the skb pointer and its data
via probe read, like:

	SEC("fexit/new_get_skb_ifindex")
	int BPF_PROG(fexit_new_get_skb_ifindex, int val, struct __sk_buff *skb, int var, int ret)
	{
		__u32 data;
		int err;

		bpf_printk("EXIT skb %p", skb);
		bpf_probe_read_kernel(&data, sizeof(data), &skb->data);
		bpf_printk("EXIT ret %d, data %p", err, data);
		return 0;
	}

I think it should fix the xdp_md acess as well

jirka


---
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ee36b7f60936..2145329f7b1b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3828,6 +3828,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	}
 
 	info->reg_type = PTR_TO_BTF_ID;
+
+	if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
+		tgt_prog = tgt_prog->aux->linked_prog;
+
 	if (tgt_prog) {
 		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
 		if (ret > 0) {

Re: fentry/fexit attach to EXT type XDP program does not work

[Eelco Chaudron]

On 27 Jul 2020, at 16:53, Jiri Olsa wrote:

> On Mon, Jul 27, 2020 at 09:59:14AM +0200, Eelco Chaudron wrote:
>>
>>
>> On 26 Jul 2020, at 14:24, Jiri Olsa wrote:
>>
>>> On Tue, Jun 09, 2020 at 10:52:34AM +0200, Eelco Chaudron wrote:
>>>
>>> SNIP
>>>
>>>>>>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>>>>>>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>>>>>>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>>>>>>    #91 xdp_fentry_ext:FAIL
>>>>>>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>>>>>
>>>>>> Any idea what could be the case here? The same fentry/fexit 
>>>>>> attach
>>>>>> code works fine in the xdp_bpf2bpf.c tests case.
>>>>
>>>> <SNIP>
>>>>>
>>>>> I think this is not supported now. That is, you cannot attach a
>>>>> fentry
>>>>> trace
>>>>> to the EXT program. The current implementation for fentry
>>>>> program simply
>>>>> trying to find and match the signature of freplace program which 
>>>>> by
>>>>> default
>>>>> is a pointer to void.
>>>>>
>>>>> It is doable in that in kernel we could recognize to-be-attached
>>>>> program
>>>>> is
>>>>> a freplace and further trace down to find the real signature. The
>>>>> related
>>>>> kernel function is btf_get_prog_ctx_type(). You can try to
>>>>> implement by
>>>>> yourself
>>>>> or I can have a patch for this once bpf-next opens.
>>>>
>>>> I’m not familiar with this area of the code, so if you could 
>>>> prepare
>>>> a patch
>>>> that would nice.
>>>> You can also send it to me before bpf-next opens and I can verify
>>>> it, and
>>>> clean up the self-test so it can be included as well.
>>>>
>>>
>>> hi,
>>> it seems that you cannot exten fentry/fexit programs,
>>> but it's possible to attach fentry/fexit to ext program.
>>>
>>>    /* Program extensions can extend all program types
>>>     * 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.
>>>     */
>>>
>>> I changed fexit_bpf2bpf.c test just to do a quick check
>>> and it seems to work:
>>
>> Hi Jiri this is exactly what I’m trying, however when you do this 
>> where the
>> first argument is a pointer to some context data which you are 
>> accessing
>> it’s failing in the verifier.
>> This is a link to the original email, which has a test patch attached 
>> that
>> will show the failure when trying to load/attach the fentry function 
>> and
>> access the context:
>>
>> https://lore.kernel.org/bpf/159162546868.10791.12432342618156330247.stgit@ebuild/
>
> ok, I tried to trace ext program with __sk_buff argument and I can see
> the issue as well.. can't acess the skb argument
>
> patch below fixes it for me, I can access the skb pointer and its data
> via probe read, like:
>
> 	SEC("fexit/new_get_skb_ifindex")
> 	int BPF_PROG(fexit_new_get_skb_ifindex, int val, struct __sk_buff 
> *skb, int var, int ret)
> 	{
> 		__u32 data;
> 		int err;
>
> 		bpf_printk("EXIT skb %p", skb);
> 		bpf_probe_read_kernel(&data, sizeof(data), &skb->data);
> 		bpf_printk("EXIT ret %d, data %p", err, data);
> 		return 0;
> 	}
>
> I think it should fix the xdp_md acess as well

Excellent patch ;) It works with xdp_md as well, and even better it does 
not require the bpf_probe_read_kernel(), so the test_xdp_bpf2bpf.c code 
just works.

Are you planning to send the patch upstream?

> jirka
>
>
> ---
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ee36b7f60936..2145329f7b1b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3828,6 +3828,10 @@ bool btf_ctx_access(int off, int size, enum 
> bpf_access_type type,
>  	}
>
>  	info->reg_type = PTR_TO_BTF_ID;
> +
> +	if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
> +		tgt_prog = tgt_prog->aux->linked_prog;
> +
>  	if (tgt_prog) {
>  		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>  		if (ret > 0) {

Re: fentry/fexit attach to EXT type XDP program does not work

[Jiri Olsa]

On Wed, Jul 29, 2020 at 08:23:56AM +0200, Eelco Chaudron wrote:

SNIP

> > > > > a patch
> > > > > that would nice.
> > > > > You can also send it to me before bpf-next opens and I can verify
> > > > > it, and
> > > > > clean up the self-test so it can be included as well.
> > > > > 
> > > > 
> > > > hi,
> > > > it seems that you cannot exten fentry/fexit programs,
> > > > but it's possible to attach fentry/fexit to ext program.
> > > > 
> > > >    /* Program extensions can extend all program types
> > > >     * 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.
> > > >     */
> > > > 
> > > > I changed fexit_bpf2bpf.c test just to do a quick check
> > > > and it seems to work:
> > > 
> > > Hi Jiri this is exactly what I’m trying, however when you do this
> > > where the
> > > first argument is a pointer to some context data which you are
> > > accessing
> > > it’s failing in the verifier.
> > > This is a link to the original email, which has a test patch
> > > attached that
> > > will show the failure when trying to load/attach the fentry function
> > > and
> > > access the context:
> > > 
> > > https://lore.kernel.org/bpf/159162546868.10791.12432342618156330247.stgit@ebuild/
> > 
> > ok, I tried to trace ext program with __sk_buff argument and I can see
> > the issue as well.. can't acess the skb argument
> > 
> > patch below fixes it for me, I can access the skb pointer and its data
> > via probe read, like:
> > 
> > 	SEC("fexit/new_get_skb_ifindex")
> > 	int BPF_PROG(fexit_new_get_skb_ifindex, int val, struct __sk_buff *skb,
> > int var, int ret)
> > 	{
> > 		__u32 data;
> > 		int err;
> > 
> > 		bpf_printk("EXIT skb %p", skb);
> > 		bpf_probe_read_kernel(&data, sizeof(data), &skb->data);
> > 		bpf_printk("EXIT ret %d, data %p", err, data);
> > 		return 0;
> > 	}
> > 
> > I think it should fix the xdp_md acess as well
> 
> Excellent patch ;) It works with xdp_md as well, and even better it does not
> require the bpf_probe_read_kernel(), so the test_xdp_bpf2bpf.c code just
> works.

great ;-) will check on xdp_md

> 
> Are you planning to send the patch upstream?

yep, I'll add some test for that and send it

thanks,
jirka