Always migrate cilium_calls_* during ELF load #28740
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 |
|
I have no opinions on the code, but regarding the release notes, remember that the readers of the release notes are our users, that don't necessarily understand the inner workings of Cilium. Particularly since this is tagged "bug", it's important to let users know what kind of bug is getting fixed and when they would be affected by it. I suggest something like: |
Good idea, thanks 👍 |
143cada to
0e7762c
Compare
|
/test |
0e7762c to
66287a3
Compare
|
/test |
66287a3 to
b0985dc
Compare
|
/test |
b0985dc to
8734d8b
Compare
|
/test |
See code comments, as the change is subtle. This sets things up for subsequent commits. As a minor benefit, make the code slightly less unreadable. Signed-off-by: Timo Beckers <timo@isovalent.com>
|
/test |
8734d8b to
c66d06a
Compare
Note: this is obviously a hack. There's a long-standing issue with Cilium's loader infrastructure where program arrays like cilium_calls_* are reused if the map's properties in the ELF are compatible with the pinned version present on the system. This has been the case since the early iproute2 days, and ebpf-go has the same behaviour by default. This is unsound, but doesn't always cause issues in practice. However, as Cilium's eBPF code base grew, along with its contributor base, increasing code churn and the need to backport fixes, this method started falling apart. Pinned tail call maps are actively used for handling traffic, and should be considered fixed, as they are an integral part of the program's control flow. When another BPF object needs to be attached to a kernel hook, it's incorrect to replace entries in an existing program array used by a prior version of the BPF object, since logic in some tail calls may have been enabled or disabled in the new one, or may have been removed altogether. A prog array's contents cannot be replaced atomically, meaning that while the prog array is being repopulated by a new ELF, a packet may be processed by a chain of programs that are partially old and partially new. This is bad. This patch fixes the specific case of cilium_calls_*, since it's the only prog array across all ELFs that consistently acts as the central tail call map. The main motivation for this approach is backportability as far back as 1.12, since we're seeing increased connectivity interruptions (leading to test flakes) during up/downgrades and restarts in that branch. This is caused by other backports creating code churn, as well as the stark difference in the contents of tail calls between major Cilium versions. This code will be replaced by a 'commit' system that does away with the re- pinning behaviour and keeps new map fd's in memory; only committing them to bpffs when the necessary endpoint prog(s) were successfully replaced. Fixes: cilium#20691 Signed-off-by: Timo Beckers <timo@isovalent.com>
c66d06a to
a137ea6
Compare
|
/test |
rgo3
reviewed
Oct 26, 2023
rgo3
approved these changes
Oct 26, 2023
This was referenced Oct 27, 2023
This was referenced Oct 27, 2023
2 tasks
|
Discussed offline with Timo. We won't backport to v1.12 as it's too big of a backport and thus risky. Instead, we'll skip this specific error in v1.12 CI (and v1.13 downgrade tests). |
skmatti
pushed a commit
to skmatti/cilium
that referenced
this pull request
Jul 24, 2024
The following upstream change unconditionally repin the map breaks the functionality for L2 interfaces of Kubevirt. cilium#28740 The bpf map for L2 interfaces are manually created in pkg/datapath/connector/utils.go and remain static, making repinning unnecessary. This change make sure the tail calls are not disturbed during graft process. It should not affect other endpoints since only L2 endpoints are calling graftDatapath. BUG: 333792124, 334106213 Change-Id: Icf2a2bf9629d79761782bd3c2ce3c9fa89d5f42f Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/990344 Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Reviewed-by: Zang Li <zangli@google.com> Lint: Lint 🤖 <android-build-ayeaye@system.gserviceaccount.com> Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Reviewed-by: Sugang Li <sugangli@google.com> Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
skmatti
pushed a commit
to skmatti/cilium
that referenced
this pull request
Jul 24, 2024
There was an upstream PR (cilium#28740) which changed the way tail call map re-pin works during datapath reload. As part of this they updated the interface to the code that initiates reload, so that instead of calling reloadDatapath() twice for each interface (once per direction), the new code now requires calling reloadDatapath() once and have the single call handle both directions. Our multi-nic datapath reloader was using the old model so was exposed to tail call map issues now that the underlying function implementation has changed. This change updates the google_loader code to match the new upstream model. Bug: b/339324159, b/338122169 Change-Id: I54b0324f1be61f07cc4b2e2d69232f0c9bceb584 Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/1018651 Lint: Lint 🤖 <android-build-ayeaye@system.gserviceaccount.com> Reviewed-by: Sugang Li <sugangli@google.com> Reviewed-by: Yifeng Shen <yfshen@google.com> Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Reviewed-by: Maciej Skrocki <maciejskrocki@google.com>
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.
Note: this is obviously a hack. There's a long-standing issue with Cilium's
loader infrastructure where program arrays like cilium_calls_* are reused if
the map's properties in the ELF are compatible with the pinned version present
on the system. This has been the case since the early iproute2 days, and
ebpf-go has the same behaviour by default. This is unsound, but doesn't always
cause issues in practice.
However, as Cilium's eBPF code base grew, along with its contributor base, and
increasing code churn and the need to backport fixes, this method started
falling apart. Pinned tail call maps are actively used for handling traffic,
and should be considered fixed, as they are an integral part of the program's
control flow. When another BPF object needs to be attached to a kernel hook,
it's incorrect to replace entries in an existing program array used by a prior
version of the BPF object, since logic in some tail calls may have been enabled
or disabled in the new one, or may have been removed altogether.
A prog array's contents cannot be replaced atomically, meaning that while the
prog array is being repopulated by a new ELF, a packet may be processed by a
chain of programs that are partially old and partially new. This is bad.
This patch fixes the specific case of cilium_calls_*, since it's the only
prog array across all ELFs that consistently acts as the central tail call map.
The main motivation for this approach is backportability as far back as 1.12,
since we're seeing increased connectivity interruptions (leading to test flakes)
during up/downgrades and restarts in that branch. This is caused by other
backports creating code churn, as well as the stark difference in the contents
of tail calls between major Cilium versions.
This code will be replaced by a 'commit' system that does away with the re-
pinning behaviour and keeps new map fd's in memory; only committing them to
bpffs when the necessary endpoint prog(s) were successfully replaced.
Fixes: #20691