* [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation
@ 2020-03-01 8:10 Andrii Nakryiko
2020-03-01 8:10 ` [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types Andrii Nakryiko
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-03-01 8:10 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, ethercflow
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Fix issue with not preserving btf_trace_##call structs when compiled under
Clang. Additionally, capture raw tracepoint arguments in raw_tp_##call
structs, directly usable from BPF programs. Convert runqslower to use those
for proof of concept and to simplify code further.
Andrii Nakryiko (3):
bpf: reliably preserve btf_trace_xxx types
bpf: generate directly-usable raw_tp_##call structs for raw
tracepoints
tools/runqslower: simplify BPF code by using raw_tp_xxx structs
include/trace/bpf_probe.h | 37 ++++++++++++++++++++++-----
tools/bpf/runqslower/runqslower.bpf.c | 22 ++++++----------
2 files changed, 38 insertions(+), 21 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types 2020-03-01 8:10 [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Andrii Nakryiko @ 2020-03-01 8:10 ` Andrii Nakryiko 2020-03-02 16:35 ` Yonghong Song 2020-03-01 8:10 ` [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints Andrii Nakryiko ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Andrii Nakryiko @ 2020-03-01 8:10 UTC (permalink / raw) To: bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko btf_trace_xxx types, crucial for tp_btf BPF programs (raw tracepoint with verifier-checked direct memory access), have to be preserved in kernel BTF to allow verifier do its job and enforce type/memory safety. It was reported ([0]) that for kernels built with Clang current type-casting approach doesn't preserve these types. This patch fixes it by declaring an anonymous union for each registered tracepoint, capturing both struct bpf_raw_event_map information, as well as recording btf_trace_##call type reliably. Structurally, it's still the same content as for a plain struct bpf_raw_event_map, so no other changes are necessary. [0] https://github.com/iovisor/bcc/issues/2770#issuecomment-591007692 Fixes: e8c423fb31fa ("bpf: Add typecast to raw_tracepoints to help BTF generation") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- include/trace/bpf_probe.h | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h index b04c29270973..1ce3be63add1 100644 --- a/include/trace/bpf_probe.h +++ b/include/trace/bpf_probe.h @@ -75,13 +75,17 @@ static inline void bpf_test_probe_##call(void) \ check_trace_callback_type_##call(__bpf_trace_##template); \ } \ typedef void (*btf_trace_##call)(void *__data, proto); \ -static struct bpf_raw_event_map __used \ - __attribute__((section("__bpf_raw_tp_map"))) \ -__bpf_trace_tp_map_##call = { \ - .tp = &__tracepoint_##call, \ - .bpf_func = (void *)(btf_trace_##call)__bpf_trace_##template, \ - .num_args = COUNT_ARGS(args), \ - .writable_size = size, \ +static union { \ + struct bpf_raw_event_map event; \ + btf_trace_##call handler; \ +} __bpf_trace_tp_map_##call __used \ +__attribute__((section("__bpf_raw_tp_map"))) = { \ + .event = { \ + .tp = &__tracepoint_##call, \ + .bpf_func = __bpf_trace_##template, \ + .num_args = COUNT_ARGS(args), \ + .writable_size = size, \ + }, \ }; #define FIRST(x, ...) x -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types 2020-03-01 8:10 ` [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types Andrii Nakryiko @ 2020-03-02 16:35 ` Yonghong Song 0 siblings, 0 replies; 11+ messages in thread From: Yonghong Song @ 2020-03-02 16:35 UTC (permalink / raw) To: Andrii Nakryiko, bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team On 3/1/20 12:10 AM, Andrii Nakryiko wrote: > btf_trace_xxx types, crucial for tp_btf BPF programs (raw tracepoint with > verifier-checked direct memory access), have to be preserved in kernel BTF to > allow verifier do its job and enforce type/memory safety. It was reported > ([0]) that for kernels built with Clang current type-casting approach doesn't > preserve these types. > > This patch fixes it by declaring an anonymous union for each registered > tracepoint, capturing both struct bpf_raw_event_map information, as well as > recording btf_trace_##call type reliably. Structurally, it's still the same > content as for a plain struct bpf_raw_event_map, so no other changes are > necessary. > > [0] https://github.com/iovisor/bcc/issues/2770#issuecomment-591007692 > > Fixes: e8c423fb31fa ("bpf: Add typecast to raw_tracepoints to help BTF generation") > Reported-by: Wenbo Zhang <ethercflow@gmail.com> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> clang seems doing a little bit optimization here... The change looks good. It is hard to have code to preserve the types in the header. union seems an acceptable way. Acked-by: Yonghong Song <yhs@fb.com> > --- > include/trace/bpf_probe.h | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h > index b04c29270973..1ce3be63add1 100644 > --- a/include/trace/bpf_probe.h > +++ b/include/trace/bpf_probe.h > @@ -75,13 +75,17 @@ static inline void bpf_test_probe_##call(void) \ > check_trace_callback_type_##call(__bpf_trace_##template); \ > } \ > typedef void (*btf_trace_##call)(void *__data, proto); \ > -static struct bpf_raw_event_map __used \ > - __attribute__((section("__bpf_raw_tp_map"))) \ > -__bpf_trace_tp_map_##call = { \ > - .tp = &__tracepoint_##call, \ > - .bpf_func = (void *)(btf_trace_##call)__bpf_trace_##template, \ > - .num_args = COUNT_ARGS(args), \ > - .writable_size = size, \ > +static union { \ > + struct bpf_raw_event_map event; \ > + btf_trace_##call handler; \ > +} __bpf_trace_tp_map_##call __used \ > +__attribute__((section("__bpf_raw_tp_map"))) = { \ > + .event = { \ > + .tp = &__tracepoint_##call, \ > + .bpf_func = __bpf_trace_##template, \ > + .num_args = COUNT_ARGS(args), \ > + .writable_size = size, \ > + }, \ > }; > > #define FIRST(x, ...) x > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints 2020-03-01 8:10 [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Andrii Nakryiko 2020-03-01 8:10 ` [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types Andrii Nakryiko @ 2020-03-01 8:10 ` Andrii Nakryiko 2020-03-02 16:36 ` Yonghong Song 2020-03-01 8:10 ` [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs Andrii Nakryiko 2020-03-03 0:59 ` [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Alexei Starovoitov 3 siblings, 1 reply; 11+ messages in thread From: Andrii Nakryiko @ 2020-03-01 8:10 UTC (permalink / raw) To: bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko In addition to btf_trace_##call typedefs to func protos, generate a struct raw_tp_##call with memory layout directly usable from BPF programs to access raw tracepoint arguments. This allows for user BPF programs to directly use such structs for their raw tracepoint BPF programs when using vmlinux.h, without having to manually copy/paste and maintain raw tracepoint argument declarations. Additionally, due to CO-RE and preserve_access_index attribute, such structs are relocatable, all the CO-RE relocations and field existence checks are available automatically to such BPF programs. runqslower example in next patch will demonstrate this usage. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- include/trace/bpf_probe.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h index 1ce3be63add1..a9e83236c181 100644 --- a/include/trace/bpf_probe.h +++ b/include/trace/bpf_probe.h @@ -55,6 +55,21 @@ /* tracepoints with more than 12 arguments will hit build error */ #define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__) +#define __RAW_TP_ARG(a) a __attribute__((aligned(8))) +#define __RAW_TP_ARGS1(a,...) __RAW_TP_ARG(a); +#define __RAW_TP_ARGS2(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS1(__VA_ARGS__); +#define __RAW_TP_ARGS3(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS2(__VA_ARGS__); +#define __RAW_TP_ARGS4(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS3(__VA_ARGS__); +#define __RAW_TP_ARGS5(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS4(__VA_ARGS__); +#define __RAW_TP_ARGS6(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS5(__VA_ARGS__); +#define __RAW_TP_ARGS7(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS6(__VA_ARGS__); +#define __RAW_TP_ARGS8(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS7(__VA_ARGS__); +#define __RAW_TP_ARGS9(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS8(__VA_ARGS__); +#define __RAW_TP_ARGS10(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS9(__VA_ARGS__); +#define __RAW_TP_ARGS11(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS10(__VA_ARGS__); +#define __RAW_TP_ARGS12(a,...) __RAW_TP_ARG(a); __RAW_TP_ARGS11(__VA_ARGS__); +#define RAW_TP_ARGS(args...) CONCATENATE(__RAW_TP_ARGS, COUNT_ARGS(args))(args) + #undef DECLARE_EVENT_CLASS #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \ static notrace void \ @@ -75,9 +90,13 @@ static inline void bpf_test_probe_##call(void) \ check_trace_callback_type_##call(__bpf_trace_##template); \ } \ typedef void (*btf_trace_##call)(void *__data, proto); \ +struct raw_tp_##call { \ + RAW_TP_ARGS(proto) \ +}; \ static union { \ struct bpf_raw_event_map event; \ btf_trace_##call handler; \ + struct raw_tp_##call *raw_tp_args; \ } __bpf_trace_tp_map_##call __used \ __attribute__((section("__bpf_raw_tp_map"))) = { \ .event = { \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints 2020-03-01 8:10 ` [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints Andrii Nakryiko @ 2020-03-02 16:36 ` Yonghong Song 0 siblings, 0 replies; 11+ messages in thread From: Yonghong Song @ 2020-03-02 16:36 UTC (permalink / raw) To: Andrii Nakryiko, bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team On 3/1/20 12:10 AM, Andrii Nakryiko wrote: > In addition to btf_trace_##call typedefs to func protos, generate a struct > raw_tp_##call with memory layout directly usable from BPF programs to access > raw tracepoint arguments. This allows for user BPF programs to directly use > such structs for their raw tracepoint BPF programs when using vmlinux.h, > without having to manually copy/paste and maintain raw tracepoint argument > declarations. Additionally, due to CO-RE and preserve_access_index attribute, > such structs are relocatable, all the CO-RE relocations and field existence > checks are available automatically to such BPF programs. > > runqslower example in next patch will demonstrate this usage. > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Yonghong Song <yhs@fb.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs 2020-03-01 8:10 [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Andrii Nakryiko 2020-03-01 8:10 ` [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types Andrii Nakryiko 2020-03-01 8:10 ` [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints Andrii Nakryiko @ 2020-03-01 8:10 ` Andrii Nakryiko 2020-03-02 16:36 ` Yonghong Song 2020-03-03 0:59 ` [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Alexei Starovoitov 3 siblings, 1 reply; 11+ messages in thread From: Andrii Nakryiko @ 2020-03-01 8:10 UTC (permalink / raw) To: bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko Convert runqslower to utilize raw_tp_xxx structs for accessing raw tracepoint arguments. Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/bpf/runqslower/runqslower.bpf.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c index 48a39f72fadf..3931ef9c9a6c 100644 --- a/tools/bpf/runqslower/runqslower.bpf.c +++ b/tools/bpf/runqslower/runqslower.bpf.c @@ -40,41 +40,35 @@ static int trace_enqueue(u32 tgid, u32 pid) } SEC("tp_btf/sched_wakeup") -int handle__sched_wakeup(u64 *ctx) +int handle__sched_wakeup(struct raw_tp_sched_wakeup *ctx) { /* TP_PROTO(struct task_struct *p) */ - struct task_struct *p = (void *)ctx[0]; - - return trace_enqueue(p->tgid, p->pid); + return trace_enqueue(ctx->p->tgid, ctx->p->pid); } SEC("tp_btf/sched_wakeup_new") -int handle__sched_wakeup_new(u64 *ctx) +int handle__sched_wakeup_new(struct raw_tp_sched_wakeup_new *ctx) { /* TP_PROTO(struct task_struct *p) */ - struct task_struct *p = (void *)ctx[0]; - - return trace_enqueue(p->tgid, p->pid); + return trace_enqueue(ctx->p->tgid, ctx->p->pid); } SEC("tp_btf/sched_switch") -int handle__sched_switch(u64 *ctx) +int handle__sched_switch(struct raw_tp_sched_switch *ctx) { /* TP_PROTO(bool preempt, struct task_struct *prev, * struct task_struct *next) */ - struct task_struct *prev = (struct task_struct *)ctx[1]; - struct task_struct *next = (struct task_struct *)ctx[2]; struct event event = {}; u64 *tsp, delta_us; long state; u32 pid; /* ivcsw: treat like an enqueue event and store timestamp */ - if (prev->state == TASK_RUNNING) - trace_enqueue(prev->tgid, prev->pid); + if (ctx->prev->state == TASK_RUNNING) + trace_enqueue(ctx->prev->tgid, ctx->prev->pid); - pid = next->pid; + pid = ctx->next->pid; /* fetch timestamp and calculate delta */ tsp = bpf_map_lookup_elem(&start, &pid); -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs 2020-03-01 8:10 ` [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs Andrii Nakryiko @ 2020-03-02 16:36 ` Yonghong Song 0 siblings, 0 replies; 11+ messages in thread From: Yonghong Song @ 2020-03-02 16:36 UTC (permalink / raw) To: Andrii Nakryiko, bpf, netdev, ast, daniel, ethercflow Cc: andrii.nakryiko, kernel-team On 3/1/20 12:10 AM, Andrii Nakryiko wrote: > Convert runqslower to utilize raw_tp_xxx structs for accessing raw tracepoint > arguments. > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Yonghong Song <yhs@fb.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation 2020-03-01 8:10 [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Andrii Nakryiko ` (2 preceding siblings ...) 2020-03-01 8:10 ` [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs Andrii Nakryiko @ 2020-03-03 0:59 ` Alexei Starovoitov 2020-03-03 4:10 ` Andrii Nakryiko 3 siblings, 1 reply; 11+ messages in thread From: Alexei Starovoitov @ 2020-03-03 0:59 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, netdev, ast, daniel, ethercflow, andrii.nakryiko, kernel-team On Sun, Mar 01, 2020 at 12:10:42AM -0800, Andrii Nakryiko wrote: > Fix issue with not preserving btf_trace_##call structs when compiled under > Clang. Additionally, capture raw tracepoint arguments in raw_tp_##call > structs, directly usable from BPF programs. Convert runqslower to use those > for proof of concept and to simplify code further. Not only folks compile kernel with clang they use the latest BPF/BTF features with it. This is very nice to see! I've applied 1st patch to make clang compiled kernel emit proper BTF. As far as patch 2 I'm not sure about 'raw_tp_' prefix. tp_btf type of progs can use the same structs. So I think there could be a better name. Also bpftool can generate them as well while emitting vmlinux.h. I think that will avoid adding few kilobytes to vmlinux BTF that kernel isn't going to use atm. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation 2020-03-03 0:59 ` [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Alexei Starovoitov @ 2020-03-03 4:10 ` Andrii Nakryiko 2020-03-03 4:59 ` Andrii Nakryiko 0 siblings, 1 reply; 11+ messages in thread From: Andrii Nakryiko @ 2020-03-03 4:10 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Wenbo Zhang, Kernel Team On Mon, Mar 2, 2020 at 4:59 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sun, Mar 01, 2020 at 12:10:42AM -0800, Andrii Nakryiko wrote: > > Fix issue with not preserving btf_trace_##call structs when compiled under > > Clang. Additionally, capture raw tracepoint arguments in raw_tp_##call > > structs, directly usable from BPF programs. Convert runqslower to use those > > for proof of concept and to simplify code further. > > Not only folks compile kernel with clang they use the latest BPF/BTF features > with it. This is very nice to see! > I've applied 1st patch to make clang compiled kernel emit proper BTF. > > As far as patch 2 I'm not sure about 'raw_tp_' prefix. tp_btf type of progs can > use the same structs. So I think there could be a better name. Also bpftool can > generate them as well while emitting vmlinux.h. I think that will avoid adding > few kilobytes to vmlinux BTF that kernel isn't going to use atm. Fair enough, I'll follow up with bpftool changes to generate such structs. I'm thinking to use tp_args_xxx name pattern, unless someone has a better idea :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation 2020-03-03 4:10 ` Andrii Nakryiko @ 2020-03-03 4:59 ` Andrii Nakryiko 2020-04-21 21:49 ` Andrii Nakryiko 0 siblings, 1 reply; 11+ messages in thread From: Andrii Nakryiko @ 2020-03-03 4:59 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Wenbo Zhang, Kernel Team On Mon, Mar 2, 2020 at 8:10 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Mar 2, 2020 at 4:59 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Sun, Mar 01, 2020 at 12:10:42AM -0800, Andrii Nakryiko wrote: > > > Fix issue with not preserving btf_trace_##call structs when compiled under > > > Clang. Additionally, capture raw tracepoint arguments in raw_tp_##call > > > structs, directly usable from BPF programs. Convert runqslower to use those > > > for proof of concept and to simplify code further. > > > > Not only folks compile kernel with clang they use the latest BPF/BTF features > > with it. This is very nice to see! > > I've applied 1st patch to make clang compiled kernel emit proper BTF. > > > > As far as patch 2 I'm not sure about 'raw_tp_' prefix. tp_btf type of progs can > > use the same structs. So I think there could be a better name. Also bpftool can > > generate them as well while emitting vmlinux.h. I think that will avoid adding > > few kilobytes to vmlinux BTF that kernel isn't going to use atm. > > Fair enough, I'll follow up with bpftool changes to generate such > structs. I'm thinking to use tp_args_xxx name pattern, unless someone > has a better idea :) Bad news. BTF_KIND_FUNC_PROTOs don't capture argument names and having something like: struct tp_args_sched_switch { bool arg1; struct task_struct *arg2; struct task_struct *arg3; }; doesn't seem like a good solution... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation 2020-03-03 4:59 ` Andrii Nakryiko @ 2020-04-21 21:49 ` Andrii Nakryiko 0 siblings, 0 replies; 11+ messages in thread From: Andrii Nakryiko @ 2020-04-21 21:49 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Wenbo Zhang, Kernel Team On Mon, Mar 2, 2020 at 8:59 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Mar 2, 2020 at 8:10 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Mar 2, 2020 at 4:59 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Sun, Mar 01, 2020 at 12:10:42AM -0800, Andrii Nakryiko wrote: > > > > Fix issue with not preserving btf_trace_##call structs when compiled under > > > > Clang. Additionally, capture raw tracepoint arguments in raw_tp_##call > > > > structs, directly usable from BPF programs. Convert runqslower to use those > > > > for proof of concept and to simplify code further. > > > > > > Not only folks compile kernel with clang they use the latest BPF/BTF features > > > with it. This is very nice to see! > > > I've applied 1st patch to make clang compiled kernel emit proper BTF. > > > > > > As far as patch 2 I'm not sure about 'raw_tp_' prefix. tp_btf type of progs can > > > use the same structs. So I think there could be a better name. Also bpftool can > > > generate them as well while emitting vmlinux.h. I think that will avoid adding > > > few kilobytes to vmlinux BTF that kernel isn't going to use atm. > > > > Fair enough, I'll follow up with bpftool changes to generate such > > structs. I'm thinking to use tp_args_xxx name pattern, unless someone > > has a better idea :) > > Bad news. BTF_KIND_FUNC_PROTOs don't capture argument names and having > something like: > > struct tp_args_sched_switch { > bool arg1; > struct task_struct *arg2; > struct task_struct *arg3; > }; > > doesn't seem like a good solution... I'd like to surface this one more time. I'd like to give just a bit more context first, though. Currently, when using various types of BPF programs (kprobe, fentry/fexit, lsm, etc), one can use few ways to extract input arguments of the "intercepted" function. One of the more user-friendly ways to do it is through BPF_PROG macro. So BPF code would look like this: SEC("lsm/file_mprotect") int BPF_PROG(test_int_hook, struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot, int ret) { if (ret != 0) { ... } /* use cma, reqprot, etc directly as variables */ } This is kind of nice, but involves quite a bit of macro magic and requires user to find and copy/paste those function prototypes and track whether they ever change. The alternative for this would be to have a memory-layout-compatible struct, that can be used directly as a context. For the above example: struct btf_lsm_file_mprotect_ctx { struct vm_area_struct *vma; unsigned long reqprot; int ret __attribute__((aligned(8))); }; With that, one can do a nice pure C code: int test_int_hook(struct btf_lsm_file_mprotect_ctx *ctx) { if (ctx->ret != 0) { ... } /* here instead of using vma, reqprot and ret directly, * one would access them as ctx fields: ctx->vma, ctx->reqprot */ } The benefit of the latter is that, hopefully, no one will have to write such struct definitions by hand, they would be generated as part of vmlinux.h and would come directly from kernel BTF (in one way or another, see below). One added benefit is that such struct is CO-RE-relocatable, so if fields are ever, say, reordered or removed, by using CO-RE mechanisms one can write a compiled-once BPF program to accommodate such changes and even incompatibilities. Now to why I'm bringing this up again. The original plan was to use bpftool to convert func_protos that are used directly by kernel (e.g., btf_trace_xxx ones for raw tracepoints) to dump them as memory layout-compatible structs at the end of vmlinux.h for use by BPF programs. Unfortunately, I don't think that will work because func_proto arguments don't preserve their names in BTF. Which, as I pointed out in previous email, ruins usability of generated structs. So I'd like to solicit feedback on how we can proceed from here. I see few possible ways to go about this: 1. Do nothing. Always an option. It sucks for users (they need to copy-paste function definitions, use BPF_PROG macro, etc), but is awesome for kernel (no changes, no extra stuff). 2. Bite a bullet and add compatible struct definitions into kernel itself. It will slightly increase kernel BTF, but will require no changes on user-space and tooling side. vmlinux.h just magically gets proper structs that are directly usable from BPF programs. One objection to that is that those structs are not directly used by kernel and thus are just a dead weight. 3. Bite even bigger bullet and convert current uses of func_proto in kernel to struct. That way we don't have unnecessary types laying around in the kernel, verifier actually will use these structs for verification. There might be a concern about backwards compatibility. Libbpf can easily accommodate such changes by searching for either struct or func_proto, whichever is available, but one can argue that we have to leave existing btf_trace_xxx func protos intact if that's considered to be part of UAPI. 4. Milder variant of #3 would be to convert typedef of func_protos into a use of proper FUNCs. They still use func_proto, but that func_proto actually preserves argument names, and would allow bpftool to dump proper structs. Same considerations of what to do with backwards compatibility of btf_trace_xxx typedefs+func_proto, but won't require much verifier changes, because it's still going to be func_proto that need to be used for verification. This is what Yonghong's bpfdump changes actually do: they use __init empty funcs to provide types for verifier. I think it's important to discuss this, because more and more BPF program types are relying on using similar approach (e.g., LSM, bpfdump), and it would be nice to provide a good **and uniform** solution to let users just use a proper context structure directly, instead of using BPF_PROG macro (best case, worst case it's a direct u64 array conversions) and copy-pasting definition. Thanks for any feedback upfront! ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-21 21:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-01 8:10 [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Andrii Nakryiko 2020-03-01 8:10 ` [PATCH bpf-next 1/3] bpf: reliably preserve btf_trace_xxx types Andrii Nakryiko 2020-03-02 16:35 ` Yonghong Song 2020-03-01 8:10 ` [PATCH bpf-next 2/3] bpf: generate directly-usable raw_tp_##call structs for raw tracepoints Andrii Nakryiko 2020-03-02 16:36 ` Yonghong Song 2020-03-01 8:10 ` [PATCH bpf-next 3/3] tools/runqslower: simplify BPF code by using raw_tp_xxx structs Andrii Nakryiko 2020-03-02 16:36 ` Yonghong Song 2020-03-03 0:59 ` [PATCH bpf-next 0/3] Improve raw tracepoint BTF types preservation Alexei Starovoitov 2020-03-03 4:10 ` Andrii Nakryiko 2020-03-03 4:59 ` Andrii Nakryiko 2020-04-21 21:49 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).