On Fri, Aug 26, 2016 at 07:31:22PM -0700, Alexei Starovoitov wrote:
> +static int perf_event_set_bpf_handler(struct perf_event *event, u32 prog_fd)
> +{
> + struct bpf_prog *prog;
> +
> + if (event->overflow_handler_context)
> + /* hw breakpoint or kernel counter */
> + return -EINVAL;
> +
> + if (event->prog)
> + return -EEXIST;
> +
> + prog = bpf_prog_get_type(prog_fd, BPF_PROG_TYPE_PERF_EVENT);
> + if (IS_ERR(prog))
> + return PTR_ERR(prog);
> +
> + event->prog = prog;
> + event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
> + WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
> + return 0;
> +}
> +
> +static void perf_event_free_bpf_handler(struct perf_event *event)
> +{
> + struct bpf_prog *prog = event->prog;
> +
> + if (!prog)
> + return;
Does it make sense to do something like:
WARN_ON_ONCE(event->overflow_handler != bpf_overflow_handler);
?
> +
> + WRITE_ONCE(event->overflow_handler, event->orig_overflow_handler);
> + event->prog = NULL;
> + bpf_prog_put(prog);
> +}
> static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
> {
> bool is_kprobe, is_tracepoint;
> struct bpf_prog *prog;
>
> + if (event->attr.type == PERF_TYPE_HARDWARE ||
> + event->attr.type == PERF_TYPE_SOFTWARE)
> + return perf_event_set_bpf_handler(event, prog_fd);
> +
> if (event->attr.type != PERF_TYPE_TRACEPOINT)
> return -EINVAL;
>
> @@ -7647,6 +7711,8 @@ static void perf_event_free_bpf_prog(struct perf_event
> *event)
> {
> struct bpf_prog *prog;
>
> + perf_event_free_bpf_handler(event);
> +
> if (!event->tp_event)
> return;
>
Does it at all make sense to merge the tp_event->prog thing into this
new event->prog?
> #ifdef CONFIG_HAVE_HW_BREAKPOINT
> @@ -8957,6 +9029,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> if (!overflow_handler && parent_event) {
> overflow_handler = parent_event->overflow_handler;
> context = parent_event->overflow_handler_context;
> + if (overflow_handler == bpf_overflow_handler) {
> + event->prog = bpf_prog_inc(parent_event->prog);
> + event->orig_overflow_handler =
> parent_event->orig_overflow_handler;
> + if (IS_ERR(event->prog)) {
> + event->prog = NULL;
> + overflow_handler = NULL;
> + }
> + }
> }
Should we not fail the entire perf_event_alloc() call in that IS_ERR()
case?
