On Tue, Jul 27, 2021 at 2:04 AM Peter Zijlstra [off-list ref] wrote:

On Mon, Jul 26, 2021 at 09:12:01AM -0700, Andrii Nakryiko wrote:

quoted

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@infradead.org>

quoted

+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);
+}

quoted

+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.

Sorry, I missed this on my last pass. Yes, it's expected. The general
convention we use for BPF when passing bpf_prog (and bpf_map and other
objects like that) is that the caller already has an incremented
refcnt before calling callee. If callee succeeds, that refcnt is
"transferred" into the caller (so callee doesn't increment it, caller
doesn't put it). If callee errors out, caller is decrementing refcnt
after necessary clean up, but callee does nothing. While asymmetrical,
in practice it results in a simple and straightforward  error handling
logic.

In this case bpf_perf_link_attach() assumes one refcnt from its
caller, but if everything is ok and perf_event_set_bpf_prog()
succeeds, we need to keep 2 refcnts: one for bpf_link and one for
perf_event_set_bpf_prog() internally. So we just bump refcnt one extra
time. I intentionally removed bpf_prog_put() from
perf_event_set_bpf_prog() in the previous patch to make error handling
uniform with the rest of the code and simpler overall.

quoted

+     bpf_prog_inc(prog);
+
+     return bpf_link_settle(&link_primer);
+
+out_put_file:
+     fput(perf_file);
+     return err;
+}