-
Notifications
You must be signed in to change notification settings - Fork 320
Add support for kernel module split BTF #1248
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
5402835
to
1d71a20
Compare
Thanks for the PR! I notice there aren't any new tests. Have you thought about how we might test this? |
From a unit testing POV, we could have some simple checks making sure the logic to merge BTFs is correct. We could generate a base BTF and X split BTFs, merge them, and make sure that the merged btf is fully consistent. From an integration test perspective, we could try loading an eBPF that depends on a kernel module. That's what my original use-case was. We could probably load an eBPF program that does something like:
to make sure that the eBPF program actually has access to kernel module structs. Would that be an acceptable testing strategy? I can work on implementing them. |
fb9813f
to
86f6d4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 14 unresolved discussions
aya-obj/src/btf/btf.rs
line 274 at r2 (raw file):
pub fn merge_split_btfs(base_btf: &Btf, split_btfs: &[Btf]) -> Btf { // Create a Btf from all the btfs. let mut out_btf = base_btf.clone();
nit: pass base_btf
by value, let the caller decide how to manage this cloning
aya-obj/src/btf/btf.rs
line 276 at r2 (raw file):
let mut out_btf = base_btf.clone(); let mut rebase_info = BtfRebaseInfo {
instead of making this mut
, consider creating rebase_info in the loop.
aya-obj/src/btf/btf.rs
line 282 at r2 (raw file):
types_new_offset: out_btf.types.types.len() as u32, }; for btf in split_btfs {
can you destructure this? it's quite hard to see that all the proper fields are being used
Code quote:
btf
aya-obj/src/btf/btf.rs
line 327 at r2 (raw file):
} /// Loads BTF metadata from `/sys/kernel/btf`.
please add back the trailing /
aya-obj/src/btf/btf.rs
line 337 at r2 (raw file):
.flatten() .filter_map(|v| v.ok()) .filter(|v| v.file_name() != "vmlinux");
this is discarding errors. why is that OK?
Code quote:
let dir_iter = std::fs::read_dir("/sys/kernel/btf")
.ok()
.into_iter()
.flatten()
.filter_map(|v| v.ok())
.filter(|v| v.file_name() != "vmlinux");
aya-obj/src/btf/btf.rs
line 341 at r2 (raw file):
match entry.file_type() { Ok(v) if !v.is_file() => continue, Err(_err) => continue,
why?
aya-obj/src/btf/btf.rs
line 346 at r2 (raw file):
match Btf::parse_file(entry.path(), Endianness::default()) { Ok(v) => split_btfs.push(v), // Ignore errors - the goal is to enhance the base BTF with as
This isn't explaining why ignoring errors is ok.
aya-obj/src/btf/btf.rs
line 352 at r2 (raw file):
} if split_btfs.len() > 0 {
do you need this conditional? why?
aya-obj/src/btf/types.rs
line 16 at r2 (raw file):
/// Offsets below this value are considered to be part of the "base BTF", /// and as such should not be relocated. pub(crate) str_rebase_from: u32,
can we use usize for all of these? it's an internal type, i think usize would be more convenient
aya-obj/src/btf/types.rs
line 21 at r2 (raw file):
/// Indices below this value are considered to be part of the "base BTF", /// and as such should not be relocated. pub(crate) types_rebase_from: u32,
these can probably be pub
(rather than pub(crate)
)
aya-obj/src/btf/types.rs
line 23 at r2 (raw file):
pub(crate) types_rebase_from: u32, /// The new starting offset for strings. pub(crate) str_new_offset: u32,
consider extracting an inner type and storing 2 of them: one for strings, one for types. This would reduce the duplication in the doc comments.
aya-obj/src/btf/types.rs
line 30 at r2 (raw file):
impl BtfRebaseInfo { fn rebase_str(&self, str_offset: u32) -> u32 { if str_offset < self.str_rebase_from {
this can be a bit pithier:
match str_offset.checked_sub(self.str_rebase_from) {
None => str_offset,
Some(str_offset) => self.str_new_offset + str_offset
}
this should be a method on the type I suggested extracting above. Then you can also write let Self { rebase_from, new_offset } = self
at the top instead of writing self.xxx
.
aya-obj/src/btf/types.rs
line 91 at r2 (raw file):
} pub(crate) fn rebase(&self, rebase_info: &BtfRebaseInfo) -> Self {
in all these methods: could you destructure self
?
aya-obj/src/btf/types.rs
line 92 at r2 (raw file):
pub(crate) fn rebase(&self, rebase_info: &BtfRebaseInfo) -> Self { Fwd {
can this be Self {
instead of Fwd? here and everywhere.
640e446
to
aeea582
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 14 unresolved discussions (waiting on @tamird)
aya-obj/src/btf/btf.rs
line 337 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this is discarding errors. why is that OK?
So, if we successfully read /sys/kernel/btf/vmlinux
and fail on some other module BTF, it'd kinda suck to be completely locked out of using the working BTFs just because one module doesn't work. Especially since the vast majority of eBPFs out there won't need those split BTF to load properly. So I made this function ignore the broken modules/skip the BTF it cannot read.
I can definitely change it to be stricter.
aya-obj/src/btf/btf.rs
line 341 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why?
Same as above.
aya-obj/src/btf/btf.rs
line 346 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
This isn't explaining why ignoring errors is ok.
Same as above, I can put the reasoning in the comment.
aya-obj/src/btf/btf.rs
line 352 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do you need this conditional? why?
This is to avoid the cloning done in merge_split_btfs. It's a simple (if perhaps premature) optimization.
aya-obj/src/btf/types.rs
line 16 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we use usize for all of these? it's an internal type, i think usize would be more convenient
u32 is the datatype used all over the place in BtfType
, so using a u32 makes a whole lot more sense here. If I use usize, then I'll end up needing to do casting all over the place, which would definitely be less convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 13 unresolved discussions (waiting on @roblabla)
aya-obj/src/btf/btf.rs
line 337 at r2 (raw file):
Previously, roblabla (Robin Lambertz) wrote…
So, if we successfully read
/sys/kernel/btf/vmlinux
and fail on some other module BTF, it'd kinda suck to be completely locked out of using the working BTFs just because one module doesn't work. Especially since the vast majority of eBPFs out there won't need those split BTF to load properly. So I made this function ignore the broken modules/skip the BTF it cannot read.I can definitely change it to be stricter.
I think we should be stricter. There may be certain kinds of errors we'd want to allow and silence, but the default position is that we don't discard errors.
aya-obj/src/btf/btf.rs
line 352 at r2 (raw file):
Previou 10000 sly, roblabla (Robin Lambertz) wrote…
This is to avoid the cloning done in merge_split_btfs. It's a simple (if perhaps premature) optimization.
See my other comment; if you make that function take its argument by value you'd avoid needing this special case.
aya-obj/src/btf/types.rs
line 16 at r2 (raw file):
Previously, roblabla (Robin Lambertz) wrote…
u32 is the datatype used all over the place in
BtfType
, so using a u32 makes a whole lot more sense here. If I use usize, then I'll end up needing to do casting all over the place, which would definitely be less convenient.
OK.
89513cc
to
ef3d529
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 12 unresolved discussions (waiting on @tamird)
aya-obj/src/btf/btf.rs
line 276 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
instead of making this
mut
, consider creating rebase_info in the loop.
Done.
aya-obj/src/btf/btf.rs
line 282 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can you destructure this? it's quite hard to see that all the proper fields are being used
Done.
aya-obj/src/btf/btf.rs
line 327 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
please add back the trailing
/
Done.
aya-obj/src/btf/btf.rs
line 337 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think we should be stricter. There may be certain kinds of errors we'd want to allow and silence, but the default position is that we don't discard errors.
Done.
aya-obj/src/btf/btf.rs
line 341 at r2 (raw file):
Previously, roblabla (Robin Lambertz) wrote…
Same as above.
Done.
aya-obj/src/btf/btf.rs
line 346 at r2 (raw file):
Previously, roblabla (Robin Lambertz) wrote…
Same as above, I can put the reasoning in the comment.
Done, doesn't ignore errors anymore.
aya-obj/src/btf/btf.rs
line 352 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
See my other comment; if you make that function take its argument by value you'd avoid needing this special case.
Done.
aya-obj/src/btf/types.rs
line 21 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
these can probably be
pub
(rather thanpub(crate)
)
Done.
aya-obj/src/btf/types.rs
line 23 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
consider extracting an inner type and storing 2 of them: one for strings, one for types. This would reduce the duplication in the doc comments.
Done.
aya-obj/src/btf/types.rs
line 30 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this can be a bit pithier:
match str_offset.checked_sub(self.str_rebase_from) {
None => str_offset,
Some(str_offset) => self.str_new_offset + str_offset
}this should be a method on the type I suggested extracting above. Then you can also write
let Self { rebase_from, new_offset } = self
at the top instead of writingself.xxx
.
Done.
aya-obj/src/btf/types.rs
line 91 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
in all these methods: could you destructure
self
?
Done.
aya-obj/src/btf/types.rs
line 92 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can this be
Self {
instead of Fwd? here and everywhere.
Done.
The kernel splits kernel modules into dedicated BTF files, using "split BTF mode". In split BTF mode, each split BTF may reference the "base BTF" (AKA the vmlinux BTF), but may not reference each-other. The way it works is fairly simple: you can combine a base BTF and split BTF by concatenating their strings and types together, creating a working merged BTF. Where things get dicey is when merging a base BTF with multiple split BTFs. Because every split BTF will start from the same offset, their string offsets and type IDs will need to be "rebased" to a new location when merging it into the output BTF. This commit adds support for merging one base BTF and multiple split BTF into a single merged BTF, allowing eBPFs to manipulate structs that come from kernel modules. It also reworks Btf::from_sys_fs to make use of this capacity.
I've added myself to review this too - not sure I'll have time this week but 🤞 At cursory glance:
|
I feel pretty strongly that since
Ergo this would always take the Also, your mileage may vary here, but from what I can see, vmlinux is usually the biggest BTF by multiple orders of magnitude, with the module BTF being much, much smaller. On my current kernel, the
That would probably be the best of both worlds here, yes.
Yes, I'm working on those.
So I tried something along those lines first, but quickly gave up as I realized it required changing quite a few abstraction in the relocation code. In particular, it means that
I'm not sure I understand what this means. For starters, I don't understand why you'd want to check module BTF types before searching the base BTF? As far as I understand, you shouldn't have any conflicts here (if you did, that would cause ambiguity - what if you had conflicts betwen different module BTFs?). Generally speaking, I think testing this approach is rather straightforward: you can just create a base BTF object and multiple "split" BTFs, merge them, and make sure everything still resolves properly. |
I agree that the silent failure there in unfortunate - in most cases I've seen related to missing split-BTF we poison the instruction and then the program fails to load, but obviously here that's not possible. The fact remains though with the API you propose (changing
Feel free to pull in the first commit from https://github.com/dave-tucker/aya/tree/split-btf which adds kallsysms parsing.
type_id's not being unique across modules is a "feature" of module BTF unfortunately.
AIUI you need this for forward declarations. The base BTF could declare Basically, I'd rather we stick as close to what libbpf does as possible. |
This still feels off to me. Rather than this, wouldn't it make more sense to keep looking if we hit a FWD to see if we can find a STRUCT? This way, the behavior is consistent whether we are in a split BTF scenario or not. |
🤷 ultimately it comes back to 👇
Or follow prior-art... the same pattern is used in cilium/ebpf too: https://github.com/cilium/ebpf/pull/1300/files#diff-82332939f914df35856d29498006ce845e5341b42f3b550179a4fa1c8a7dddcf It seems having |
The kernel splits kernel modules into dedicated BTF files, using "split BTF mode". In split BTF mode, each split BTF may reference the "base BTF" (AKA the vmlinux BTF), but may not reference each-other. The way it works is fairly simple: you can combine a base BTF and split BTF by concatenating their strings and types together, creating a working merged BTF.
Where things get dicey is when merging a base BTF with multiple split BTFs. Because every split BTF will start from the same offset, their string offsets and type IDs will need to be "rebased" to a new location when merging it into the output BTF.
This commit adds support for merging one base BTF and multiple split BTF into a single merged BTF, allowing eBPFs to manipulate structs that come from kernel modules. It also reworks
Btf::from_sys_fs
to make use of this capacity.Fixes #300
This change is