On Mon, Jul 26, 2021 at 09:12:01AM -0700, Andrii Nakryiko wrote:
> Introduce a new type of BPF link - BPF perf link. This brings perf_event-based
> BPF program attachments (perf_event, tracepoints, kprobes, and uprobes) into
> the common BPF link infrastructure, allowing to list all active perf_event
> based attachments, auto-detaching BPF program from perf_event when link's FD
> is closed, get generic BPF link fdinfo/get_info functionality.
>
> BPF_LINK_CREATE command expects perf_event's FD as target_fd. No extra flags
> are currently supported.
>
> Force-detaching and atomic BPF program updates are not yet implemented, but
> with perf_event-based BPF links we now have common framework for this without
> the need to extend ioctl()-based perf_event interface.
>
> One interesting consideration is a new value for bpf_attach_type, which
> BPF_LINK_CREATE command expects. Generally, it's either 1-to-1 mapping from
> bpf_attach_type to bpf_prog_type, or many-to-1 mapping from a subset of
> bpf_attach_types to one bpf_prog_type (e.g., see BPF_PROG_TYPE_SK_SKB or
> BPF_PROG_TYPE_CGROUP_SOCK). In this case, though, we have three different
> program types (KPROBE, TRACEPOINT, PERF_EVENT) using the same perf_event-based
> mechanism, so it's many bpf_prog_types to one bpf_attach_type. I chose to
> define a single BPF_PERF_EVENT attach type for all of them and adjust
> link_create()'s logic for checking correspondence between attach type and
> program type.
>
> The alternative would be to define three new attach types (e.g., BPF_KPROBE,
> BPF_TRACEPOINT, and BPF_PERF_EVENT), but that seemed like unnecessary overkill
> and BPF_KPROBE will cause naming conflicts with BPF_KPROBE() macro, defined by
> libbpf. I chose to not do this to avoid unnecessary proliferation of
> bpf_attach_type enum values and not have to deal with naming conflicts.
>
So I have no idea what all that means... I don't speak BPF. That said,
the patch doesn't look terrible.
One little question below, but otherwise:
Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> +static void bpf_perf_link_release(struct bpf_link *link)
> +{
> + struct bpf_perf_link *perf_link = container_of(link, struct bpf_perf_link, link);
> + struct perf_event *event = perf_link->perf_file->private_data;
> +
> + perf_event_free_bpf_prog(event);
> + fput(perf_link->perf_file);
> +}
> +static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> +{
> + struct bpf_link_primer link_primer;
> + struct bpf_perf_link *link;
> + struct perf_event *event;
> + struct file *perf_file;
> + int err;
> +
> + if (attr->link_create.flags)
> + return -EINVAL;
> +
> + perf_file = perf_event_get(attr->link_create.target_fd);
> + if (IS_ERR(perf_file))
> + return PTR_ERR(perf_file);
> +
> + link = kzalloc(sizeof(*link), GFP_USER);
> + if (!link) {
> + err = -ENOMEM;
> + goto out_put_file;
> + }
> + bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
> + link->perf_file = perf_file;
> +
> + err = bpf_link_prime(&link->link, &link_primer);
> + if (err) {
> + kfree(link);
> + goto out_put_file;
> + }
> +
> + event = perf_file->private_data;
> + err = perf_event_set_bpf_prog(event, prog);
> + if (err) {
> + bpf_link_cleanup(&link_primer);
> + goto out_put_file;
> + }
> + /* perf_event_set_bpf_prog() doesn't take its own refcnt on prog */
Is that otherwise expected? AFAICT the previous users of that function
were guaranteed the existance of the BPF program. But afaict there is
nothing that prevents perf_event_*_bpf_prog() from doing the addition
refcounting if that is more convenient.
> + bpf_prog_inc(prog);
> +
> + return bpf_link_settle(&link_primer);
> +
> +out_put_file:
> + fput(perf_file);
> + return err;
> +}
