Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

elf_reader: emit UnspecifiedPrograms into CollectionSpec #529

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

ti-mo
Copy link
Collaborator

@ti-mo ti-mo commented Dec 16, 2021

eBPF programs emitted to sections with unknown prefixes were previously withheld from CollectionSpec.Programs. Add a ProgramSpec.SectionName field to allow the caller to determine (Attach)Type manually, and include all programs that were explicitly assigned to an ELF section.

@ti-mo ti-mo requested a review from lmb December 16, 2021 15:04
@ti-mo ti-mo changed the title elf_reader: emit UnspecifiedPrograms into CollectionSpec elf_reader: emit UnspecifiedPrograms into CollectionSpec Dec 16, 2021
@@ -286,7 +286,11 @@ func NewCollectionWithOptions(spec *CollectionSpec, opts CollectionOptions) (*Co
}
}

for progName := range spec.Programs {
for progName, prog := range spec.Programs {
if prog.Type == UnspecifiedProgram {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this would be the expected behaviour, and should keep things backwards-compatible. Instead of filtering UnspecifiedPrograms out of the CollectionSpec, filter them out on their way into the kernel.

Any other ideas to accomplish this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. What happens if a prog array points at an UnspecifiedProgram?

Seems OK on the surface.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth documenting this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lazy-loader doesn't take this code path afaik, but the kernel should complain about an invalid program. Good point, will investigate.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think returning an error when a programarray references unspecifiedprogram is OK.

Copy link
Collaborator Author

@ti-mo ti-mo Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an explicit UnspecifiedProgram check to (collectionLoader).loadProgram() since there's not much point in trying to let the kernel load these progs. An explicit check also prevents possible dependent maps etc. from getting lazy-loaded, so some work is saved overall.

I've tested putting btf_map_init.c's tail_main() into .text, and it turns out this check isn't tripped currently, since loadProgram() operates on cs.Programs, which .text is not allowed to emit programs into. Maybe in a future patch it will, or into a cs.Functions field, etc. but in the meantime I think it doesn't hurt to have the check there.

@@ -286,7 +286,11 @@ func NewCollectionWithOptions(spec *CollectionSpec, opts CollectionOptions) (*Co
}
}

for progName := range spec.Programs {
for progName, prog := range spec.Programs {
if prog.Type == UnspecifiedProgram {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. What happens if a prog array points at an UnspecifiedProgram?

Seems OK on the surface.

eBPF programs emitted to sections with unknown prefixes were previously
withheld from CollectionSpec.Programs. Add a ProgramSpec.SectionName field
to allow the caller to determine (Attach)Type manually, and include all
programs that were explicitly assigned to an ELF section.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo merged commit fc62b52 into cilium:master Dec 21, 2021
@ti-mo ti-mo deleted the tb/free-section-naming branch December 21, 2021 16:03
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.

None yet

2 participants