* [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
@ 2020-09-09 15:11 Jiri Olsa
2020-09-09 15:11 ` [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jiri Olsa @ 2020-09-09 15:11 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Eelco Chaudron, netdev, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen
Eelco reported we can't properly access arguments if the tracing
program is attached to extension program.
Having following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
It's not possible to access skb argument in the fentry program,
with following error from verifier:
; int BPF_PROG(fentry, struct sk_buff *skb)
0: (79) r1 = *(u64 *)(r1 +0)
invalid bpf_context access off=0 size=8
The problem is that btf_ctx_access gets the context type for the
traced program, which is in this case the extension.
But when we trace extension program, we want to get the context
type of the program that the extension is attached to, so we can
access the argument properly in the trace program.
Reported-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/bpf/btf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f9ac6935ab3c..37ad01c32e5a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
}
info->reg_type = PTR_TO_BTF_ID;
+
+ /* When we trace extension program, we want to get the context
+ * type of the program that the extension is attached to, so
+ * we can access the argument properly in the trace program.
+ */
+ 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) {
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-09 15:11 [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Jiri Olsa
@ 2020-09-09 15:11 ` Jiri Olsa
2020-09-10 22:34 ` Andrii Nakryiko
2020-09-09 15:54 ` [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
2020-09-10 18:00 ` Alexei Starovoitov
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2020-09-09 15:11 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer, Eelco Chaudron,
Toke Høiland-Jørgensen
Adding test that setup following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
The test verifies that the tracing program can
dereference skb argument properly.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/trace_ext.c | 93 +++++++++++++++++++
.../selftests/bpf/progs/test_trace_ext.c | 18 ++++
.../bpf/progs/test_trace_ext_tracing.c | 25 +++++
3 files changed, 136 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
new file mode 100644
index 000000000000..1089dafb4653
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#include "test_trace_ext.skel.h"
+#include "test_trace_ext_tracing.skel.h"
+
+static __u32 duration;
+
+void test_trace_ext(void)
+{
+ struct test_trace_ext_tracing *skel_trace = NULL;
+ struct test_trace_ext_tracing__bss *bss_trace;
+ const char *file = "./test_pkt_md_access.o";
+ struct test_trace_ext *skel_ext = NULL;
+ struct test_trace_ext__bss *bss_ext;
+ int err, prog_fd, ext_fd;
+ struct bpf_object *obj;
+ char buf[100];
+ __u32 retval;
+ __u64 len;
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+ if (CHECK_FAIL(err))
+ return;
+
+ DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+ .attach_prog_fd = prog_fd,
+ );
+
+ skel_ext = test_trace_ext__open_opts(&opts);
+ if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
+ goto cleanup;
+
+ err = test_trace_ext__load(skel_ext);
+ if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
+ libbpf_strerror(err, buf, sizeof(buf));
+ fprintf(stderr, "%s\n", buf);
+ goto cleanup;
+ }
+
+ err = test_trace_ext__attach(skel_ext);
+ if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
+ goto cleanup;
+
+ ext_fd = bpf_program__fd(skel_ext->progs.test_pkt_md_access_new);
+
+ DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts_trace,
+ .attach_prog_fd = ext_fd,
+ );
+
+ skel_trace = test_trace_ext_tracing__open_opts(&opts_trace);
+ if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
+ goto cleanup;
+
+ err = test_trace_ext_tracing__load(skel_trace);
+ if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
+ libbpf_strerror(err, buf, sizeof(buf));
+ fprintf(stderr, "%s\n", buf);
+ goto cleanup;
+ }
+
+ err = test_trace_ext_tracing__attach(skel_trace);
+ if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
+ goto cleanup;
+
+ err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+ NULL, NULL, &retval, &duration);
+ CHECK(err || retval, "",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration);
+
+ bss_ext = skel_ext->bss;
+ bss_trace = skel_trace->bss;
+
+ len = bss_ext->ext_called;
+
+ CHECK(bss_ext->ext_called == 0,
+ "check", "failed to trigger freplace/test_pkt_md_access\n");
+ CHECK(bss_trace->fentry_called != len,
+ "check", "failed to trigger fentry/test_pkt_md_access_new\n");
+ CHECK(bss_trace->fexit_called != len,
+ "check", "failed to trigger fexit/test_pkt_md_access_new\n");
+
+cleanup:
+ test_trace_ext__destroy(skel_ext);
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
new file mode 100644
index 000000000000..a6318f6b52ee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Facebook
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+volatile __u64 ext_called = 0;
+
+SEC("freplace/test_pkt_md_access")
+int test_pkt_md_access_new(struct __sk_buff *skb)
+{
+ ext_called = skb->len;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
new file mode 100644
index 000000000000..9e52a831446f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+volatile __u64 fentry_called = 0;
+
+SEC("fentry/test_pkt_md_access_new")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+ fentry_called = skb->len;
+ return 0;
+}
+
+volatile __u64 fexit_called = 0;
+
+SEC("fexit/test_pkt_md_access_new")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+ fexit_called = skb->len;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
2020-09-09 15:11 [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Jiri Olsa
2020-09-09 15:11 ` [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace Jiri Olsa
@ 2020-09-09 15:54 ` Toke Høiland-Jørgensen
2020-09-09 18:58 ` Jiri Olsa
2020-09-10 18:00 ` Alexei Starovoitov
2 siblings, 1 reply; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-09 15:54 UTC (permalink / raw)
To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann
Cc: Eelco Chaudron, netdev, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer
Jiri Olsa <jolsa@kernel.org> writes:
> Eelco reported we can't properly access arguments if the tracing
> program is attached to extension program.
>
> Having following program:
>
> SEC("classifier/test_pkt_md_access")
> int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
> SEC("freplace/test_pkt_md_access")
> int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
> SEC("fentry/test_pkt_md_access_new")
> int BPF_PROG(fentry, struct sk_buff *skb)
>
> It's not possible to access skb argument in the fentry program,
> with following error from verifier:
>
> ; int BPF_PROG(fentry, struct sk_buff *skb)
> 0: (79) r1 = *(u64 *)(r1 +0)
> invalid bpf_context access off=0 size=8
>
> The problem is that btf_ctx_access gets the context type for the
> traced program, which is in this case the extension.
>
> But when we trace extension program, we want to get the context
> type of the program that the extension is attached to, so we can
> access the argument properly in the trace program.
>
> Reported-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/bpf/btf.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f9ac6935ab3c..37ad01c32e5a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> }
>
> info->reg_type = PTR_TO_BTF_ID;
> +
> + /* When we trace extension program, we want to get the context
> + * type of the program that the extension is attached to, so
> + * we can access the argument properly in the trace program.
> + */
> + if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
> + tgt_prog = tgt_prog->aux->linked_prog;
> +
In the discussion about multi-attach for freplace we kinda concluded[0]
that this linked_prog pointer was going away after attach. I have this
basically working, but need to test a bit more before posting it (see
[1] for current status).
But with this I guess we'll need to either do something different? Maybe
go chase down the target via the bpf_link or something?
-Toke
[0] https://lore.kernel.org/bpf/20200722002918.574pruibvlxfblyq@ast-mbp.dhcp.thefacebook.com/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=bpf-freplace-multi-attach-alt-03
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
2020-09-09 15:54 ` [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
@ 2020-09-09 18:58 ` Jiri Olsa
2020-09-10 9:49 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2020-09-09 18:58 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Eelco Chaudron,
netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer
On Wed, Sep 09, 2020 at 05:54:58PM +0200, Toke Høiland-Jørgensen wrote:
> Jiri Olsa <jolsa@kernel.org> writes:
>
> > Eelco reported we can't properly access arguments if the tracing
> > program is attached to extension program.
> >
> > Having following program:
> >
> > SEC("classifier/test_pkt_md_access")
> > int test_pkt_md_access(struct __sk_buff *skb)
> >
> > with its extension:
> >
> > SEC("freplace/test_pkt_md_access")
> > int test_pkt_md_access_new(struct __sk_buff *skb)
> >
> > and tracing that extension with:
> >
> > SEC("fentry/test_pkt_md_access_new")
> > int BPF_PROG(fentry, struct sk_buff *skb)
> >
> > It's not possible to access skb argument in the fentry program,
> > with following error from verifier:
> >
> > ; int BPF_PROG(fentry, struct sk_buff *skb)
> > 0: (79) r1 = *(u64 *)(r1 +0)
> > invalid bpf_context access off=0 size=8
> >
> > The problem is that btf_ctx_access gets the context type for the
> > traced program, which is in this case the extension.
> >
> > But when we trace extension program, we want to get the context
> > type of the program that the extension is attached to, so we can
> > access the argument properly in the trace program.
> >
> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/bpf/btf.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index f9ac6935ab3c..37ad01c32e5a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > }
> >
> > info->reg_type = PTR_TO_BTF_ID;
> > +
> > + /* When we trace extension program, we want to get the context
> > + * type of the program that the extension is attached to, so
> > + * we can access the argument properly in the trace program.
> > + */
> > + if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
> > + tgt_prog = tgt_prog->aux->linked_prog;
> > +
>
> In the discussion about multi-attach for freplace we kinda concluded[0]
> that this linked_prog pointer was going away after attach. I have this
> basically working, but need to test a bit more before posting it (see
> [1] for current status).
ok, feel free to use the test case from patch 2 ;-)
>
> But with this I guess we'll need to either do something different? Maybe
> go chase down the target via the bpf_link or something?
I'll check, could you please CC me on your next post?
thanks,
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
2020-09-09 18:58 ` Jiri Olsa
@ 2020-09-10 9:49 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-10 9:49 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Eelco Chaudron,
netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer
Jiri Olsa <jolsa@redhat.com> writes:
> On Wed, Sep 09, 2020 at 05:54:58PM +0200, Toke Høiland-Jørgensen wrote:
>> Jiri Olsa <jolsa@kernel.org> writes:
>>
>> > Eelco reported we can't properly access arguments if the tracing
>> > program is attached to extension program.
>> >
>> > Having following program:
>> >
>> > SEC("classifier/test_pkt_md_access")
>> > int test_pkt_md_access(struct __sk_buff *skb)
>> >
>> > with its extension:
>> >
>> > SEC("freplace/test_pkt_md_access")
>> > int test_pkt_md_access_new(struct __sk_buff *skb)
>> >
>> > and tracing that extension with:
>> >
>> > SEC("fentry/test_pkt_md_access_new")
>> > int BPF_PROG(fentry, struct sk_buff *skb)
>> >
>> > It's not possible to access skb argument in the fentry program,
>> > with following error from verifier:
>> >
>> > ; int BPF_PROG(fentry, struct sk_buff *skb)
>> > 0: (79) r1 = *(u64 *)(r1 +0)
>> > invalid bpf_context access off=0 size=8
>> >
>> > The problem is that btf_ctx_access gets the context type for the
>> > traced program, which is in this case the extension.
>> >
>> > But when we trace extension program, we want to get the context
>> > type of the program that the extension is attached to, so we can
>> > access the argument properly in the trace program.
>> >
>> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> > ---
>> > kernel/bpf/btf.c | 8 ++++++++
>> > 1 file changed, 8 insertions(+)
>> >
>> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> > index f9ac6935ab3c..37ad01c32e5a 100644
>> > --- a/kernel/bpf/btf.c
>> > +++ b/kernel/bpf/btf.c
>> > @@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> > }
>> >
>> > info->reg_type = PTR_TO_BTF_ID;
>> > +
>> > + /* When we trace extension program, we want to get the context
>> > + * type of the program that the extension is attached to, so
>> > + * we can access the argument properly in the trace program.
>> > + */
>> > + if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
>> > + tgt_prog = tgt_prog->aux->linked_prog;
>> > +
>>
>> In the discussion about multi-attach for freplace we kinda concluded[0]
>> that this linked_prog pointer was going away after attach. I have this
>> basically working, but need to test a bit more before posting it (see
>> [1] for current status).
>
> ok, feel free to use the test case from patch 2 ;-)
>
>>
>> But with this I guess we'll need to either do something different? Maybe
>> go chase down the target via the bpf_link or something?
>
> I'll check, could you please CC me on your next post?
Sure, will do!
-Toke
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
2020-09-09 15:11 [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Jiri Olsa
2020-09-09 15:11 ` [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace Jiri Olsa
2020-09-09 15:54 ` [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
@ 2020-09-10 18:00 ` Alexei Starovoitov
2020-09-11 10:52 ` Toke Høiland-Jørgensen
2 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2020-09-10 18:00 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Eelco Chaudron,
Network Development, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen
On Wed, Sep 9, 2020 at 8:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Eelco reported we can't properly access arguments if the tracing
> program is attached to extension program.
>
> Having following program:
>
> SEC("classifier/test_pkt_md_access")
> int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
> SEC("freplace/test_pkt_md_access")
> int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
> SEC("fentry/test_pkt_md_access_new")
> int BPF_PROG(fentry, struct sk_buff *skb)
>
> It's not possible to access skb argument in the fentry program,
> with following error from verifier:
>
> ; int BPF_PROG(fentry, struct sk_buff *skb)
> 0: (79) r1 = *(u64 *)(r1 +0)
> invalid bpf_context access off=0 size=8
>
> The problem is that btf_ctx_access gets the context type for the
> traced program, which is in this case the extension.
>
> But when we trace extension program, we want to get the context
> type of the program that the extension is attached to, so we can
> access the argument properly in the trace program.
>
> Reported-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/bpf/btf.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f9ac6935ab3c..37ad01c32e5a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> }
>
> info->reg_type = PTR_TO_BTF_ID;
> +
> + /* When we trace extension program, we want to get the context
> + * type of the program that the extension is attached to, so
> + * we can access the argument properly in the trace program.
> + */
> + 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);
I think it would be cleaner to move resolve_prog_type() from verifier.c
and use that helper function here.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-09 15:11 ` [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace Jiri Olsa
@ 2020-09-10 22:34 ` Andrii Nakryiko
2020-09-11 11:01 ` Toke Høiland-Jørgensen
2020-09-11 13:02 ` Jiri Olsa
0 siblings, 2 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2020-09-10 22:34 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Networking, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, Jesper Dangaard Brouer, Eelco Chaudron,
Toke Høiland-Jørgensen
On Wed, Sep 9, 2020 at 8:38 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that setup following program:
>
> SEC("classifier/test_pkt_md_access")
> int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
> SEC("freplace/test_pkt_md_access")
> int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
> SEC("fentry/test_pkt_md_access_new")
> int BPF_PROG(fentry, struct sk_buff *skb)
>
> The test verifies that the tracing program can
> dereference skb argument properly.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/trace_ext.c | 93 +++++++++++++++++++
> .../selftests/bpf/progs/test_trace_ext.c | 18 ++++
> .../bpf/progs/test_trace_ext_tracing.c | 25 +++++
> 3 files changed, 136 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
> new file mode 100644
> index 000000000000..1089dafb4653
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include <sys/stat.h>
> +#include <linux/sched.h>
> +#include <sys/syscall.h>
> +
> +#include "test_trace_ext.skel.h"
> +#include "test_trace_ext_tracing.skel.h"
> +
> +static __u32 duration;
> +
> +void test_trace_ext(void)
> +{
> + struct test_trace_ext_tracing *skel_trace = NULL;
> + struct test_trace_ext_tracing__bss *bss_trace;
> + const char *file = "./test_pkt_md_access.o";
> + struct test_trace_ext *skel_ext = NULL;
> + struct test_trace_ext__bss *bss_ext;
> + int err, prog_fd, ext_fd;
> + struct bpf_object *obj;
> + char buf[100];
> + __u32 retval;
> + __u64 len;
> +
> + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> + if (CHECK_FAIL(err))
> + return;
We should avoid using bpf_prog_load() for new code. Can you please
just skeleton instead? Or at least bpf_object__open_file()?
> +
> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> + .attach_prog_fd = prog_fd,
> + );
DECLARE_LIBBPF_OPTS does declare a variable, so should be together
with all the other variables above, otherwise some overly strict C89
mode compiler will start complaining. You can assign
`opts.attach_prog_fd = prog_fd;` outside of declaration. But I also
don't think you need this one. Having .attach_prog_fd in open_opts is
not great, because it's a per-program setting specified at bpf_object
level. Would bpf_program__set_attach_target() work here?
> +
> + skel_ext = test_trace_ext__open_opts(&opts);
> + if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
> + goto cleanup;
> +
> + err = test_trace_ext__load(skel_ext);
> + if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
> + libbpf_strerror(err, buf, sizeof(buf));
> + fprintf(stderr, "%s\n", buf);
> + goto cleanup;
> + }
> +
> + err = test_trace_ext__attach(skel_ext);
> + if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
> + goto cleanup;
> +
> + ext_fd = bpf_program__fd(skel_ext->progs.test_pkt_md_access_new);
> +
> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts_trace,
> + .attach_prog_fd = ext_fd,
> + );
> +
same
> + skel_trace = test_trace_ext_tracing__open_opts(&opts_trace);
> + if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
> + goto cleanup;
> +
> + err = test_trace_ext_tracing__load(skel_trace);
> + if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
> + libbpf_strerror(err, buf, sizeof(buf));
> + fprintf(stderr, "%s\n", buf);
> + goto cleanup;
> + }
> +
> + err = test_trace_ext_tracing__attach(skel_trace);
> + if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
> + goto cleanup;
> +
> + err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> + NULL, NULL, &retval, &duration);
> + CHECK(err || retval, "",
> + "err %d errno %d retval %d duration %d\n",
> + err, errno, retval, duration);
> +
> + bss_ext = skel_ext->bss;
> + bss_trace = skel_trace->bss;
> +
> + len = bss_ext->ext_called;
> +
> + CHECK(bss_ext->ext_called == 0,
> + "check", "failed to trigger freplace/test_pkt_md_access\n");
> + CHECK(bss_trace->fentry_called != len,
> + "check", "failed to trigger fentry/test_pkt_md_access_new\n");
> + CHECK(bss_trace->fexit_called != len,
> + "check", "failed to trigger fexit/test_pkt_md_access_new\n");
> +
> +cleanup:
> + test_trace_ext__destroy(skel_ext);
> + bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> new file mode 100644
> index 000000000000..a6318f6b52ee
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Facebook
> +#include <linux/bpf.h>
> +#include <stdbool.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
> +
> +volatile __u64 ext_called = 0;
nit: no need for volatile, global variables are not going anywhere;
same below in two places
> +
> +SEC("freplace/test_pkt_md_access")
> +int test_pkt_md_access_new(struct __sk_buff *skb)
> +{
> + ext_called = skb->len;
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
> new file mode 100644
> index 000000000000..9e52a831446f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +volatile __u64 fentry_called = 0;
> +
> +SEC("fentry/test_pkt_md_access_new")
> +int BPF_PROG(fentry, struct sk_buff *skb)
> +{
> + fentry_called = skb->len;
> + return 0;
> +}
> +
> +volatile __u64 fexit_called = 0;
> +
> +SEC("fexit/test_pkt_md_access_new")
> +int BPF_PROG(fexit, struct sk_buff *skb)
> +{
> + fexit_called = skb->len;
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs
2020-09-10 18:00 ` Alexei Starovoitov
@ 2020-09-11 10:52 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-11 10:52 UTC (permalink / raw)
To: Alexei Starovoitov, Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Eelco Chaudron,
Network Development, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Jesper Dangaard Brouer
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Wed, Sep 9, 2020 at 8:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
>>
>> Eelco reported we can't properly access arguments if the tracing
>> program is attached to extension program.
>>
>> Having following program:
>>
>> SEC("classifier/test_pkt_md_access")
>> int test_pkt_md_access(struct __sk_buff *skb)
>>
>> with its extension:
>>
>> SEC("freplace/test_pkt_md_access")
>> int test_pkt_md_access_new(struct __sk_buff *skb)
>>
>> and tracing that extension with:
>>
>> SEC("fentry/test_pkt_md_access_new")
>> int BPF_PROG(fentry, struct sk_buff *skb)
>>
>> It's not possible to access skb argument in the fentry program,
>> with following error from verifier:
>>
>> ; int BPF_PROG(fentry, struct sk_buff *skb)
>> 0: (79) r1 = *(u64 *)(r1 +0)
>> invalid bpf_context access off=0 size=8
>>
>> The problem is that btf_ctx_access gets the context type for the
>> traced program, which is in this case the extension.
>>
>> But when we trace extension program, we want to get the context
>> type of the program that the extension is attached to, so we can
>> access the argument properly in the trace program.
>>
>> Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>> kernel/bpf/btf.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index f9ac6935ab3c..37ad01c32e5a 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3859,6 +3859,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> }
>>
>> info->reg_type = PTR_TO_BTF_ID;
>> +
>> + /* When we trace extension program, we want to get the context
>> + * type of the program that the extension is attached to, so
>> + * we can access the argument properly in the trace program.
>> + */
>> + 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);
>
> I think it would be cleaner to move resolve_prog_type() from verifier.c
> and use that helper function here.
FYI, I've added a different version of this patch to my freplace
multi-attach series (since the approach here was incompatible with
that).
-Toke
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-10 22:34 ` Andrii Nakryiko
@ 2020-09-11 11:01 ` Toke Høiland-Jørgensen
2020-09-11 13:02 ` Jiri Olsa
1 sibling, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-11 11:01 UTC (permalink / raw)
To: Andrii Nakryiko, Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Networking, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, Jesper Dangaard Brouer, Eelco Chaudron
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 9, 2020 at 8:38 AM Jiri Olsa <jolsa@kernel.org> wrote:
>>
>> Adding test that setup following program:
>>
>> SEC("classifier/test_pkt_md_access")
>> int test_pkt_md_access(struct __sk_buff *skb)
>>
>> with its extension:
>>
>> SEC("freplace/test_pkt_md_access")
>> int test_pkt_md_access_new(struct __sk_buff *skb)
>>
>> and tracing that extension with:
>>
>> SEC("fentry/test_pkt_md_access_new")
>> int BPF_PROG(fentry, struct sk_buff *skb)
>>
>> The test verifies that the tracing program can
>> dereference skb argument properly.
>>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Just FYI, I included this same patch in my freplace series. I didn't
change anything in the version I just resent, but I'll work with Jiri
and get an updated version of this into the next version based on your
comments here... :)
-Toke
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-10 22:34 ` Andrii Nakryiko
2020-09-11 11:01 ` Toke Høiland-Jørgensen
@ 2020-09-11 13:02 ` Jiri Olsa
1 sibling, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2020-09-11 13:02 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Networking, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, Jesper Dangaard Brouer, Eelco Chaudron,
Toke Høiland-Jørgensen
On Thu, Sep 10, 2020 at 03:34:26PM -0700, Andrii Nakryiko wrote:
SNIP
> > +
> > +void test_trace_ext(void)
> > +{
> > + struct test_trace_ext_tracing *skel_trace = NULL;
> > + struct test_trace_ext_tracing__bss *bss_trace;
> > + const char *file = "./test_pkt_md_access.o";
> > + struct test_trace_ext *skel_ext = NULL;
> > + struct test_trace_ext__bss *bss_ext;
> > + int err, prog_fd, ext_fd;
> > + struct bpf_object *obj;
> > + char buf[100];
> > + __u32 retval;
> > + __u64 len;
> > +
> > + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > + if (CHECK_FAIL(err))
> > + return;
>
> We should avoid using bpf_prog_load() for new code. Can you please
> just skeleton instead? Or at least bpf_object__open_file()?
ok
>
> > +
> > + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > + .attach_prog_fd = prog_fd,
> > + );
>
> DECLARE_LIBBPF_OPTS does declare a variable, so should be together
> with all the other variables above, otherwise some overly strict C89
> mode compiler will start complaining. You can assign
> `opts.attach_prog_fd = prog_fd;` outside of declaration. But I also
> don't think you need this one. Having .attach_prog_fd in open_opts is
> not great, because it's a per-program setting specified at bpf_object
> level. Would bpf_program__set_attach_target() work here?
right, I'll try it, it should be enough
SNIP
> > +
> > +cleanup:
> > + test_trace_ext__destroy(skel_ext);
> > + bpf_object__close(obj);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > new file mode 100644
> > index 000000000000..a6318f6b52ee
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 Facebook
> > +#include <linux/bpf.h>
> > +#include <stdbool.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_endian.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +volatile __u64 ext_called = 0;
>
> nit: no need for volatile, global variables are not going anywhere;
> same below in two places
ok, thanks
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-11 14:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09 15:11 [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Jiri Olsa
2020-09-09 15:11 ` [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace Jiri Olsa
2020-09-10 22:34 ` Andrii Nakryiko
2020-09-11 11:01 ` Toke Høiland-Jørgensen
2020-09-11 13:02 ` Jiri Olsa
2020-09-09 15:54 ` [PATCH bpf-next 1/2] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
2020-09-09 18:58 ` Jiri Olsa
2020-09-10 9:49 ` Toke Høiland-Jørgensen
2020-09-10 18:00 ` Alexei Starovoitov
2020-09-11 10:52 ` Toke Høiland-Jørgensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.