[v1.14] Always migrate cilium_calls_* during ELF load #28830
Merged
Conversation
This file contains 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
[ upstream commit 9420217 ] 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>
maintainer-s-little-helper
bot
added
backport/1.14
|
/test-backport-1.14 |
[ upstream commit 94ec45a ] 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>
ti-mo
force-pushed
the
tb/v1.14-migrate-cilium-calls
branch
from
fd9f5a5
to
49860e4
Compare
margamanterola
added
the
dont-merge/waiting-for-user-feedback
|
Marking as "Don't merge", we'll let the changes bake in main for a few days to check that they are safe. |
|
/test-backport-1.14 |
|
Can we get this one merged? This would allow to see if it makes things better with the upgrade/downgrade from main test. |
ti-mo
removed
the
dont-merge/waiting-for-user-feedback
ti-mo
requested review from
a team and
dylandreimerink
and removed request for
a team
dylandreimerink
approved these changes
Nov 8, 2023
brb
reviewed
Nov 8, 2023
| @@ -90,35 +92,52 @@ func repinMap(bpffsPath string, name string, spec *ebpf.MapSpec) error { | |||
| if err != nil { | |||
| return fmt.Errorf("map not found at path %s: %v", name, err) | |||
| } | |||
| defer pinned.Close() | |||
There was a problem hiding this comment.
I assume that closing a pinned map FD doesn't cause any side-effects.
There was a problem hiding this comment.
Yep, this Close() should've always been there. Without it, the GC closes the fd.
brb
reviewed
Nov 8, 2023
|
|
||
| if pinned.Type() == spec.Type && | ||
| pinned.KeySize() == spec.KeySize && | ||
| pinned.ValueSize() == spec.ValueSize && | ||
| pinned.Flags() == spec.Flags && | ||
| pinned.MaxEntries() == spec.MaxEntries { | ||
| return nil | ||
| // cilium_calls_xdp is shared between XDP interfaces and should only be |
There was a problem hiding this comment.
Could you create a GH issue to document the fact that the cilium_calls_xdp is shared among all XDP progs. I think it's a ticking bomb once we move more functionality to the XDP layer.
brb
approved these changes
Nov 8, 2023
squeed
approved these changes
Nov 8, 2023
maintainer-s-little-helper
bot
added
ready-to-merge
brb
added a commit
that referenced
this pull request
Nov 8, 2023
Since [1] got merged, we can re-enable the check. [1]: #28830. Signed-off-by: Martynas Pumputis <m@lambda.lt>
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.
Manual backport of #28740.