Make perf_event_open() propagate errors for use in bpf_perf_event_open()
Subsystems (7)
Arnaldo Carvalho de Melo
maintainerHi Peter,
bpf_perf_event_open() already returns a value, but if
perf_event_output's output_begin (mostly perf_output_begin) fails, the only way to know about that is looking before/after the rb->lost, right?
For ring buffer users that is ok, we'll get a PERF_RECORD_LOST,
etc, but for BPF programs it would be convenient to get that -ENOSPC and do some fallback, whatever makes sense, like in my augmented_syscalls stuff for 'perf trace', i.e. don't augment it (i.e. push stuff at the end of the normal payload), just don't filter the raw_syscalls:sys_enter, 'perf trace' will get the enter syscall enter event without the pointer dereference at the end, etc, warn the user but don't lose a syscall in the strace-like output.
What do you think? Am I missing something? Probably ;-)
Ah, its just test built.
- Arnaldo
include/linux/perf_event.h +3 -3
@@ -978,9 +978,9 @@ extern void perf_event_output_forward(struct perf_event *event,extern void perf_event_output_backward (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs); -extern void perf_event_output (struct perf_event *event, -struct perf_sample_data *data, -struct pt_regs *regs); +extern int perf_event_output (struct perf_event *event, +struct perf_sample_data *data, +struct pt_regs *regs);static inline bool is_default_overflow_handler (struct perf_event *event)
kernel/events/core.c +7 -4
@@ -6489,7 +6489,7 @@ void perf_prepare_sample(struct perf_event_header *header, data->phys_addr =perf_virt_to_phys (data->addr); } -static __always_inline void +static __always_inline int __perf_event_output(struct perf_event *event, struct perf_sample_data *data, struct pt_regs *regs,
@@ -6499,13 +6499,15 @@ __perf_event_output(struct perf_event *event, {struct perf_output_handle handle;struct perf_event_header header; +int err;/* protect the callchain buffers */ rcu_read_lock ();perf_prepare_sample (&header, data, event, regs); -if (output_begin (&handle, event, header.size)) + err =output_begin (&handle, event, header.size); +if (err) goto exit;perf_output_sample (&handle, &header, data, event);
@@ -6514,6 +6516,7 @@ __perf_event_output(struct perf_event *event, exit:rcu_read_unlock (); +return err; }void
@@ -6532,12 +6535,12 @@ perf_event_output_backward(struct perf_event *event,__perf_event_output (event, data, regs, perf_output_begin_backward); } -void +int perf_event_output (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs) { -__perf_event_output (event, data, regs, perf_output_begin); +return __perf_event_output (event, data, regs, perf_output_begin); }/*
kernel/trace/bpf_trace.c +1 -2
@@ -431,8 +431,7 @@ __bpf_perf_event_output(struct pt_regs *regs, struct bpf_map *map,if (unlikely (event->oncpu != cpu))return -EOPNOTSUPP ; -perf_event_output (event, sd, regs); -return 0 ; +return perf_event_output (event, sd, regs); } BPF_CALL_5(bpf_perf_event_output, struct pt_regs *, regs, struct bpf_map *, map,
tools/perf/examples/bpf/augmented_raw_syscalls.c +2 -2
@@ -141,8 +141,8 @@ int sys_enter(struct syscall_enter_args *args) len =sizeof (augmented_args.args); } -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); -return 0 ; +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); }SEC ("raw_syscalls:sys_exit" )
tools/perf/examples/bpf/augmented_syscalls.c +7 -7
@@ -55,9 +55,9 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ len -=sizeof (augmented_args.filename.value) - augmented_args.filename.size; \ len &=sizeof (augmented_args.filename.value) -1 ; \ } \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, len); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, len); \ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
@@ -125,10 +125,10 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \/* addrlen = augmented_args.args.addrlen; */ \/* */ \probe_read (&augmented_args.addr, addrlen, args->addr_ptr); \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ -sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ +sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen);\ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
tools/perf/examples/bpf/etcsnoop.c +5 -5
@@ -49,11 +49,11 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ args->filename_ptr); \if (__builtin_memcmp (augmented_args.filename.value, etc,4 ) !=0 ) \return 0 ; \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ - (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ - augmented_args.filename.size)); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ + (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ + augmented_args.filename.size)); \ }struct syscall_enter_openat_args {
Jamal Hadi Salim
Tested-byQuoted 16 lines · Arnaldo Carvalho de Melo
Hi Peter,
bpf_perf_event_open()already returns a value, but if
perf_event_output'soutput_begin(mostlyperf_output_begin) fails, the only way to know about that is looking before/after therb->lost, right?For ring buffer users that is ok, we'll get a
PERF_RECORD_LOST,
etc, but for BPF programs it would be convenient to get that-ENOSPCand do some fallback, whatever makes sense, like in myaugmented_syscallsstuff for 'perf trace', i.e. don't augment it (i.e. push stuff at the end of the normal payload), just don't filter the raw_syscalls:sys_enter, 'perf trace' will get the enter syscall enter event without the pointer dereference at the end, etc, warn the user but don't lose a syscall in the strace-like output.
Quoted 3 lines
What do you think? Am I missing something? Probably ;-)
Ah, its just test built.
Works as advertised ;->
cheers,
jamal
Peter Zijlstra
maintainerQuoted 16 lines · Arnaldo Carvalho de Melo
Hi Peter,
bpf_perf_event_open()already returns a value, but if
perf_event_output'soutput_begin(mostlyperf_output_begin) fails, the only way to know about that is looking before/after therb->lost, right?For ring buffer users that is ok, we'll get a
PERF_RECORD_LOST,
etc, but for BPF programs it would be convenient to get that-ENOSPCand do some fallback, whatever makes sense, like in myaugmented_syscallsstuff for 'perf trace', i.e. don't augment it (i.e. push stuff at the end of the normal payload), just don't filter the raw_syscalls:sys_enter, 'perf trace' will get the enter syscall enter event without the pointer dereference at the end, etc, warn the user but don't lose a syscall in the strace-like output.
Quoted 1 line
What do you think? Am I missing something? Probably ;-)
Seems ok to do this.
Em Fri, Jan 18, 2019 at 03:52:37PM +0100, Peter Zijlstra escreveu:
Quoted 17 lines · Arnaldo Carvalho de Melo
Quoted 16 lines
Hi Peter,
bpf_perf_event_open()already returns a value, but if
perf_event_output'soutput_begin(mostlyperf_output_begin) fails, the only way to know about that is looking before/after therb->lost, right?For ring buffer users that is ok, we'll get a
PERF_RECORD_LOST,
etc, but for BPF programs it would be convenient to get that-ENOSPCand do some fallback, whatever makes sense, like in myaugmented_syscallsstuff for 'perf trace', i.e. don't augment it (i.e. push stuff at the end of the normal payload), just don't filter the raw_syscalls:sys_enter, 'perf trace' will get the enter syscall enter event without the pointer dereference at the end, etc, warn the user but don't lose a syscall in the strace-like output.
Quoted 3 lines
Quoted 1 line
What do you think? Am I missing something? Probably ;-)
Seems ok to do this.
Can I take that as an Acked-by?
This is the patch with a cset comment, if you're ok, I'll put it together with Song's PERF_RECORD_{KSYMBOL,BPF_EVENT} into my perf/core branch and send to Ingo,
please advise,
- Arnaldo
commit 1b3b3dee572d0b77a316ab6715091201be6832de
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Fri Jan 11 13:20:20 2019 -0300
perf: Make perf_event_output() propagate the output() return
For the original mode of operation it isn't needed, since we report back errors via PERF_RECORD_LOST records in the ring buffer, but for use in bpf_perf_event_output() it is convenient to return the errors, basically -ENOSPC.
Currently bpf_perf_event_output() returns an error indication, the last thing it does, which is to push it to the ring buffer is that can fail and if so, this failure won't be reported back to its users, fix it.
include/linux/perf_event.h +3 -3
@@ -978,9 +978,9 @@ extern void perf_event_output_forward(struct perf_event *event,extern void perf_event_output_backward (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs); -extern void perf_event_output (struct perf_event *event, -struct perf_sample_data *data, -struct pt_regs *regs); +extern int perf_event_output (struct perf_event *event, +struct perf_sample_data *data, +struct pt_regs *regs);static inline bool is_default_overflow_handler (struct perf_event *event)
kernel/events/core.c +7 -4
@@ -6489,7 +6489,7 @@ void perf_prepare_sample(struct perf_event_header *header, data->phys_addr =perf_virt_to_phys (data->addr); } -static __always_inline void +static __always_inline int __perf_event_output(struct perf_event *event, struct perf_sample_data *data, struct pt_regs *regs,
@@ -6499,13 +6499,15 @@ __perf_event_output(struct perf_event *event, {struct perf_output_handle handle;struct perf_event_header header; +int err;/* protect the callchain buffers */ rcu_read_lock ();perf_prepare_sample (&header, data, event, regs); -if (output_begin (&handle, event, header.size)) + err =output_begin (&handle, event, header.size); +if (err) goto exit;perf_output_sample (&handle, &header, data, event);
@@ -6514,6 +6516,7 @@ __perf_event_output(struct perf_event *event, exit:rcu_read_unlock (); +return err; }void
@@ -6532,12 +6535,12 @@ perf_event_output_backward(struct perf_event *event,__perf_event_output (event, data, regs, perf_output_begin_backward); } -void +int perf_event_output (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs) { -__perf_event_output (event, data, regs, perf_output_begin); +return __perf_event_output (event, data, regs, perf_output_begin); }/*
kernel/trace/bpf_trace.c +1 -2
@@ -431,8 +431,7 @@ __bpf_perf_event_output(struct pt_regs *regs, struct bpf_map *map,if (unlikely (event->oncpu != cpu))return -EOPNOTSUPP ; -perf_event_output (event, sd, regs); -return 0 ; +return perf_event_output (event, sd, regs); } BPF_CALL_5(bpf_perf_event_output, struct pt_regs *, regs, struct bpf_map *, map,
tools/perf/examples/bpf/augmented_raw_syscalls.c +2 -2
@@ -141,8 +141,8 @@ int sys_enter(struct syscall_enter_args *args) len =sizeof (augmented_args.args); } -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); -return 0 ; +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); }SEC ("raw_syscalls:sys_exit" )
tools/perf/examples/bpf/augmented_syscalls.c +7 -7
@@ -55,9 +55,9 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ len -=sizeof (augmented_args.filename.value) - augmented_args.filename.size; \ len &=sizeof (augmented_args.filename.value) -1 ; \ } \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, len); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, len); \ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
@@ -125,10 +125,10 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \/* addrlen = augmented_args.args.addrlen; */ \/* */ \probe_read (&augmented_args.addr, addrlen, args->addr_ptr); \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ -sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ +sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen);\ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
tools/perf/examples/bpf/etcsnoop.c +5 -5
@@ -49,11 +49,11 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ args->filename_ptr); \if (__builtin_memcmp (augmented_args.filename.value, etc,4 ) !=0 ) \return 0 ; \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ - (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ - augmented_args.filename.size)); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ + (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ + augmented_args.filename.size)); \ }struct syscall_enter_openat_args {
Quoted 15 lines · Arnaldo Carvalho de Melo
commit
1b3b3dee572d0b77a316ab6715091201be6832deAuthor: Arnaldo Carvalho de Melo <[email protected]> Date: Fri Jan 11 13:20:20 2019 -0300perf: Make
perf_event_output()propagate theoutput()returnFor the original mode of operation it isn't needed, since we report back errors via
PERF_RECORD_LOSTrecords in the ring buffer, but for use inbpf_perf_event_output()it is convenient to return the errors, basically-ENOSPC.Currently
bpf_perf_event_output()returns an error indication, the last thing it does, which is to push it to the ring buffer is that can fail and if so, this failure won't be reported back to its users, fix it.
Quoted 10 lines
Reported-by Jamal Hadi Salim <[email protected]>Tested-by Jamal Hadi Salim <[email protected]>Cc Adrian Hunter <[email protected]>Cc Alexei Starovoitov <[email protected]>Cc Daniel Borkmann <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Peter Zijlstra <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
/me hates on git-format :-)
tip-bot for Arnaldo Carvalho de Melo
merge bot[tip:perf/core] perf: Make perf_event_output() propagate the output() return
In reply to Arnaldo Carvalho de Meloperf: Make perf_event_output() propagate the output() return
Commit-ID: 5620196951192f7cd2da0a04e7c0113f40bfc14e Gitweb: https://git.kernel.org/tip/5620196951192f7cd2da0a04e7c0113f40bfc14e Author: Arnaldo Carvalho de Melo <[email protected]> AuthorDate: Fri, 11 Jan 2019 13:20:20 -0300 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Mon, 21 Jan 2019 17:00:57 -0300
perf: Make perf_event_output() propagate the output() return
For the original mode of operation it isn't needed, since we report back errors via PERF_RECORD_LOST records in the ring buffer, but for use in bpf_perf_event_output() it is convenient to return the errors, basically -ENOSPC.
Currently bpf_perf_event_output() returns an error indication, the last thing it does, which is to push it to the ring buffer is that can fail and if so, this failure won't be reported back to its users, fix it.
--- include/linux/perf_event.h | 6 +++--- kernel/events/core.c | 11 +++++++---- kernel/trace/bpf_trace.c | 3 +-- tools/perf/examples/bpf/augmented_raw_syscalls.c | 4 ++-- tools/perf/examples/bpf/augmented_syscalls.c | 14 +++++++------- tools/perf/examples/bpf/etcsnoop.c | 10 +++++----- 6 files changed, 25 insertions(+), 23 deletions(-)
include/linux/perf_event.h +3 -3
@@ -978,9 +978,9 @@ extern void perf_event_output_forward(struct perf_event *event,extern void perf_event_output_backward (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs); -extern void perf_event_output (struct perf_event *event, -struct perf_sample_data *data, -struct pt_regs *regs); +extern int perf_event_output (struct perf_event *event, +struct perf_sample_data *data, +struct pt_regs *regs);static inline bool is_default_overflow_handler (struct perf_event *event)
kernel/events/core.c +7 -4
@@ -6489,7 +6489,7 @@ void perf_prepare_sample(struct perf_event_header *header, data->phys_addr =perf_virt_to_phys (data->addr); } -static __always_inline void +static __always_inline int __perf_event_output(struct perf_event *event, struct perf_sample_data *data, struct pt_regs *regs,
@@ -6499,13 +6499,15 @@ __perf_event_output(struct perf_event *event, {struct perf_output_handle handle;struct perf_event_header header; +int err;/* protect the callchain buffers */ rcu_read_lock ();perf_prepare_sample (&header, data, event, regs); -if (output_begin (&handle, event, header.size)) + err =output_begin (&handle, event, header.size); +if (err) goto exit;perf_output_sample (&handle, &header, data, event);
@@ -6514,6 +6516,7 @@ __perf_event_output(struct perf_event *event, exit:rcu_read_unlock (); +return err; }void
@@ -6532,12 +6535,12 @@ perf_event_output_backward(struct perf_event *event,__perf_event_output (event, data, regs, perf_output_begin_backward); } -void +int perf_event_output (struct perf_event *event,struct perf_sample_data *data,struct pt_regs *regs) { -__perf_event_output (event, data, regs, perf_output_begin); +return __perf_event_output (event, data, regs, perf_output_begin); }/*
kernel/trace/bpf_trace.c +1 -2
@@ -431,8 +431,7 @@ __bpf_perf_event_output(struct pt_regs *regs, struct bpf_map *map,if (unlikely (event->oncpu != cpu))return -EOPNOTSUPP ; -perf_event_output (event, sd, regs); -return 0 ; +return perf_event_output (event, sd, regs); } BPF_CALL_5(bpf_perf_event_output, struct pt_regs *, regs, struct bpf_map *, map,
tools/perf/examples/bpf/augmented_raw_syscalls.c +2 -2
@@ -141,8 +141,8 @@ int sys_enter(struct syscall_enter_args *args) len =sizeof (augmented_args.args); } -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); -return 0 ; +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , &augmented_args, len); }SEC ("raw_syscalls:sys_exit" )
tools/perf/examples/bpf/augmented_syscalls.c +7 -7
@@ -55,9 +55,9 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ len -=sizeof (augmented_args.filename.value) - augmented_args.filename.size; \ len &=sizeof (augmented_args.filename.value) -1 ; \ } \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, len); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, len); \ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
@@ -125,10 +125,10 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \/* addrlen = augmented_args.args.addrlen; */ \/* */ \probe_read (&augmented_args.addr, addrlen, args->addr_ptr); \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ -sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ +sizeof (augmented_args) -sizeof (augmented_args.addr) + addrlen);\ } \int syscall_exit (syscall )(struct syscall_exit_args *args) \ { \
tools/perf/examples/bpf/etcsnoop.c +5 -5
@@ -49,11 +49,11 @@ int syscall_enter(syscall)(struct syscall_enter_##syscall##_args *args) \ args->filename_ptr); \if (__builtin_memcmp (augmented_args.filename.value, etc,4 ) !=0 ) \return 0 ; \ -perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ - &augmented_args, \ - (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ - augmented_args.filename.size)); \ -return 0 ; \ +/* If perf_event_output fails, return non-zero so that it gets recorded unaugmented */ \ +return perf_event_output (args, &__augmented_syscalls__,BPF_F_CURRENT_CPU , \ + &augmented_args, \ + (sizeof (augmented_args) -sizeof (augmented_args.filename.value) + \ + augmented_args.filename.size)); \ }struct syscall_enter_openat_args {