| Message ID | 20220414223704.341028-2-alobakin@pm.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: random unpopular userspace fixes (32 bit et al.) | expand |
On Thu, Apr 14, 2022 at 3:45 PM Alexander Lobakin <alobakin@pm.me> wrote: > > When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty. > However, the structure is being used by bpftool indirectly via BTF. > This leads to: > > skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' > return BPF_CORE_READ(event, bpf_cookie); > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > ... > > skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long') > return BPF_CORE_READ(event, bpf_cookie); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Tools and samples can't use any CONFIG_ definitions, so the fields > used there should always be present. > Move CONFIG_BPF_SYSCALL block out of the CONFIG_PERF_EVENTS block > to make it available unconditionally. > > Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output") > Signed-off-by: Alexander Lobakin <alobakin@pm.me> While I can't think of a real failure with this approach, it does feel weird to me. Can we fix this with bpf_core_field_exists()? Thanks, Song > --- > include/linux/perf_event.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index af97dd427501..b1d5715b8b34 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -762,12 +762,14 @@ struct perf_event { > u64 (*clock)(void); > perf_overflow_handler_t overflow_handler; > void *overflow_handler_context; > +#endif /* CONFIG_PERF_EVENTS */ > #ifdef CONFIG_BPF_SYSCALL > perf_overflow_handler_t orig_overflow_handler; > struct bpf_prog *prog; > u64 bpf_cookie; > #endif > > +#ifdef CONFIG_PERF_EVENTS > #ifdef CONFIG_EVENT_TRACING > struct trace_event_call *tp_event; > struct event_filter *filter; > -- > 2.35.2 > >
On Fri, Apr 15, 2022 at 4:07 PM Song Liu <song@kernel.org> wrote: > > On Thu, Apr 14, 2022 at 3:45 PM Alexander Lobakin <alobakin@pm.me> wrote: > > > > When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty. > > However, the structure is being used by bpftool indirectly via BTF. > > This leads to: > > > > skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' > > return BPF_CORE_READ(event, bpf_cookie); > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > > > ... > > > > skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long') > > return BPF_CORE_READ(event, bpf_cookie); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Tools and samples can't use any CONFIG_ definitions, so the fields > > used there should always be present. > > Move CONFIG_BPF_SYSCALL block out of the CONFIG_PERF_EVENTS block > > to make it available unconditionally. > > > > Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output") > > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > > While I can't think of a real failure with this approach, it does feel > weird to me. Can we fix this with bpf_core_field_exists()? Hmm.. the error happens at compile time, so I guess it is not very easy. Andrii, Do you have some recommendation on this? Song
On Thu, Apr 14, 2022 at 10:44:48PM +0000, Alexander Lobakin wrote: > When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty. > However, the structure is being used by bpftool indirectly via BTF. > This leads to: > > skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' > return BPF_CORE_READ(event, bpf_cookie); > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > ... > > skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long') > return BPF_CORE_READ(event, bpf_cookie); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Tools and samples can't use any CONFIG_ definitions, so the fields > used there should always be present. > Move CONFIG_BPF_SYSCALL block out of the CONFIG_PERF_EVENTS block > to make it available unconditionally. Urgh, this is nasty.. did you verify nothing relies on that structure actually being empty? Also, why are we changing kernel headers to fix some daft userspace issue? > Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output") > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > --- > include/linux/perf_event.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index af97dd427501..b1d5715b8b34 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -762,12 +762,14 @@ struct perf_event { > u64 (*clock)(void); > perf_overflow_handler_t overflow_handler; > void *overflow_handler_context; > +#endif /* CONFIG_PERF_EVENTS */ > #ifdef CONFIG_BPF_SYSCALL > perf_overflow_handler_t orig_overflow_handler; > struct bpf_prog *prog; > u64 bpf_cookie; > #endif > > +#ifdef CONFIG_PERF_EVENTS > #ifdef CONFIG_EVENT_TRACING > struct trace_event_call *tp_event; > struct event_filter *filter; > -- > 2.35.2 > >
On Tue, Apr 19, 2022 at 2:04 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Apr 14, 2022 at 10:44:48PM +0000, Alexander Lobakin wrote: > > When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty. > > However, the structure is being used by bpftool indirectly via BTF. > > This leads to: > > > > skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' > > return BPF_CORE_READ(event, bpf_cookie); > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > > > > ... > > > > skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long') > > return BPF_CORE_READ(event, bpf_cookie); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Tools and samples can't use any CONFIG_ definitions, so the fields > > used there should always be present. > > Move CONFIG_BPF_SYSCALL block out of the CONFIG_PERF_EVENTS block > > to make it available unconditionally. > > Urgh, this is nasty.. did you verify nothing relies on that structure > actually being empty? > > Also, why are we changing kernel headers to fix some daft userspace > issue? > I agree, this is quite ugly. And I think it's not necessary at all. BPF CO-RE, which bpftool relies on here, allows to have bpftool's own minimal definition of struct perf_event with bpf_cookie field and not rely on UAPI headers having full definition. Something like this: struct perf_event___local { u64 bpf_cookie; } __attribute__((preserve_access_index)); Then use `struct perf_event___local` (note the three underscores, they are important) instead of struct perf_event in BPF code. And we'll have to do the same for struct bpf_perf_link, I presume? > > Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output") > > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > > --- > > include/linux/perf_event.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index af97dd427501..b1d5715b8b34 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -762,12 +762,14 @@ struct perf_event { > > u64 (*clock)(void); > > perf_overflow_handler_t overflow_handler; > > void *overflow_handler_context; > > +#endif /* CONFIG_PERF_EVENTS */ > > #ifdef CONFIG_BPF_SYSCALL > > perf_overflow_handler_t orig_overflow_handler; > > struct bpf_prog *prog; > > u64 bpf_cookie; > > #endif > > > > +#ifdef CONFIG_PERF_EVENTS > > #ifdef CONFIG_EVENT_TRACING > > struct trace_event_call *tp_event; > > struct event_filter *filter; > > -- > > 2.35.2 > > > >
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index af97dd427501..b1d5715b8b34 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -762,12 +762,14 @@ struct perf_event { u64 (*clock)(void); perf_overflow_handler_t overflow_handler; void *overflow_handler_context; +#endif /* CONFIG_PERF_EVENTS */ #ifdef CONFIG_BPF_SYSCALL perf_overflow_handler_t orig_overflow_handler; struct bpf_prog *prog; u64 bpf_cookie; #endif +#ifdef CONFIG_PERF_EVENTS #ifdef CONFIG_EVENT_TRACING struct trace_event_call *tp_event; struct event_filter *filter;
When CONFIG_PERF_EVENTS is not set, struct perf_event remains empty. However, the structure is being used by bpftool indirectly via BTF. This leads to: skeleton/pid_iter.bpf.c:49:30: error: no member named 'bpf_cookie' in 'struct perf_event' return BPF_CORE_READ(event, bpf_cookie); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ ... skeleton/pid_iter.bpf.c:49:9: error: returning 'void' from a function with incompatible result type '__u64' (aka 'unsigned long long') return BPF_CORE_READ(event, bpf_cookie); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Tools and samples can't use any CONFIG_ definitions, so the fields used there should always be present. Move CONFIG_BPF_SYSCALL block out of the CONFIG_PERF_EVENTS block to make it available unconditionally. Fixes: cbdaf71f7e65 ("bpftool: Add bpf_cookie to link output") Signed-off-by: Alexander Lobakin <alobakin@pm.me> --- include/linux/perf_event.h | 2 ++ 1 file changed, 2 insertions(+) -- 2.35.2