Skip to content

Comments

map: check MapReplacements compatibility after applying BPF_F_MMAPABLE#1655

Merged
ti-mo merged 1 commit intocilium:mainfrom
ti-mo:tb/mmapable-map-reuse
Jan 24, 2025
Merged

map: check MapReplacements compatibility after applying BPF_F_MMAPABLE#1655
ti-mo merged 1 commit intocilium:mainfrom
ti-mo:tb/mmapable-map-reuse

Conversation

@ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Jan 24, 2025

There were 2 problems with the existing implementation:

  • cl.loadMap would mutate the original MapSpec before loading
  • map compatibility was checked before cl.loadMap applied the BPF_F_MMAPABLE flag

To address this, the following changes were made:

  • MapSpec.Copy() is called in loadMap, to follow suit with cl.loadProgram
  • the compatibility check on MapReplacements was deferred to loadMap, after flags are mutated
  • Added a superficial CollectionSpec mutation test after loading, using the same logic as TestLoadCollectionSpec. testutils.IsDeepCopy() isn't usable here since MapSpec.Copy() doesn't deepcopy .Contents.
  • Added a regression test on using .data and .rodata in MapReplacements

Fixes issues seen in cilium/pwru#484. cc @brb @Asphaltt

There were 2 problems with the existing implementation:
- cl.loadMap would mutate the original MapSpec before loading
- map compatibility was checked before cl.loadMap applied the BPF_F_MMAPABLE flag

To address this, the following changes were made:
- MapSpec.Copy() is called in loadMap, to follow suit with cl.loadProgram
- the compatibility check on MapReplacements was deferred to loadMap, after
  flags are mutated
- Added a superficial CollectionSpec mutation test after loading, using the same
  logic as TestLoadCollectionSpec. testutils.IsDeepCopy() isn't usable here since
  MapSpec.Copy() doesn't deepcopy .Contents.
- Added a regression test on using .data and .rodata in MapReplacements

Signed-off-by: Timo Beckers <[email protected]>
@ti-mo ti-mo requested a review from a team as a code owner January 24, 2025 10:16
@ti-mo ti-mo merged commit 278a914 into cilium:main Jan 24, 2025
17 checks passed
@ti-mo ti-mo deleted the tb/mmapable-map-reuse branch January 24, 2025 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant