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

Message ID 159162546868.10791.12432342618156330247.stgit@ebuild
State New
Headers show
Series
  • fentry/fexit attach to EXT type XDP program does not work
Related show

Commit Message

Eelco Chaudron June 8, 2020, 2:11 p.m.
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

Comments

Jiri Olsa July 26, 2020, 12:24 p.m. | #1
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";
Eelco Chaudron July 27, 2020, 7:59 a.m. | #2
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>
Jiri Olsa July 27, 2020, 2:53 p.m. | #3
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) {
Eelco Chaudron July 29, 2020, 6:23 a.m. | #4
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) {
Jiri Olsa July 29, 2020, 8:09 a.m. | #5
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

Patch

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