| Message ID | 20171213074252.823255-1-yhs@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net-next] bpf/tracing: fix kernel/events/core.c compilation error | expand |
On 12/13/2017 08:42 AM, Yonghong Song wrote: > Commit f371b304f12e ("bpf/tracing: allow user space to > query prog array on the same tp") introduced a perf > ioctl command to query prog array attached to the > same perf tracepoint. The commit introduced a > compilation error when either CONFIG_BPF_SYSCALL or > CONFIG_EVENT_TRACING is not defined: > kernel/events/core.o: In function `perf_ioctl': > core.c:(.text+0x98c4): undefined reference to `bpf_event_query_prog_array' > > This patch fixed this error. > > Fixes: f371b304f12e ("bpf/tracing: allow user space to query prog array on the same tp") > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Yonghong Song <yhs@fb.com> Looking at _perf_ioctl(), we also have perf_event_set_bpf_prog() there. It's basically under CONFIG_EVENT_TRACING, which later calls perf_event_attach_bpf_prog() which is under CONFIG_BPF_EVENTS, so where we have the dummy handler returning -EOPNOTSUPP when BPF events is not set. bpf_trace.c is only built when CONFIG_BPF_EVENTS is set and that by itself depends on BPF_SYSCALL already. So it would be more correct to do the same thing here ... #if defined(CONFIG_EVENT_TRACING) && defined(CONFIG_BPF_EVENTS) [...] > --- > include/linux/bpf.h | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 93e15b9..8dbbfd7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -254,7 +254,6 @@ typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src, > > u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size, > void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy); > -int bpf_event_query_prog_array(struct perf_event *event, void __user *info); > > int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > union bpf_attr __user *uattr); > @@ -558,6 +557,15 @@ static inline int sock_map_prog(struct bpf_map *map, > } > #endif > > +#if defined(CONFIG_EVENT_TRACING) && defined(CONFIG_BPF_SYSCALL) > +int bpf_event_query_prog_array(struct perf_event *event, void __user *info); > +#else > +static inline int bpf_event_query_prog_array(struct perf_event *event, void __user *info) > +{ > + return -EOPNOTSUPP; > +} > +#endif > + > /* verifier prototypes for helper functions called from eBPF programs */ > extern const struct bpf_func_proto bpf_map_lookup_elem_proto; > extern const struct bpf_func_proto bpf_map_update_elem_proto; >
On 12/13/17 7:44 AM, Daniel Borkmann wrote: > On 12/13/2017 08:42 AM, Yonghong Song wrote: >> Commit f371b304f12e ("bpf/tracing: allow user space to >> query prog array on the same tp") introduced a perf >> ioctl command to query prog array attached to the >> same perf tracepoint. The commit introduced a >> compilation error when either CONFIG_BPF_SYSCALL or >> CONFIG_EVENT_TRACING is not defined: >> kernel/events/core.o: In function `perf_ioctl': >> core.c:(.text+0x98c4): undefined reference to `bpf_event_query_prog_array' >> >> This patch fixed this error. >> >> Fixes: f371b304f12e ("bpf/tracing: allow user space to query prog array on the same tp") >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >> Signed-off-by: Yonghong Song <yhs@fb.com> > > Looking at _perf_ioctl(), we also have perf_event_set_bpf_prog() > there. It's basically under CONFIG_EVENT_TRACING, which later calls > perf_event_attach_bpf_prog() which is under CONFIG_BPF_EVENTS, so > where we have the dummy handler returning -EOPNOTSUPP when BPF > events is not set. bpf_trace.c is only built when CONFIG_BPF_EVENTS > is set and that by itself depends on BPF_SYSCALL already. So it would > be more correct to do the same thing here ... > > #if defined(CONFIG_EVENT_TRACING) && defined(CONFIG_BPF_EVENTS) > [...] +1 #ifdef CONFIG_BPF_EVENTS works, whereas CONFIG_EVENT_TRACING probably not, since kprobe can be disabled independently which will turn off BPF_EVENTS and body of bpf_event_query_prog_array() will be gone.
On 12/13/17 7:50 AM, Alexei Starovoitov wrote: > On 12/13/17 7:44 AM, Daniel Borkmann wrote: >> On 12/13/2017 08:42 AM, Yonghong Song wrote: >>> Commit f371b304f12e ("bpf/tracing: allow user space to >>> query prog array on the same tp") introduced a perf >>> ioctl command to query prog array attached to the >>> same perf tracepoint. The commit introduced a >>> compilation error when either CONFIG_BPF_SYSCALL or >>> CONFIG_EVENT_TRACING is not defined: >>> kernel/events/core.o: In function `perf_ioctl': >>> core.c:(.text+0x98c4): undefined reference to >>> `bpf_event_query_prog_array' >>> >>> This patch fixed this error. >>> >>> Fixes: f371b304f12e ("bpf/tracing: allow user space to query prog >>> array on the same tp") >>> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >>> Signed-off-by: Yonghong Song <yhs@fb.com> >> >> Looking at _perf_ioctl(), we also have perf_event_set_bpf_prog() >> there. It's basically under CONFIG_EVENT_TRACING, which later calls >> perf_event_attach_bpf_prog() which is under CONFIG_BPF_EVENTS, so >> where we have the dummy handler returning -EOPNOTSUPP when BPF >> events is not set. bpf_trace.c is only built when CONFIG_BPF_EVENTS >> is set and that by itself depends on BPF_SYSCALL already. So it would >> be more correct to do the same thing here ... >> >> #if defined(CONFIG_EVENT_TRACING) && defined(CONFIG_BPF_EVENTS) >> [...] > > +1 > #ifdef CONFIG_BPF_EVENTS > works, whereas CONFIG_EVENT_TRACING probably not, since kprobe > can be disabled independently which will turn off BPF_EVENTS > and body of bpf_event_query_prog_array() will be gone. I tested to enable/disable uprobe/kprobe/both and my patch works. But I did not test enable a non uprobe/kprobe tracing event (e.g., CONFIG_FUNCTION_TRACER) where CONFIG_TRACING and CONFIG_EVENT_TRACING is on but CONFIG_UPROBES_EVENT/CONFIG_KPROBES_EVENT is off and then my patch breaks. Looks like #ifdef CONFIG_BPF_EVENTS is suffice. This config will enable to include bpf_trace.c with the real definition. It will depend on KPROBE_EVENTS or UPROBE_EVENTS and either of them will enable CONFIG_TRACING and then CONFIG_EVENT_TRACING. Will resubmit the patch after testing.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 93e15b9..8dbbfd7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -254,7 +254,6 @@ typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src, u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size, void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy); -int bpf_event_query_prog_array(struct perf_event *event, void __user *info); int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, union bpf_attr __user *uattr); @@ -558,6 +557,15 @@ static inline int sock_map_prog(struct bpf_map *map, } #endif +#if defined(CONFIG_EVENT_TRACING) && defined(CONFIG_BPF_SYSCALL) +int bpf_event_query_prog_array(struct perf_event *event, void __user *info); +#else +static inline int bpf_event_query_prog_array(struct perf_event *event, void __user *info) +{ + return -EOPNOTSUPP; +} +#endif + /* verifier prototypes for helper functions called from eBPF programs */ extern const struct bpf_func_proto bpf_map_lookup_elem_proto; extern const struct bpf_func_proto bpf_map_update_elem_proto;
Commit f371b304f12e ("bpf/tracing: allow user space to query prog array on the same tp") introduced a perf ioctl command to query prog array attached to the same perf tracepoint. The commit introduced a compilation error when either CONFIG_BPF_SYSCALL or CONFIG_EVENT_TRACING is not defined: kernel/events/core.o: In function `perf_ioctl': core.c:(.text+0x98c4): undefined reference to `bpf_event_query_prog_array' This patch fixed this error. Fixes: f371b304f12e ("bpf/tracing: allow user space to query prog array on the same tp") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Yonghong Song <yhs@fb.com> --- include/linux/bpf.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)