link: close perfEventLink.fd before perf event in Close() #918
Conversation
|
One legitimate CI failure: https://ebpf.semaphoreci.com/jobs/97ac8e96-14ff-470f-8f67-6e581e07bf07#L756 |
link/perf_event.go
Outdated
| return fmt.Errorf("perf link close: %w", err) | ||
| } | ||
|
|
||
| // This will fail with EBUSY if the link above was a pinned bpf_link. |
There was a problem hiding this comment.
From the code it seems like we don't support pinned perf events?
There was a problem hiding this comment.
based on the commit message, the comment looks good if s/pinned/tracefs-backed
There was a problem hiding this comment.
@lmb Perf events cannot be pinned, they are not bpf resources. Edit: You probably meant perfEventLink.Pin() returning ErrNotSupported.
@mmat11 I'll make the comment a bit more elaborate, I really do mean 'pinned'. Closing a bpf_link's fd doesn't detach its event.
Edit: I'll remove the comment, was thinking too far: 'how could we end up successfully closing perfEventLink and receive EBUSY from pl.pe.Close()?' -> if we're dealing with a pinned perfEventLink, but we don't support that. It's technically possible to pin a pmu-backed bpf_link, but there's no way to persist the pmu past the lifetime of the app, so the link would likely go defunct after app exit anyway.
And so far we only create bpf_link based on pmu perf event? Is the behaviour OK for link based on pmu?
In general, or just for MaxActive > 0? If the latter it would be good to reword, the former interpretation would be pretty bad. |
If a Kretprobe is created with RetprobeMaxActive > 0 on a kernel supporting bpf_link, a bpf_link is created based on a tracefs-backed perf event. perfEventLink.Close() would try to remove the tracefs entry first, resulting in EBUSY because of the open link. This error, in turn, leaked the bpf_link fd. This also caused perf events to never be removed from tracefs/kprobe_events if they were created with maxactive > 0. This commit closes the link and the perf event in the correct order. Signed-off-by: Timo Beckers <timo@isovalent.com>
Yes, that happened to be the case because kernels supporting bpf_link generally also support kprobe PMUs.
Yes, closing a PMU that has an active bpf_link attached works just fine.
Yes, only when maxactive > 0. Reworded. |
ti-mo
force-pushed
the
tb/perfeventlink-leak
branch
from
534a63a
to
653d80d
Compare
If a Kretprobe is created with RetprobeMaxActive > 0 on a kernel supporting bpf_link, a bpf_link is created based on a tracefs-backed perf event.
perfEventLink.Close() would try to remove the tracefs entry first, resulting in EBUSY because of the open link. This error, in turn, leaked the bpf_link fd. This also caused perf events to never be removed from tracefs/kprobe_events.
This commit closes the link and the perf event in the correct order.
Found by the fd tracer in #732.
Unfortunately, this also means pinning a retprobe link with maxactive set means tracefs entries will leak by design. The only real solution I see is #842.