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, linker: add kfunc support #966

Merged
merged 3 commits into from
Mar 29, 2023
Merged

elf_reader, linker: add kfunc support #966

merged 3 commits into from
Mar 29, 2023

Conversation

rgo3
Copy link
Contributor

@rgo3 rgo3 commented Mar 8, 2023

This commit adds support for handling the following extern 
kfunc declaration in bpf programs:

extern void bpf_kfunc_foo(void *mem) __ksym;

If during relocation a call to a kfunc is found we set the corresponding 
btf.Func as Metadata on the instruction and change Instruction.Src to 
asm.PseudoKfuncCall.

fixupKfunc() has been added to the linker, to find the type id of the 
btf.Func in the running kernel and set Instruction.Constant to the found 
btf id.

@lmb lmb self-requested a review March 8, 2023 16:54
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Very nice, things slot into place really well and it has a small scope. Thanks!

elf_reader.go Outdated Show resolved Hide resolved
elf_reader.go Outdated Show resolved Hide resolved
elf_reader.go Outdated Show resolved Hide resolved
elf_reader_test.go Outdated Show resolved Hide resolved
linker.go Outdated Show resolved Hide resolved
linker.go Outdated
}

// we should compare args and return type here
// coreAreTypesCompatible(kfm.Type, fn.Type) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we'll have to export that function. How about btf.CheckCompatibility(Type, Type) error?

The big comment at the top of the function should move into the function body, it's too much implementation detail. We'll also have to replace errImpossibleRelocation with a more meaningful error, maybe even an exported one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is #896 different from exporting coreAreTypesCompatible(Type, Type)? cc @ti-mo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hitting my btf knowledge limit again with the CI failure. Looks like either Return or Parameters of the FuncProto will be a Typedef and we don't support that in CheckTypeCompatibility. Wondering if adding *Typdef to the case *Pointer, *Array statement would solve this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, the reason that we don't handle Typedef in coreAreTypesCompatible is that the function assumes that all qualifiers and Typedefs have been stripped out using UnderlyingType.

Wondering if adding *Typdef to the case *Pointer, *Array statement would solve this?

Unfortunately not, since we want to treat the following as equal:

local: Typedef{Int}
target: Int{}

If we just add to the switch case, we'd still hit

ebpf/btf/core.go

Lines 878 to 880 in 5ea5368

if reflect.TypeOf(localType) != reflect.TypeOf(targetType) {
return fmt.Errorf("type mismatch: %w", errImpossibleRelocation)
}
which would fail since Typedef != Int.

The best fix is to create a wrapper function I think. Leave coreAreTypesCompatible unexported, and instead export the wrapper. The wrapper calls Copy(local, UnderlyingType), etc. and then invokes coreAreTypesCompatible. CO-RE relocation would still call coreAreTypesCompatible but external callers get the more sensible behaviour.

linker.go Outdated Show resolved Hide resolved
linker.go Outdated Show resolved Hide resolved
linker.go Outdated Show resolved Hide resolved
@rgo3
Copy link
Contributor Author

rgo3 commented Mar 9, 2023

All the LibBPFCompat Tests that are failing because of STB_WEAK need to be skipped right?

@rgo3 rgo3 force-pushed the kfuncs branch 4 times, most recently from f25500d to 0c17375 Compare March 10, 2023 17:13
@lmb
Copy link
Collaborator

lmb commented Mar 13, 2023

All the LibBPFCompat Tests that are failing because of STB_WEAK need to be skipped right?

Yes. Does it make sense to return ErrNotSupported for STB_WEAK as well?

btf/btf.go Show resolved Hide resolved
elf_reader.go Outdated Show resolved Hide resolved
elf_reader.go Show resolved Hide resolved
@rgo3 rgo3 force-pushed the kfuncs branch 2 times, most recently from 588fffa to b25500e Compare March 20, 2023 22:34
btf/core.go Outdated Show resolved Hide resolved
btf/core.go Outdated Show resolved Hide resolved
btf/core.go Outdated Show resolved Hide resolved
linker.go Outdated
}

// we should compare args and return type here
// coreAreTypesCompatible(kfm.Type, fn.Type) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, the reason that we don't handle Typedef in coreAreTypesCompatible is that the function assumes that all qualifiers and Typedefs have been stripped out using UnderlyingType.

Wondering if adding *Typdef to the case *Pointer, *Array statement would solve this?

Unfortunately not, since we want to treat the following as equal:

local: Typedef{Int}
target: Int{}

If we just add to the switch case, we'd still hit

ebpf/btf/core.go

Lines 878 to 880 in 5ea5368

if reflect.TypeOf(localType) != reflect.TypeOf(targetType) {
return fmt.Errorf("type mismatch: %w", errImpossibleRelocation)
}
which would fail since Typedef != Int.

The best fix is to create a wrapper function I think. Leave coreAreTypesCompatible unexported, and instead export the wrapper. The wrapper calls Copy(local, UnderlyingType), etc. and then invokes coreAreTypesCompatible. CO-RE relocation would still call coreAreTypesCompatible but external callers get the more sensible behaviour.

linker.go Show resolved Hide resolved
With this commit coreAreTypesCompatible() is wrapped and exported as
CheckTypeCompatibility(). This is needed to support type compatibility
checks for kfuncs.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3 rgo3 marked this pull request as ready for review March 27, 2023 23:43
@lmb lmb self-requested a review March 28, 2023 15:43
Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
This commit adds support for handling the following extern
kfunc declaration in bpf programs:

extern void bpf_kfunc_foo(void *mem) __ksym;

If during relocation a call to a kfunc is found we set the corresponding
btf.Func as Metadata on the instruction and change Instruction.Src to
asm.PseudoKfuncCall.

fixupKfuncs() has been added to the linker, to find the type id of the
btf.Func in the running kernel and set Instruction.Constant to the found
btf id.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@lmb lmb merged commit ad5afdc into cilium:master Mar 29, 2023
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