loader: attach XDP programs using bpf_link #28308
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test |
|
/test |
c1e0667 to
e524710
Compare
|
/test |
41cd870 to
1432100
Compare
|
/test |
|
After the offline code walk with @ti-mo and @lmb last week I'll move this into ready for review. I've left 2 TODO's open intentionally to be discussed during review, most important probably being the decision around the bpffs linkpath we choose to store pins. I think that there is a possibility to backport this entire PR (the fix to |
asauber
requested changes
Oct 20, 2023
48c811a to
01f7fc9
Compare
asauber
approved these changes
Nov 6, 2023
This commit fixes a bug where Cilium expected the XDP mode flag to be carried in the netlink.LinkXdp.Flags field, while in reality the XDP mode is actually contained in the LinkXdp.AttachMode field. Also added a regression test. Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
|
@cilium/tophat Sorry for the noise, you got pinged because pkg/socketlb didn't have a codeowner. I assigned one now. |
A follow-up commit will introduce attaching XDP programs using bpf_link. Those attachments cannot be overridden using netlink. If an older version of Cilium wants to replace an XDP program on a managed interface, it'll need to remove the bpf_link first. This commit teaches the agent to remove the (currently) only XDP entrypoint, cil_xdp_entry, before reattaching it using netlink. Note that this transition is never seamless, since some time passes between deleting the link and attaching the new program. Also note that this downgrade path should seldom be used. If Cilium is upgraded from a version with only netlink support, the new Cilium version will continue to use netlink. Only if a fresh node is deployed with a new Cilium version supporting bpf_link, then downgraded to one supporting only netlink, will the bpf_link need to be removed. For adding XDP bpf_link support, a per-device bpffs directory will be created to pin the device's links, maps, etc. Follow-up commits will put XDP bpf_links here. Subdirectories will be created for each device, for example: /sys/fs/bpf/cilium/devices/enp5s0/ Then, following the convention we're already using for cgroup links, another subdirectory will be created for links, followed by the entrypoint name: /sys/fs/bpf/cilium/devices/enp5s0/links/cil_xdp_entry Signed-off-by: Robin Gögge <r.goegge@isovalent.com> Co-authored-by: Timo Beckers <timo@isovalent.com>
…ilters Signed-off-by: Timo Beckers <timo@isovalent.com>
We're using 0755 for all bpffs directories, so standardize on this. Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Package socketlb had existing code for updating and unpinning links based on a pin path and a program handle. Factor these out into package bpf so the XDP loader can start using them. Signed-off-by: Timo Beckers <timo@isovalent.com>
This extends the loader package to utilize bpf_link to attach XDP programs if the kernel supports it. To avoid connectivity issues during upgrades, existing XDP netlink attachments will not be replaced by bpf_link-based ones. For these interfaces, we keep replacing the existing netlink XDP attachment. bpf_link will only be used on newly-created or rebooted nodes. This matches the behaviour of package socketlb. A high-level overview of the changes: - Introduced a simple loader.DetachXDP function to ensure a given link doesn't have a given XDP program attached. This takes care of detaching both netlink and xdp_link-based progs. - attachProgram() was split into attachTCProgram() and attachXDPProgram(). - attachXDPProgram() implements the functionality outlined in the first paragraph. - Tests were added for all code paths (clean host, existing netlink prog, as well as existing bpf_link prog). Signed-off-by: Robin Gögge <r.goegge@isovalent.com> Co-authored-by: Timo Beckers <timo@isovalent.com>
This was a forwards-compatibility measure intended for older agent versions and is now redundant. Signed-off-by: Timo Beckers <timo@isovalent.com>
|
/test |
ti-mo
approved these changes
Nov 9, 2023
squeed
approved these changes
Nov 9, 2023
@rgo3 If I see things correctly, this happened with #29105 and #29104. So the |
|
Yes, we forgot to update the labels unfortunately 😞 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits.
The first two commits (prog removal and xdp bpf_link cleanup) will be submitted as a separate PR for backporting to 1.14 and 1.13 so these versions can handle downgrades from bpf_link-enabled Cilium agents. They were kept in this branch since future commits depended on them, and getting this broken up into smaller commits that make sense was already a challenge.
On a high level, this PR:
/sys/fs/bpf/cilium/devices/enp5s0/links/cil_xdp_entryattachProgram()intoattachTCProgram()andattachXDPProgram()(this was a minimal change)link.XDPAttachFlagsas parameters instead of a simple u32This also puts us in a better position for adding tcx support during the next cycle.
Fixes: #27630