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.

Sign up for GitHub

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

Jump to bottom

elf_reader: emit UnspecifiedPrograms into CollectionSpec #529

Merged
merged 1 commit into from
Dec 21, 2021
Merged

elf_reader: emit UnspecifiedPrograms into CollectionSpec #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

continue
}

if _, err := loader.loadProgram(progName); err != nil {
return nil, err
}
Expand Down Expand Up @@ -424,6 +428,12 @@ func (cl *collectionLoader) loadProgram(progName string) (*Program, error) {
return nil, fmt.Errorf("unknown program %s", progName)
}

// Bail out early if we know the kernel is going to reject the program.
// This skips loading map dependencies, saving some cleanup work later.
if progSpec.Type == UnspecifiedProgram {
return nil, fmt.Errorf("cannot load program %s: program type is unspecified", progName)
}

progSpec = progSpec.Copy()

// Rewrite any reference to a valid map in the program's instructions,
Expand Down
7 changes: 5 additions & 2 deletions elf_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ func (ec *elfCode) loadProgramSections() (map[string]*ProgramSpec, error) {
Flags: progFlags,
AttachType: attachType,
AttachTo: attachTo,
SectionName: sec.Name,
License: ec.license,
KernelVersion: ec.version,
Instructions: insns,
Expand All @@ -323,9 +324,11 @@ func (ec *elfCode) loadProgramSections() (map[string]*ProgramSpec, error) {
return nil, fmt.Errorf("populating references: %w", err)
}

// Don't emit programs of unknown type to preserve backwards compatibility.
// Hide programs (e.g. library functions) that were not explicitly emitted
// to an ELF section. These could be exposed in a separate CollectionSpec
// field later to allow them to be modified.
for n, p := range progs {
if p.Type == UnspecifiedProgram {
if p.SectionName == ".text" {
delete(progs, n)
}
}
Expand Down
40 changes: 28 additions & 12 deletions elf_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,24 +96,40 @@ func TestLoadCollectionSpec(t *testing.T) {
},
Programs: map[string]*ProgramSpec{
"xdp_prog": {
Name: "xdp_prog",
Type: XDP,
License: "MIT",
Name: "xdp_prog",
Type: XDP,
SectionName: "xdp",
License: "MIT",
},
"no_relocation": {
Name: "no_relocation",
Type: SocketFilter,
License: "MIT",
Name: "no_relocation",
Type: SocketFilter,
SectionName: "socket",
License: "MIT",
},
"asm_relocation": {
Name: "asm_relocation",
Type: SocketFilter,
License: "MIT",
Name: "asm_relocation",
Type: SocketFilter,
SectionName: "socket/2",
License: "MIT",
},
"data_sections": {
Name: "data_sections",
Type: SocketFilter,
License: "MIT",
Name: "data_sections",
Type: SocketFilter,
SectionName: "socket/3",
License: "MIT",
},
"global_fn3": {
Name: "global_fn3",
Type: UnspecifiedProgram,
SectionName: "other",
License: "MIT",
},
"static_fn": {
Name: "static_fn",
Type: UnspecifiedProgram,
SectionName: "static",
License: "MIT",
},
},
}
Expand Down
6 changes: 6 additions & 0 deletions prog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,17 @@ type ProgramSpec struct {
// Type determines at which hook in the kernel a program will run.
Type ProgramType
AttachType AttachType

// Name of a kernel data structure or function to attach to. Its
// interpretation depends on Type and AttachType.
AttachTo string

// The program to attach to. Must be provided manually.
AttachTarget *Program

// The name of the ELF section this program orininated from.
SectionName string

Instructions asm.Instructions

// Flags is passed to the kernel and specifies additional program
Expand Down