| Message ID | 20200909151115.1559418-1-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Fix context type resolving for extension programs | expand |
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
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
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
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.
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
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) {
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(+)