Add initial __kconfig support with LINUX_KERNEL_VERSION. by eiffel-fl · Pull Request #869 · cilium/ebpf

eiffel-fl · 2022-11-29T15:29:23Z

Hi.

This PR is a first step toward __kconfig support which is discussed in #698.
When a .kconfig data section is detected, a .kconfig map is created, its content will be initialized depending on the CONFIG_* values.
During relocation, variable inside the .kconfig data section will have their offset changed to reflect their value inside the .kconfig map.

For the moment, it only enables using LINUX_KERNEL_VERSION. The .kconfig map will be updated to contain the result of internal.KernelVersion().

I tested it with some basic eBPF program I wrote:

root@81d71157c164:/mnt/cilium-ebpf-test-kconfig# more bpf/test.bpf.c 
// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>

extern int LINUX_KERNEL_VERSION __kconfig;

SEC("tp_btf/block_rq_insert")
int insert(u64 *ctx)
{
        bpf_printk("LINUX_KERNEL_VERSION: %d\n", LINUX_KERNEL_VERSION);

        return 0;
}

char LICENSE[] SEC("license") = "GPL";
root@81d71157c164:/mnt/ebpf# go run ./...
# Switch to other terminal
$ sudo cat /sys/kernel/debug/tracing/trace_pipe                                                               
 .../2-579     [006] d.... 11213.059870: bpf_trace_printk: LINUX_KERNEL_VERSION: 331584

Sadly, I was not able to make the added TestLinuxKernelVersion() works because the ELF misses BTF and I need btfSpec to not be nil to check data section.
I think I miss some steps in the ELF creation.

If you see any way to improve this PR, feel free to share.

Best regards and thank you in advance.

ti-mo

Gave this a first round. Running the tests doesn't work for me.

btf/types.go

btf/btf.go

testdata/linux_kernel_version.c

internal/errors.go

elf_reader.go

eiffel-fl

Thank you for the review!
I address almost all of your comments and the code is now better than the first version.
I have nonetheless some questions or points I did not understand:

testdata/linux_kernel_version.c

elf_reader.go

eiffel-fl

Hi.

I addressed all your comments, expect the one regarding the place of the test (see my comment about it).
Everything is working and thanks to your comments the code is now clearer, as a consequence I mark the PR as ready.

Best regards and thank you.

testdata/linux_kernel_version.c

elf_reader_test.go

elf_reader.go

This permits simplifying function called on elfCode by removing this map from the function argument. Also, it avoids to Lookup on BTF as we can directly use the map. Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

This commit is a first step toward __kconfig support. When a .kconfig data section is detected, a .kconfig map is created, its content will be initialized depending on the CONFIG_* values. During relocation, variable inside the .kconfig data section will have their offset changed to reflect their value inside the .kconfig map. For the moment, it only enables using LINUX_KERNEL_VERSION. The .kconfig map will be updated to contain the result of internal.KernelVersion(). Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl · 2022-12-23T16:43:52Z

I addressed all of your comments expect the one regarding the test, please take a look at my answer because I am not 100% sure this is possible to add this test to loader.c.

Thank you for the reviews and enjoy this Christmas weekend 🎅🎄🤶🎁🥳🎉!

brycekahle · 2023-02-13T17:31:51Z

@lmb @ti-mo We are interested in getting this over the finish line too, so if I can help, please let me know.

ti-mo · 2023-02-15T14:10:13Z

I've been hacking on this for a few days as well, this might take a bit longer to get right. The tests are passing, but there seems to be a conflict between the freestyle relocations we support (see loader.c:asm_relocation()) and ksyms, since the relocations they emit are identical. (see https://cilium.slack.com/archives/C027KBX679U/p1676385904331069, but echoing here)

extern int LINUX_KERNEL_VERSION __kconfig;

__section("socket") int kconfig() {
        return LINUX_KERNEL_VERSION;
}

->

0000000000000000 <kconfig>:
       0:	18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r1 = 0 ll
		0000000000000000:  R_BPF_64_64	LINUX_KERNEL_VERSION

Symbol table '.symtab' contains 5 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
...
     3: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND LINUX_KERNEL_VERSION

vs.

__section("socket/2") int asm_relocation() {
        int my_const;
        asm("%0 = MY_CONST ll" : "=r"(my_const));
        return my_const;
}

->

0000000000000000 <asm_relocation>:
       0:	18 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r0 = 0 ll
		0000000000000000:  R_BPF_64_64	MY_CONST

    33: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND MY_CONST

This makes it so we can't discern between relocations manually declared using asm and the types of relocations emitted for ksyms by clang. We'd like to continue supporting all use cases, but we'll have to get a little creative in doing so.

The current proposal has a few issues, mostly around applying relocations, as well as overall strictness. All ksyms are currently being treated as if they were declared with __weak, that will need to change.

I'd like to take over this work, will report back if there's progress.

eiffel-fl · 2023-02-15T15:00:51Z

Hi.

I've been hacking on this for a few days as well, this might take a bit longer to get right. The tests are passing, but there seems to be a conflict between the freestyle relocations we support (see loader.c:asm_relocation()) and ksyms, since the relocations they emit are identical. (see https://cilium.slack.com/archives/C027KBX679U/p1676385904331069, but echoing here)

I am not sure to understand.
What is the purpose of asm_relocation()? Is this only proper to cilium/ebpf because when implementing this I got a lot of inspiration from what is done in libbpf.

[...]

This makes it so we can't discern between relocations manually declared using asm and the types of relocations emitted for ksyms by clang. We'd like to continue supporting all use cases, but we'll have to get a little creative in doing so.

The current proposal has a few issues, mostly around applying relocations, as well as overall strictness. All ksyms are currently being treated as if they were declared with __weak, that will need to change.

I would need to take another look at libbpf but I think __kconfig are also __weak.
Maybe this is just a matter of changing this:
458466c#diff-85c5d20202d0d0139275cd09e697528f8da73c766a1ce0baae9e26205d5852ceR1151-R1153
?

I'd like to take over this work, will report back if there's progress.

I am not sure I understand what you mean.
I did not abandon this work, I was just waiting for some feedback.

Best regards.

ti-mo · 2023-02-16T11:25:31Z

I did not abandon this work, I was just waiting for some feedback.

Sorry Francis, that's some bad communication on my part. You've addressed all my feedback nicely and things are in a good shape! I hadn't taken the time to look at this a bit closer up, so I discovered a few unknown unknowns this week. I was hoping I could collaborate by adding a few patches underneath or on top before merging this, that's all. Sorry for the misunderstanding, I wasn't implying you abandoned the work at all. 😊

What is the purpose of asm_relocation()? Is this only proper to cilium/ebpf because when implementing this I got a lot of inspiration from what is done in libbpf.

Correct, this is an old hack that was relied on by some users to implement static data before the kernel had support for pseudo/frozen maps like .(ro)data. There are still kernels out there that don't support them, so we're not going to drop support for this right now.

We have a similar issue for adding kfunc support, since we've always permitted forward declarations in C (see fwd_decl.c). This allows the caller to append to Instructions with .WithSymbol("fwd") after loading the ELF, but before loading the prog into the kernel, to inject behaviour at runtime without recompiling.

Had a discussion with Lorenz about this, we worked out some of the compatibility concerns I had around asm_relocation:

If .kconfig is present, all relos against NOTYPE/GLOBAL/UND symbols must be executed against .kconfig. Any such symbols that don't appear in .kconfig are an error.
We won't support __weak in this PR, that can be done later (since only LINUX_KERNEL_VERSION atm)

We're still debating whether or not we should expose .kconfig's MapSpec.Contents in the CollectionSpec, or if we'll populate it during map load.

eiffel-fl · 2023-02-16T13:35:26Z

I did not abandon this work, I was just waiting for some feedback.

Sorry Francis, that's some bad communication on my part. You've addressed all my feedback nicely and things are in a good shape! I hadn't taken the time to look at this a bit closer up, so I discovered a few unknown unknowns this week. I was hoping I could collaborate by adding a few patches underneath or on top before merging this, that's all. Sorry for the misunderstanding, I wasn't implying you abandoned the work at all. blush

No problem at all!
I was not sure about what you meant hence my asking.
How do you plan to add the patches? Opening a dedicated PR on top of this one? In any case, I think I can take a look at it!

What is the purpose of asm_relocation()? Is this only proper to cilium/ebpf because when implementing this I got a lot of inspiration from what is done in libbpf.

Correct, this is an old hack that was relied on by some users to implement static data before the kernel had support for pseudo/frozen maps like .(ro)data. There are still kernels out there that don't support them, so we're not going to drop support for this right now.

Definitely, even if this is some legacy, the two should remain.

We have a similar issue for adding kfunc support, since we've always permitted forward declarations in C (see fwd_decl.c). This allows the caller to append to Instructions with .WithSymbol("fwd") after loading the ELF, but before loading the prog into the kernel, to inject behaviour at runtime without recompiling.

Had a discussion with Lorenz about this, we worked out some of the compatibility concerns I had around asm_relocation:
* If .kconfig is present, all relos against NOTYPE/GLOBAL/UND symbols must be executed against .kconfig. Any such symbols that don't appear in .kconfig are an error.

So, you mean to put in .kconfig even symbols which are not marked as __kconfig?
I understand it is hard to find a silver bullet but it would be better to have thing clearly separated.

I will take a look at asm_relocation() as I am not familiar at all with this part of the code.

ti-mo · 2023-02-17T10:49:56Z

How do you plan to add the patches?

By default, maintainers of a repo are allowed to push to PR branches in forks, unless explicitly disabled. The sidebar of this PR says Maintainers are allowed to edit this pull request. for me, but I don't seem to manage to push to it. Could you check if this isn't disabled on your end? No need to create other PRs, let's keep the discussion centered here.

So, you mean to put in .kconfig even symbols which are not marked as __kconfig?

No, I mean we wouldn't support both __kconfig and inline asm relocations (like MY_CONST in asm_relocation()) in the same ELF. If a .kconfig map is present, all NOTYPE/GLOBAL/UND symbols must exist in it. If no .kconfig map is present and we need to relocate a MY_CONST, we just call ins.WithReference() and call it a day. The linker will complain later if the caller didn't fix up the instruction and didn't remove the reference.

eiffel-fl · 2023-02-17T13:16:19Z

How do you plan to add the patches?

By default, maintainers of a repo are allowed to push to PR branches in forks, unless explicitly disabled. The sidebar of this PR says Maintainers are allowed to edit this pull request. for me, but I don't seem to manage to push to it. Could you check if this isn't disabled on your end? No need to create other PRs, let's keep the discussion centered here.

It does not seem my branch to be protected, can you please paste the error message you got?

So, you mean to put in .kconfig even symbols which are not marked as __kconfig?

No, I mean we wouldn't support both __kconfig and inline asm relocations (like MY_CONST in asm_relocation()) in the same ELF. If a .kconfig map is present, all NOTYPE/GLOBAL/UND symbols must exist in it. If no .kconfig map is present and we need to relocate a MY_CONST, we just call ins.WithReference() and call it a day. The linker will complain later if the caller didn't fix up the instruction and didn't remove the reference.

OK, thank you for the clarification!

ti-mo · 2023-02-21T19:47:57Z

@eiffel-fl Sorry, on PTO this week. Just gave this another try (this is after gh pr checkout eiffel-fl:francis/ebpf-kconfig and reworking existing commits and prepending one commit to the existing two:

λ  ebpf francis/ebpf-kconfig ✓ git push -f
ERROR: Permission to eiffel-fl/ebpf.git denied to ti-mo.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Do you have any kind of branch protection set up on your fork? There's a branch protection setting that controls force-pushes, but not sure if it's on or off by default.

eiffel-fl · 2023-02-22T15:13:43Z

@eiffel-fl Sorry, on PTO this week. Just gave this another try (this is after gh pr checkout eiffel-fl:francis/ebpf-kconfig and reworking existing commits and prepending one commit to the existing two:

No problem!

Do you have any kind of branch protection set up on your fork? There's a branch protection setting that controls force-pushes, but not sure if it's on or off by default.

I do not have any protection set, but my fork is actually a fork of a fork (joamaki/ebpf).

ti-mo · 2023-02-27T11:30:24Z

@eiffel-fl Could you try re-forking from cilium/ebpf instead, and recreating this PR using my branch at https://github.com/ti-mo/ebpf/tree/tb/kconfig? I've added some code that fixes the alignment of the .kconfig Datasec and moved populating .kconfig to happen at map load time instead of ELF load time. Otherwise, missing kconfig values would prevent the ELF from loading, leaving the user with no way to fix things up.

Please close this PR when you've recreated it, thanks!

eiffel-fl · 2023-02-27T11:51:26Z

Closing as superseded by #947.

ti-mo requested changes Dec 8, 2022

View reviewed changes

ti-mo marked this pull request as draft December 8, 2022 12:14

eiffel-fl commented Dec 14, 2022

View reviewed changes

testdata/linux_kernel_version.c Show resolved Hide resolved

elf_reader.go Outdated Show resolved Hide resolved

elf_reader.go Outdated Show resolved Hide resolved

elf_reader.go Outdated Show resolved Hide resolved

ti-mo reviewed Dec 15, 2022

View reviewed changes

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.go Outdated Show resolved Hide resolved

eiffel-fl marked this pull request as ready for review December 20, 2022 18:01

eiffel-fl commented Dec 20, 2022

View reviewed changes

ti-mo requested changes Dec 23, 2022

View reviewed changes

eiffel-fl added 2 commits December 23, 2022 17:35

Add map of MapSpec to elfCode.

cc92432

This permits simplifying function called on elfCode by removing this map from the function argument. Also, it avoids to Lookup on BTF as we can directly use the map. Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl mentioned this pull request Feb 27, 2023

Support __kconfig macro for LINUX_KERNEL_VERSION #947

Closed

eiffel-fl closed this Feb 27, 2023

eiffel-fl mentioned this pull request Feb 28, 2023

Support __kconfig macro for LINUX_KERNEL_VERSION #951

Merged

Conversation

eiffel-fl commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eiffel-fl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eiffel-fl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eiffel-fl commented Dec 23, 2022

Uh oh!

brycekahle commented Feb 13, 2023

Uh oh!

ti-mo commented Feb 15, 2023

Uh oh!

eiffel-fl commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo commented Feb 16, 2023

Uh oh!

eiffel-fl commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo commented Feb 17, 2023

Uh oh!

eiffel-fl commented Feb 17, 2023

Uh oh!

ti-mo commented Feb 21, 2023

Uh oh!

eiffel-fl commented Feb 22, 2023

Uh oh!

ti-mo commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eiffel-fl commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eiffel-fl commented Nov 29, 2022 •

edited

Loading

eiffel-fl commented Feb 15, 2023 •

edited

Loading

eiffel-fl commented Feb 16, 2023 •

edited

Loading

ti-mo commented Feb 27, 2023 •

edited

Loading