devices: drop cilium/ebpf{,link} deps - #64
Conversation
d5f40e9 to
733c596
Compare
…ranch Temporarily point the opencontainers/cgroups dependency at the drop-cilium-ebpf branch (opencontainers/cgroups#64) via a go.mod replace, and re-vendor, so CI can exercise the cilium/ebpf main+link package removal end-to-end in runc. This must not be merged: the replace directive points at a personal fork branch. Once opencontainers/cgroups#64 lands and is tagged, this should be replaced by a normal dependency bump. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
733c596 to
cf4738d
Compare
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the devices eBPF cgroup-device filter implementation to stop importing github.com/cilium/ebpf’s main and link packages, and instead perform the required operations via direct bpf(2) syscalls while continuing to use github.com/cilium/ebpf/asm for instruction assembly. This aligns with the stated goal of reducing consumer binary size by avoiding heavy transitive dependencies.
Changes:
- Replaced
cilium/ebpfprogram/link usage with thin wrappers aroundBPF_PROG_*commands and raw program fds. - Updated cgroup device filter attach/query logic to operate on fds (with explicit closes) instead of
*ebpf.Program. - Added
nativeEndianselection via build-tagged endian-specific files to satisfyasm.Instructions.Marshalrequirements.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go.sum | Removes now-unused transitive dependencies after dropping cilium/ebpf main/link usage. |
| devices/endian_le.go | Provides nativeEndian = binary.LittleEndian under little-endian arch build tags. |
| devices/endian_be.go | Provides nativeEndian = binary.BigEndian under big-endian arch build tags. |
| devices/ebpf_linux.go | Replaces ebpf/link usage with direct bpf(2) syscall wrappers and fd-based program management. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err = bpfProgAttach(dirFd, progFd, attachFlags, replaceFd) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to call BPF_PROG_ATTACH (BPF_CGROUP_DEVICE, BPF_F_ALLOW_MULTI): %w", err) | ||
| } |
cf4738d to
0fd3600
Compare
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
0fd3600 to
8decd05
Compare
There was a problem hiding this comment.
@kolyshkin nice, thanks! Did you c&p the definitions? In that case, can you link them so it's simpler to review?
Also, don't we have overlapping functionality in the cgroups package?
Also, I guess no go.mod changes because it is used for the asm part? Nice that the size is decreased anyways :)
Again I'm afraid I fail to understand what you mean here @rata, can you please elaborate or point to whatever you have in mind? |
8decd05 to
ae8789d
Compare
I did not, I just recreated a bare minimum (poor boy) version of functionality that we used from cilium/ebpf and cilium/ebpf/link, basically wiring I have added a separate second commit, linking to the original cilium/ebpf functions. Let me know if you want it or not so I will squash or remove it. |
|
@AkihiroSuda @thaJeztah PTAL (I think I've addressed all of your comments). The second commit is optional and can either be squashed or removed. |
kolyshkin
left a comment
There was a problem hiding this comment.
Looking into kernel's tools/lib/bpf/bpf.c, I see that BPF_PROG_LOAD is retried up to 5 times when EAGAIN is received. Also, cilium/ebpf does that (indefinitely).
Implemented the same.
3779064 to
e52ab80
Compare
e52ab80 to
eba30e4
Compare
|
@thaJeztah @rata @cyphar PTAL |
@rata please let me know what you want to do here; I'm fine either way |
There was a problem hiding this comment.
@kolyshkin Sorry I missed the notifications. I spent quite some time to review this now, to compensate :)
It seems there are quite some subtleties to replace cilium :-/
| continue | ||
| } | ||
|
|
||
| return r1, os.NewSyscallError("bpf", err) |
There was a problem hiding this comment.
os.NewSyscallError() returns syscall.Errno, not an error type. Don't callers of this function always see error is not nil?
There was a problem hiding this comment.
No, it returns an error (for the last 15 years or so). It also returns nil if err == nil.
https://github.com/golang/go/blob/591ed42daf93672b56c786e89272425ffca7dca4/src/os/error.go#L64-L72
There was a problem hiding this comment.
Ah! You probably meant that unix.Syscall returns syscall.Errno not error, and thus it needs to be handled correctly (can't compare to nil, need to compare to 0).
Just found it myself (when testing in opencontainers/runc#5340) and fixed.
There was a problem hiding this comment.
Yes, exactly that! Sorry I wasn't clear :)
|
|
||
| func findAttachedCgroupDeviceFilters(dirFd int) (_ []*ebpf.Program, retErr error) { | ||
| func bpf(cmd uintptr, attr unsafe.Pointer, size uintptr) (uintptr, error) { | ||
| for { |
There was a problem hiding this comment.
I think we should bound this loop. The verifier returns EAGAIN if there are pending signals: torvalds/linux@c349480
It seems cilium has a workaround for this: https://github.com/cilium/ebpf/blob/v0.17.3/internal/sys/syscall.go#L18-L24
I think we should at least bound the loop, but probably handle it properly as cilium does. What do you think? Am I missing something?
There was a problem hiding this comment.
Yes I saw that code in cilium/ebpf and decided to omit it as no one uses profiler in production.
Maybe I was wrong.
There was a problem hiding this comment.
I guess maybe some software like kubernetes might use oc/cgroups/devices (currently it does not -- the only user is runc) and have pprof enabled.
Implemented the same "disable SIGPROF when using BPF_PROG_LOAD" approach by copying code from cilium/ebpf.
There was a problem hiding this comment.
Because it is any signal, can't the go runtime trigger this also? It sends SIGUSR to preempt and schedule goroutines.
I think I'd still bound the loop, if it doesn't work in a few retries, we can fail and let the higher level container creation be started again. We don't want an infinite loop on any circumstances.
|
|
||
| // The load failed. Retry with the verifier log enabled so we can include | ||
| // it in the error (the first attempt skips it, as it is the fast path). | ||
| log := make([]byte, 64*1024) |
There was a problem hiding this comment.
If this is still small, it seems in some kernels it can still return a non useful error. Cilium seems to retry until ENOSPC is not returned: https://github.com/cilium/ebpf/blob/v0.17.3/prog.go#L439-L443
Shall we add some bounded retry mechanism here?
| logrus.WithFields(logrus.Fields{ | ||
| "fd": oldFd, | ||
| }).Logf(logLevel, "removing old filter %d from cgroup", idx) |
There was a problem hiding this comment.
Not sure logging the fd is useful here. Before we had quite some data to log. Can't we recover some?
There was a problem hiding this comment.
Agree that listing fd is useless. TBH everything else we printed is kind of useless, too; for example:
=== RUN TestFreeze
=== RUN TestFreeze/SystemdViaSet
time="2026-07-26T05:13:46Z" level=info msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 0 from cgroup" id=101 name= run_count=0 runtime=0s tag=531db05b114e9af3 type=CGroupDevice
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 1 from cgroup" id=102 name=sd_devices run_count=0 runtime=0s tag=3b0b81b071f088cd type=CGroupDevice
time="2026-07-26T05:13:46Z" level=info msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 0 from cgroup" id=103 name= run_count=0 runtime=0s tag=531db05b114e9af3 type=CGroupDevice
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 1 from cgroup" id=104 name=sd_devices run_count=0 runtime=0s tag=3b0b81b071f088cd type=CGroupDevice
=== RUN TestFreeze/SystemdViaPauseResume
=== RUN TestFreeze/FSViaSet
time="2026-07-26T05:13:46Z" level=info msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 0 from cgroup" id=108 name= run_count=0 runtime=0s tag=531db05b114e9af3 type=CGroupDevice
time="2026-07-26T05:13:46Z" level=info msg="removing old filter 1 from cgroup" id=109 name= run_count=0 runtime=0s tag=531db05b114e9af3 type=CGroupDevice
=== RUN TestFreeze/FSViaPauseResume
So maybe we should just remove this logging entirely? WDYT @cyphar ?
There was a problem hiding this comment.
Removed the "removing old filter ..." log message for now.
eba30e4 to
17c1710
Compare
|
@rata thanks a lot for your review, I think I addressed all your comments; PTAL |
17c1710 to
9b49352
Compare
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Replace the use of the cilium/ebpf and cilium/ebpf/link with direct bpf(2) syscalls. Keep cilium/ebpf/asm for instruction assembly. Notes: - the eBPF device-filter programs are now tracked by raw file descriptors instead of *ebpf.Program handles; - asm.Instructions.Marshal requires a concrete binary.LittleEndian or binary.BigEndian, so endian.go detects the native byte order at runtime as a workaround. This could be done during compile time but requires maintaining a list of all GOARCHes; - the "removing old filter %d from cgroup" log messages are removed: this always happens when using systemd and the messages are not useful, plus obtaining the details would add more code; This reduces the runc binary size by about ~1M. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Link to cilium/ebpf analogs. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
9b49352 to
9da2685
Compare
Let's drop the second biggest (by size) runc dependency and reduce the binary size by another ~1MB. Draft/DNM until opencontainers/cgroups#64 is merged/released. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
rata
left a comment
There was a problem hiding this comment.
@kolyshkin thanks! Left another round of comments. It's definitely tricky, I haven't looked into all of it yet
| // bpf(2) treats fd 0 as unset, so we need to dup it. | ||
| newFD, err := unix.FcntlInt(fd, unix.F_DUPFD_CLOEXEC, 1) |
There was a problem hiding this comment.
Sorry, I don't follow this. Why do we need to dup it unconditionally?
| if err == nil { | ||
| return fd, nil | ||
| } |
There was a problem hiding this comment.
Isn't this a rather unexpected thing to happen? This failed before without logs, we are trying to run it again with log, to give a proper error. I'm unsure what we should do if this happens, shall we say all is fine? Or shall we just fail saying we couldn't get a log of the failure?
Sometimes I think the latter is better, so we can't get tricked into something completely unexpected (and have a security bug, maybe?). And if it's a problem, we can always change it and backport it.
What do you think?
| const minLogSize = 64 * 1024 | ||
| // If the log does not fit into the buffer, the kernel returns ENOSPC, in | ||
| // which case retry with a bigger buffer, up to maxLogSize (the kernel | ||
| // rejects a log_size larger than UINT_MAX >> 8 anyway). |
There was a problem hiding this comment.
It seems newer kernels support a bigger buffer (https://github.com/torvalds/linux/blob/v5.2/kernel/bpf/verifier.c#L8215). I think it's compeltely fine to keep it to 16MB, but I'd amend the comment :D
|
|
||
| fd, err = bpfFD(unix.BPF_PROG_LOAD, unsafe.Pointer(&attr), unsafe.Sizeof(attr)) | ||
| runtime.KeepAlive(insnsBytes) | ||
| runtime.KeepAlive(licensePtr) |
There was a problem hiding this comment.
Don't we need to keep alive log too?
| // The code below is copied from | ||
| // github.com/cilium/ebpf/internal/sys/signals.go@v0.22.0. |
There was a problem hiding this comment.
MIT requires some attribution, I think, but not sure if this code is a "significant portion". Shall we add them?
|
|
||
| func findAttachedCgroupDeviceFilters(dirFd int) (_ []*ebpf.Program, retErr error) { | ||
| func bpf(cmd uintptr, attr unsafe.Pointer, size uintptr) (uintptr, error) { | ||
| for { |
There was a problem hiding this comment.
Because it is any signal, can't the go runtime trigger this also? It sends SIGUSR to preempt and schedule goroutines.
I think I'd still bound the loop, if it doesn't work in a few retries, we can fail and let the higher level container creation be started again. We don't want an infinite loop on any circumstances.
| ProgIds: uint64(uintptr(unsafe.Pointer(&progIds[0]))), | ||
| ProgCnt: uint32(len(progIds)), | ||
| } | ||
|
|
||
| // Fetch the list of program ids. | ||
| _, _, errno := unix.Syscall(unix.SYS_BPF, | ||
| uintptr(unix.BPF_PROG_QUERY), | ||
| uintptr(unsafe.Pointer(&query)), | ||
| unsafe.Sizeof(query)) | ||
| _, err := bpf(unix.BPF_PROG_QUERY, unsafe.Pointer(&query), unsafe.Sizeof(query)) | ||
| runtime.KeepAlive(progIds) |
There was a problem hiding this comment.
Okay, my AI friends caught a weird bug that existed before but we are making slightly more possible to hit now.
The thing is: ProgIds is on the stack and is not tracked as a pointer, as it is uint64 and the pointer it has is gotten via unsafe. If the go runtime decides it needs more memory for this goroutine, it will allocate a bigger stack and move things there. Pointers are re-written, but as ProgIds is not a pointer for the runtime, it is not. And if calling bpf() makes something that needs a bigger stack, we hit this issue.
The way out of this is using https://pkg.go.dev/runtime#Pinner.Pin, it seems. But I'm not very familiar with it.
Replace the use of the cilium/ebpf and cilium/ebpf/link with direct
bpf(2) syscalls. Keep cilium/ebpf/asm for instruction assembly.
Notes:
descriptors instead of *ebpf.Program handles;
binary.BigEndian, thus endian_{le,be}.go are introduced as a
workaround.
This reduces the runc binary size by about ~1M.
NOTE that this is probably limited to runc, because for other users (k8s, cri-o) this was already solved by opencontainers/runc#4248.
Being tested in opencontainers/runc#5340.
For initial discussion about this, see opencontainers/runc#5218.