| Message ID | 20191010041503.2526303-6-ast@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: revolutionize bpf tracing | expand |
On Wed, Oct 9, 2019 at 9:17 PM Alexei Starovoitov <ast@kernel.org> wrote: > > For raw tracepoint program types libbpf will try to find > btf_id of raw tracepoint in vmlinux's BTF. > It's a responsiblity of bpf program author to annotate the program > with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint. > If "name" is indeed a valid raw tracepoint then in-kernel BTF > will have "btf_trace_##name" typedef that points to function > prototype of that raw tracepoint. BTF description captures > exact argument the kernel C code is passing into raw tracepoint. > The kernel verifier will check the types while loading bpf program. > > libbpf keeps BTF type id in expected_attach_type, but since > kernel ignores this attribute for tracing programs copy it > into attach_btf_id attribute before loading. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/lib/bpf/bpf.c | 3 +++ > tools/lib/bpf/libbpf.c | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+) > [...]
On Wed, Oct 9, 2019 at 9:17 PM Alexei Starovoitov <ast@kernel.org> wrote: > > For raw tracepoint program types libbpf will try to find > btf_id of raw tracepoint in vmlinux's BTF. > It's a responsiblity of bpf program author to annotate the program > with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint. > If "name" is indeed a valid raw tracepoint then in-kernel BTF > will have "btf_trace_##name" typedef that points to function > prototype of that raw tracepoint. BTF description captures > exact argument the kernel C code is passing into raw tracepoint. > The kernel verifier will check the types while loading bpf program. > > libbpf keeps BTF type id in expected_attach_type, but since > kernel ignores this attribute for tracing programs copy it > into attach_btf_id attribute before loading. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > tools/lib/bpf/bpf.c | 3 +++ > tools/lib/bpf/libbpf.c | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index cbb933532981..79046067720f 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -228,6 +228,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, > memset(&attr, 0, sizeof(attr)); > attr.prog_type = load_attr->prog_type; > attr.expected_attach_type = load_attr->expected_attach_type; > + if (attr.prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) > + /* expected_attach_type is ignored for tracing progs */ > + attr.attach_btf_id = attr.expected_attach_type; > attr.insn_cnt = (__u32)load_attr->insns_cnt; > attr.insns = ptr_to_u64(load_attr->insns); > attr.license = ptr_to_u64(load_attr->license); > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index a02cdedc4e3f..8bf30a67428c 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4586,6 +4586,23 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, > continue; > *prog_type = section_names[i].prog_type; > *expected_attach_type = section_names[i].expected_attach_type; > + if (*prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { > + struct btf *btf = bpf_core_find_kernel_btf(); > + char raw_tp_btf_name[128] = "btf_trace_"; > + char *dst = raw_tp_btf_name + sizeof("btf_trace_") - 1; > + int ret; > + > + if (IS_ERR(btf)) > + /* lack of kernel BTF is not a failure */ > + return 0; > + /* prepend "btf_trace_" prefix per kernel convention */ > + strncat(dst, name + section_names[i].len, > + sizeof(raw_tp_btf_name) - (dst - raw_tp_btf_name)); > + ret = btf__find_by_name(btf, raw_tp_btf_name); > + if (ret > 0) > + *expected_attach_type = ret; Well, actually, I realized after I gave Acked-by, so not yet :) This needs kernel feature probe of whether kernel supports specifying attach_btf_id, otherwise on older kernels we'll stop successfully loading valid program. But even if kernel supports attach_btf_id, I think users still need to opt in into specifying attach_btf_id by libbpf. Think about existing raw_tp programs that are using bpf_probe_read() because they were not created with this kernel feature in mind. They will suddenly stop working without any of user's fault. One way to do this would be another section prefix for raw tracepoint w/ BTF. E.g., "raw_tp_typed" or something along those lines? I'll say it upfront, I don't think "raw_tp_btf" is great name, because we rely on BTF for a lot of other stuff already, so just adding _btf prefix doesn't mean specifically BTF type tracking in kernel ;) > + btf__free(btf); > + } > return 0; > } > pr_warning("failed to guess program type based on ELF section name '%s'\n", name); > -- > 2.23.0 >
On 10/11/19 11:07 AM, Andrii Nakryiko wrote: > On Wed, Oct 9, 2019 at 9:17 PM Alexei Starovoitov <ast@kernel.org> wrote: >> >> For raw tracepoint program types libbpf will try to find >> btf_id of raw tracepoint in vmlinux's BTF. >> It's a responsiblity of bpf program author to annotate the program >> with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint. >> If "name" is indeed a valid raw tracepoint then in-kernel BTF >> will have "btf_trace_##name" typedef that points to function >> prototype of that raw tracepoint. BTF description captures >> exact argument the kernel C code is passing into raw tracepoint. >> The kernel verifier will check the types while loading bpf program. >> >> libbpf keeps BTF type id in expected_attach_type, but since >> kernel ignores this attribute for tracing programs copy it >> into attach_btf_id attribute before loading. >> >> Signed-off-by: Alexei Starovoitov <ast@kernel.org> >> --- >> tools/lib/bpf/bpf.c | 3 +++ >> tools/lib/bpf/libbpf.c | 17 +++++++++++++++++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> index cbb933532981..79046067720f 100644 >> --- a/tools/lib/bpf/bpf.c >> +++ b/tools/lib/bpf/bpf.c >> @@ -228,6 +228,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, >> memset(&attr, 0, sizeof(attr)); >> attr.prog_type = load_attr->prog_type; >> attr.expected_attach_type = load_attr->expected_attach_type; >> + if (attr.prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) >> + /* expected_attach_type is ignored for tracing progs */ >> + attr.attach_btf_id = attr.expected_attach_type; >> attr.insn_cnt = (__u32)load_attr->insns_cnt; >> attr.insns = ptr_to_u64(load_attr->insns); >> attr.license = ptr_to_u64(load_attr->license); >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index a02cdedc4e3f..8bf30a67428c 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -4586,6 +4586,23 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, >> continue; >> *prog_type = section_names[i].prog_type; >> *expected_attach_type = section_names[i].expected_attach_type; >> + if (*prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { >> + struct btf *btf = bpf_core_find_kernel_btf(); >> + char raw_tp_btf_name[128] = "btf_trace_"; >> + char *dst = raw_tp_btf_name + sizeof("btf_trace_") - 1; >> + int ret; >> + >> + if (IS_ERR(btf)) >> + /* lack of kernel BTF is not a failure */ >> + return 0; >> + /* prepend "btf_trace_" prefix per kernel convention */ >> + strncat(dst, name + section_names[i].len, >> + sizeof(raw_tp_btf_name) - (dst - raw_tp_btf_name)); >> + ret = btf__find_by_name(btf, raw_tp_btf_name); >> + if (ret > 0) >> + *expected_attach_type = ret; > > Well, actually, I realized after I gave Acked-by, so not yet :) > > This needs kernel feature probe of whether kernel supports specifying > attach_btf_id, otherwise on older kernels we'll stop successfully > loading valid program. The code above won't find anything on older kernels. The patch 1 of the series has to be there for proper btf to be generated by pahole. Before that happens expected_attach_type will stay zero and corresponding copy in attach_btf_id will be zero as well. I see no issues being compatible with older kernels. > But even if kernel supports attach_btf_id, I think users still need to > opt in into specifying attach_btf_id by libbpf. Think about existing > raw_tp programs that are using bpf_probe_read() because they were not > created with this kernel feature in mind. They will suddenly stop > working without any of user's fault. This one is excellent catch. loop1.c should have caught it, since it has SEC("raw_tracepoint/kfree_skb") { int nested_loops(volatile struct pt_regs* ctx) .. = PT_REGS_RC(ctx); and verifier would have rejected it. But the way the test is written it's not using libbpf's autodetect of program type, so everything is passing.
On 10/11/19 5:40 PM, Alexei Starovoitov wrote: >> But even if kernel supports attach_btf_id, I think users still need to >> opt in into specifying attach_btf_id by libbpf. Think about existing >> raw_tp programs that are using bpf_probe_read() because they were not >> created with this kernel feature in mind. They will suddenly stop >> working without any of user's fault. > > This one is excellent catch. > loop1.c should have caught it, since it has > SEC("raw_tracepoint/kfree_skb") > { > int nested_loops(volatile struct pt_regs* ctx) > .. = PT_REGS_RC(ctx); > > and verifier would have rejected it. > But the way the test is written it's not using libbpf's autodetect > of program type, so everything is passing. With: diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c index 1c01ee2600a9..e27156dce10d 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c @@ -67,7 +67,7 @@ void test_bpf_verif_scale(void) */ { "pyperf600_nounroll.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, - { "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, + { "loop1.o", BPF_PROG_TYPE_UNSPEC}, { "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, libbpf prog auto-detection kicks in and ... # ./test_progs -n 3/10 libbpf: load bpf program failed: Permission denied libbpf: -- BEGIN DUMP LOG --- libbpf: raw_tp 'kfree_skb' doesn't have 10-th argument invalid bpf_context access off=80 size=8 Good :) The verifier is doing its job.
On Fri, Oct 11, 2019 at 6:29 PM Alexei Starovoitov <ast@fb.com> wrote: > > On 10/11/19 5:40 PM, Alexei Starovoitov wrote: > >> But even if kernel supports attach_btf_id, I think users still need to > >> opt in into specifying attach_btf_id by libbpf. Think about existing > >> raw_tp programs that are using bpf_probe_read() because they were not > >> created with this kernel feature in mind. They will suddenly stop > >> working without any of user's fault. > > > > This one is excellent catch. > > loop1.c should have caught it, since it has > > SEC("raw_tracepoint/kfree_skb") > > { > > int nested_loops(volatile struct pt_regs* ctx) > > .. = PT_REGS_RC(ctx); > > > > and verifier would have rejected it. > > But the way the test is written it's not using libbpf's autodetect > > of program type, so everything is passing. > > With: > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c > b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c > index 1c01ee2600a9..e27156dce10d 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c > @@ -67,7 +67,7 @@ void test_bpf_verif_scale(void) > */ > { "pyperf600_nounroll.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, > > - { "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, > + { "loop1.o", BPF_PROG_TYPE_UNSPEC}, > { "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, > > libbpf prog auto-detection kicks in and ... > # ./test_progs -n 3/10 > libbpf: load bpf program failed: Permission denied > libbpf: -- BEGIN DUMP LOG --- > libbpf: > raw_tp 'kfree_skb' doesn't have 10-th argument > invalid bpf_context access off=80 size=8 > > Good :) The verifier is doing its job. oh, another super intuitive error from verifier ;) 10th argument, what?..
On Fri, Oct 11, 2019 at 5:40 PM Alexei Starovoitov <ast@fb.com> wrote: > > On 10/11/19 11:07 AM, Andrii Nakryiko wrote: > > On Wed, Oct 9, 2019 at 9:17 PM Alexei Starovoitov <ast@kernel.org> wrote: > >> > >> For raw tracepoint program types libbpf will try to find > >> btf_id of raw tracepoint in vmlinux's BTF. > >> It's a responsiblity of bpf program author to annotate the program > >> with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint. > >> If "name" is indeed a valid raw tracepoint then in-kernel BTF > >> will have "btf_trace_##name" typedef that points to function > >> prototype of that raw tracepoint. BTF description captures > >> exact argument the kernel C code is passing into raw tracepoint. > >> The kernel verifier will check the types while loading bpf program. > >> > >> libbpf keeps BTF type id in expected_attach_type, but since > >> kernel ignores this attribute for tracing programs copy it > >> into attach_btf_id attribute before loading. > >> > >> Signed-off-by: Alexei Starovoitov <ast@kernel.org> > >> --- > >> tools/lib/bpf/bpf.c | 3 +++ > >> tools/lib/bpf/libbpf.c | 17 +++++++++++++++++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > >> index cbb933532981..79046067720f 100644 > >> --- a/tools/lib/bpf/bpf.c > >> +++ b/tools/lib/bpf/bpf.c > >> @@ -228,6 +228,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, > >> memset(&attr, 0, sizeof(attr)); > >> attr.prog_type = load_attr->prog_type; > >> attr.expected_attach_type = load_attr->expected_attach_type; > >> + if (attr.prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) > >> + /* expected_attach_type is ignored for tracing progs */ > >> + attr.attach_btf_id = attr.expected_attach_type; > >> attr.insn_cnt = (__u32)load_attr->insns_cnt; > >> attr.insns = ptr_to_u64(load_attr->insns); > >> attr.license = ptr_to_u64(load_attr->license); > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index a02cdedc4e3f..8bf30a67428c 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -4586,6 +4586,23 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, > >> continue; > >> *prog_type = section_names[i].prog_type; > >> *expected_attach_type = section_names[i].expected_attach_type; > >> + if (*prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { > >> + struct btf *btf = bpf_core_find_kernel_btf(); > >> + char raw_tp_btf_name[128] = "btf_trace_"; > >> + char *dst = raw_tp_btf_name + sizeof("btf_trace_") - 1; > >> + int ret; > >> + > >> + if (IS_ERR(btf)) > >> + /* lack of kernel BTF is not a failure */ > >> + return 0; > >> + /* prepend "btf_trace_" prefix per kernel convention */ > >> + strncat(dst, name + section_names[i].len, > >> + sizeof(raw_tp_btf_name) - (dst - raw_tp_btf_name)); > >> + ret = btf__find_by_name(btf, raw_tp_btf_name); > >> + if (ret > 0) > >> + *expected_attach_type = ret; > > > > Well, actually, I realized after I gave Acked-by, so not yet :) > > > > This needs kernel feature probe of whether kernel supports specifying > > attach_btf_id, otherwise on older kernels we'll stop successfully > > loading valid program. > > The code above won't find anything on older kernels. > The patch 1 of the series has to be there for proper btf to be > generated by pahole. > Before that happens expected_attach_type will stay zero > and corresponding copy in attach_btf_id will be zero as well. > I see no issues being compatible with older kernels. indeed, this one is not an issue. > > > But even if kernel supports attach_btf_id, I think users still need to > > opt in into specifying attach_btf_id by libbpf. Think about existing > > raw_tp programs that are using bpf_probe_read() because they were not > > created with this kernel feature in mind. They will suddenly stop > > working without any of user's fault. > > This one is excellent catch. > loop1.c should have caught it, since it has > SEC("raw_tracepoint/kfree_skb") > { > int nested_loops(volatile struct pt_regs* ctx) > .. = PT_REGS_RC(ctx); > > and verifier would have rejected it. > But the way the test is written it's not using libbpf's autodetect > of program type, so everything is passing.
On 10/11/19 9:38 PM, Andrii Nakryiko wrote: > On Fri, Oct 11, 2019 at 6:29 PM Alexei Starovoitov <ast@fb.com> wrote: >> >> On 10/11/19 5:40 PM, Alexei Starovoitov wrote: >>>> But even if kernel supports attach_btf_id, I think users still need to >>>> opt in into specifying attach_btf_id by libbpf. Think about existing >>>> raw_tp programs that are using bpf_probe_read() because they were not >>>> created with this kernel feature in mind. They will suddenly stop >>>> working without any of user's fault. >>> >>> This one is excellent catch. >>> loop1.c should have caught it, since it has >>> SEC("raw_tracepoint/kfree_skb") >>> { >>> int nested_loops(volatile struct pt_regs* ctx) >>> .. = PT_REGS_RC(ctx); >>> >>> and verifier would have rejected it. >>> But the way the test is written it's not using libbpf's autodetect >>> of program type, so everything is passing. >> >> With: >> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c >> b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c >> index 1c01ee2600a9..e27156dce10d 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c >> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c >> @@ -67,7 +67,7 @@ void test_bpf_verif_scale(void) >> */ >> { "pyperf600_nounroll.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, >> >> - { "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, >> + { "loop1.o", BPF_PROG_TYPE_UNSPEC}, >> { "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT }, >> >> libbpf prog auto-detection kicks in and ... >> # ./test_progs -n 3/10 >> libbpf: load bpf program failed: Permission denied >> libbpf: -- BEGIN DUMP LOG --- >> libbpf: >> raw_tp 'kfree_skb' doesn't have 10-th argument >> invalid bpf_context access off=80 size=8 >> >> Good :) The verifier is doing its job. > > oh, another super intuitive error from verifier ;) 10th argument, what?.. I know, but there is no env->linfo and no insn_idx to call verbose_linfo() from there. That's even bigger refactoring that I'd rather to later.
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index cbb933532981..79046067720f 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -228,6 +228,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, memset(&attr, 0, sizeof(attr)); attr.prog_type = load_attr->prog_type; attr.expected_attach_type = load_attr->expected_attach_type; + if (attr.prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) + /* expected_attach_type is ignored for tracing progs */ + attr.attach_btf_id = attr.expected_attach_type; attr.insn_cnt = (__u32)load_attr->insns_cnt; attr.insns = ptr_to_u64(load_attr->insns); attr.license = ptr_to_u64(load_attr->license); diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index a02cdedc4e3f..8bf30a67428c 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4586,6 +4586,23 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, continue; *prog_type = section_names[i].prog_type; *expected_attach_type = section_names[i].expected_attach_type; + if (*prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { + struct btf *btf = bpf_core_find_kernel_btf(); + char raw_tp_btf_name[128] = "btf_trace_"; + char *dst = raw_tp_btf_name + sizeof("btf_trace_") - 1; + int ret; + + if (IS_ERR(btf)) + /* lack of kernel BTF is not a failure */ + return 0; + /* prepend "btf_trace_" prefix per kernel convention */ + strncat(dst, name + section_names[i].len, + sizeof(raw_tp_btf_name) - (dst - raw_tp_btf_name)); + ret = btf__find_by_name(btf, raw_tp_btf_name); + if (ret > 0) + *expected_attach_type = ret; + btf__free(btf); + } return 0; } pr_warning("failed to guess program type based on ELF section name '%s'\n", name);
For raw tracepoint program types libbpf will try to find btf_id of raw tracepoint in vmlinux's BTF. It's a responsiblity of bpf program author to annotate the program with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint. If "name" is indeed a valid raw tracepoint then in-kernel BTF will have "btf_trace_##name" typedef that points to function prototype of that raw tracepoint. BTF description captures exact argument the kernel C code is passing into raw tracepoint. The kernel verifier will check the types while loading bpf program. libbpf keeps BTF type id in expected_attach_type, but since kernel ignores this attribute for tracing programs copy it into attach_btf_id attribute before loading. Signed-off-by: Alexei Starovoitov <ast@kernel.org> --- tools/lib/bpf/bpf.c | 3 +++ tools/lib/bpf/libbpf.c | 17 +++++++++++++++++ 2 files changed, 20 insertions(+)