[PATCH] libbpf: Use the correct fd when attaching to perf events

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Fixes: 63f2f5ee856ba ("libbpf: add ability to attach/detach BPF program to perf event")
Signed-off-by: Sultan Alsawaf <redacted>
---
 tools/lib/bpf/libbpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d43cc3f29dae..3d20d57d4af5 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -9538,7 +9538,7 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
 	if (!link)
 		return ERR_PTR(-ENOMEM);
 	link->detach = &bpf_link__detach_perf_event;
-	link->fd = pfd;
+	link->fd = prog_fd;
 
 	if (ioctl(pfd, PERF_EVENT_IOC_SET_BPF, prog_fd) < 0) {
 		err = -errno;

-- 
2.30.2

On Fri, Mar 12, 2021 at 1:43 PM Sultan Alsawaf [off-list ref] wrote:

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Why? Can you elaborate on what issue you ran into with the current code?

On Fri, Mar 12, 2021 at 05:31:14PM -0800, Andrii Nakryiko wrote:

On Fri, Mar 12, 2021 at 1:43 PM Sultan Alsawaf [off-list ref] wrote:

quoted

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Why? Can you elaborate on what issue you ran into with the current code?

bpf_link__pin() would fail with -EINVAL when using tracepoints, kprobes, or
uprobes. The failure would happen inside the kernel, in bpf_link_get_from_fd()
right here:
	if (f.file->f_op != &bpf_link_fops) {
		fdput(f);
		return ERR_PTR(-EINVAL);
	}

Since bpf wasn't looking for the perf event fd, I swapped it for the program fd
and bpf_link__pin() worked.

Sultan

On Fri, Mar 12, 2021 at 6:22 PM Sultan Alsawaf [off-list ref] wrote:

On Fri, Mar 12, 2021 at 05:31:14PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Mar 12, 2021 at 1:43 PM Sultan Alsawaf [off-list ref] wrote:

quoted

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Why? Can you elaborate on what issue you ran into with the current code?

bpf_link__pin() would fail with -EINVAL when using tracepoints, kprobes, or
uprobes. The failure would happen inside the kernel, in bpf_link_get_from_fd()
right here:
        if (f.file->f_op != &bpf_link_fops) {
                fdput(f);
                return ERR_PTR(-EINVAL);
        }

kprobe/tracepoint/perf_event attachments behave like bpf_link (so
libbpf uses user-space high-level bpf_link APIs for it), but they are
not bpf_link-based in the kernel. So bpf_link__pin() won't work for
such types of programs until we actually have bpf_link-backed
attachment support in the kernel itself. I never got to implementing
this because we already had auto-detachment properties from perf_event
FD itself. But it would be nice to have that done as a real bpf_link
in the kernel (with all the observability, program update,
force-detach support).

Looking for volunteers to make this happen ;)

Since bpf wasn't looking for the perf event fd, I swapped it for the program fd
and bpf_link__pin() worked.

But you were pinning the BPF program, not a BPF link. Which is not
what should have happen.

Sultan

On Fri, Mar 12, 2021 at 06:33:01PM -0800, Andrii Nakryiko wrote:

On Fri, Mar 12, 2021 at 6:22 PM Sultan Alsawaf [off-list ref] wrote:

quoted

On Fri, Mar 12, 2021 at 05:31:14PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Mar 12, 2021 at 1:43 PM Sultan Alsawaf [off-list ref] wrote:

quoted

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Why? Can you elaborate on what issue you ran into with the current code?

bpf_link__pin() would fail with -EINVAL when using tracepoints, kprobes, or
uprobes. The failure would happen inside the kernel, in bpf_link_get_from_fd()
right here:
        if (f.file->f_op != &bpf_link_fops) {
                fdput(f);
                return ERR_PTR(-EINVAL);
        }

kprobe/tracepoint/perf_event attachments behave like bpf_link (so
libbpf uses user-space high-level bpf_link APIs for it), but they are
not bpf_link-based in the kernel. So bpf_link__pin() won't work for
such types of programs until we actually have bpf_link-backed
attachment support in the kernel itself. I never got to implementing
this because we already had auto-detachment properties from perf_event
FD itself. But it would be nice to have that done as a real bpf_link
in the kernel (with all the observability, program update,
force-detach support).

Looking for volunteers to make this happen ;)

quoted

Since bpf wasn't looking for the perf event fd, I swapped it for the program fd
and bpf_link__pin() worked.

But you were pinning the BPF program, not a BPF link. Which is not
what should have happen.

This is the code in question:
	link = bpf_program__attach(prog);
	// make sure `link` is valid, blah blah...
	bpf_link__pin(link, some_path);

Are you saying that this usage is incorrect?

Sultan

On Fri, Mar 12, 2021 at 6:43 PM Sultan Alsawaf [off-list ref] wrote:

On Fri, Mar 12, 2021 at 06:33:01PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Mar 12, 2021 at 6:22 PM Sultan Alsawaf [off-list ref] wrote:

quoted

On Fri, Mar 12, 2021 at 05:31:14PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Mar 12, 2021 at 1:43 PM Sultan Alsawaf [off-list ref] wrote:

quoted

From: Sultan Alsawaf <redacted>

We should be using the program fd here, not the perf event fd.

Why? Can you elaborate on what issue you ran into with the current code?

bpf_link__pin() would fail with -EINVAL when using tracepoints, kprobes, or
uprobes. The failure would happen inside the kernel, in bpf_link_get_from_fd()
right here:
        if (f.file->f_op != &bpf_link_fops) {
                fdput(f);
                return ERR_PTR(-EINVAL);
        }

kprobe/tracepoint/perf_event attachments behave like bpf_link (so
libbpf uses user-space high-level bpf_link APIs for it), but they are
not bpf_link-based in the kernel. So bpf_link__pin() won't work for
such types of programs until we actually have bpf_link-backed
attachment support in the kernel itself. I never got to implementing
this because we already had auto-detachment properties from perf_event
FD itself. But it would be nice to have that done as a real bpf_link
in the kernel (with all the observability, program update,
force-detach support).

Looking for volunteers to make this happen ;)

quoted

Since bpf wasn't looking for the perf event fd, I swapped it for the program fd
and bpf_link__pin() worked.

But you were pinning the BPF program, not a BPF link. Which is not
what should have happen.

This is the code in question:
        link = bpf_program__attach(prog);
        // make sure `link` is valid, blah blah...
        bpf_link__pin(link, some_path);

Are you saying that this usage is incorrect?

Right, for kprobe/tracepoint/perf_event attachments it's not
supported. cgroup, xdp, raw_tracepoint and
fentry/fexit/fmod_ret/freplace (and a few more) attachments are
bpf_links in the kernel, so it works for them.

Sultan