Experimental BTF support #182
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davibe
reviewed
Feb 8, 2024
alessandrod
reviewed
Feb 8, 2024
alessandrod
reviewed
Feb 8, 2024
Closed
dave-tucker
requested changes
Feb 8, 2024
There was a problem hiding this comment.
A couple of things:
- Could you take a pass through the open issues and make sure to add them to the
Closes:part of the PR description that I've added if they are fixed in this PR - Where are the docs? 😄
A section in the README about "BTF Support" would be excellent, explaining how to pass the linker flag to bpf-linker when working on an Aya project.
vadorovsky
force-pushed
the
feature/fix-di
branch
from
b7892bf
to
950237d
Compare
|
@dave-tucker I added a small section in README. Do you think it's enough? I would rather document more details (like C-shims) in the book. |
vadorovsky
force-pushed
the
feature/fix-di
branch
from
950237d
to
1b2bb86
Compare
dave-tucker
approved these changes
Feb 14, 2024
alessandrod
reviewed
Feb 16, 2024
alessandrod
approved these changes
Feb 17, 2024
The previous way of stripping (instead of sanitizing) generic names was not correct. When we define `MyStruct<T>`, then each implementation like `MyStruct<u32>` or `MyStruct<u64>` is in fact a separate type. So to differentiate them in BTF, we need to keep all of them with distinct names. However, Linux kernel doesn't accept BTF type names with the following characters: `<`, `>`, `::`, `,`, ` `. So we need to sanitize them.
First of all, change the name of `strip_generics` function to `sanitize_type_name`. The goal of it is more general - it makes sure that Rust type names are sanitized to be compatible with C. Then, instead of using `replace`, iterate over all characters once and replace characters unsupported by C with their hex representations.
The memory layout of a struct in Rust is undefined[0]; when struct fields are reordered by the compiler, we can end up with DI which has non-monotonic field offsets. The verifier does not like this[1]. Sort struct fields by offset in DI to please the verifier. [0] https://github.com/rust-lang/reference/blob/master/src/types/struct.md [1] torvalds/linux@6283fa3
Previously we were sanitizing only struct types (`DW_tag_structure_type`). Here it's done also for functions (`DW_tag_subprogram`). This change also fixes the `sanitize_type_name` function to accept the `_` character. It's a valid character in C types and converting it to `_5F_` is not great for readability.
Requires a compiletest-rs release containing Manishearth/compiletest-rs#278.
Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
A FileCheck rule needed to be changed from CHECK-NEXT to CHECK. Building with DI implies some DI metadata being inserted after the label. This makes the CHECK-NEXT directive failing as it is hitting DI metadata instead of the (initially) expected match. Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
return a 128 bytes bounded and unique type name out of sanitize_type_name function Signed-off-by: Quentin JEROME <qjerome@users.noreply.github.com>
Before this change, an anonymous C struct present in a shim (written
in C), linked by bpf-linker to a Rust program, would trigger a
null dereference. An example of a struct triggering it is:
```c
typedef struct {
int counter;
} atomic_t;
```
To avoid such footguns, provide a helper function for getting the
DIType name safely.
Fixes #130
Rust does not provide a possibility of defining anonymous types, but we need it to produce BTF acceptable by the kernel for BTF maps. As a workaround, use our DI sanitizer to make type anonymous if it contains a field of type `AyaBtfMapMarker`. To not affect the layout of the struct, it should be defined as `PhantomData`: ```rust pub struct AyaBtfMapMarker(PhantomData<()>); ``` Previously, we were "fixing" this issue by hard-coding `HashMap` as a type for which we remove the name. Since there are many map types in Aya, and we might introduce more in the future, having tha marker type is a better solution.
Provide Rust wrappers for LLVM metadata and debug info types and aim to wrap LLVM C API functions as methods of these wrappers, therefore try to make the logic of sanitizing the DI as safe as possible. The wrappers are stored in two separate modules: * `ir` - for general LLVM IR types like `Metadata` or `MDNode`. * `di` - for types representing debug information nodes like `DINode`, `DIType` etc.
Instead, perform type conversions with `TryFrom` trait and `as_*` methods. Also, add lifetimes to all IR and debug info types.
This way, the raw pointer fields marked as `pub(super)` are not accessible outside the wrapper modules. This makes the modules containing a logic safer and less error-prone.
It's unnecessary. Implement `as_*` methods or `From` trait instead.
Check for data carrying enum first, then attempt members replacement.
Instead of having methods like `as_scope()` or `as_type()`, just implement methods like `name()` or `replace_name()` directly in any LLVM DI type which inherits such method (in LLVM C++ hierarchy), whenever we need it.
Fixes the following debug assertion: Program received signal SIGABRT, Aborted. __pthread_kill_implementation (threadid=281474842404608, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 Download failed: Invalid argument. Continuing without source file ./nptl/./nptl/pthread_kill.c. 44 ./nptl/pthread_kill.c: No such file or directory. (gdb) bt __pthread_kill_implementation (threadid=281474842404608, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 0x0000fffff7a31114 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78 0x0000fffff79eab2c in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 0x0000fffff79d74bc in __GI_abort () at ./stdlib/abort.c:79 0x0000fffff79e4434 in __assert_fail_base (fmt=0xfffff7b005d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0xaaaaac9b1da8 "Val && \"isa<> used on a null pointer\"", file=file@entry=0xaaaaac9b1d60 "/home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h", line=line@entry=109, function=function@entry=0xaaaaaca291f0 "static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = llvm::MetadataAsValue; From = llvm::Value]") at ./assert/assert.c:92 0x0000fffff79e449c in __GI___assert_fail (assertion=0xaaaaac9b1da8 "Val && \"isa<> used on a null pointer\"", file=0xaaaaac9b1d60 "/home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h", line=109, function=0xaaaaaca291f0 "static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = llvm::MetadataAsValue; From = llvm::Value]") at ./assert/assert.c:101 0x0000aaaaabd6b8f8 in llvm::isa_impl_cl<llvm::MetadataAsValue, llvm::Value const*>::doit (Val=0x0) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:109 0x0000aaaaabd6c788 in llvm::isa_impl_cl<llvm::MetadataAsValue, llvm::Value const*>::doit (Val=<optimized out>) at /home/ubuntu/src/llvm-project/llvm/lib/IR/Core.cpp:1090 llvm::isa_impl_wrap<llvm::MetadataAsValue, llvm::Value const*, llvm::Value const*>::doit (Val=<optimized out>) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:137 llvm::isa_impl_wrap<llvm::MetadataAsValue, llvm::Value const* const, llvm::Value const*>::doit (Val=<optimized out>) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:129 llvm::CastIsPossible<llvm::MetadataAsValue, llvm::Value const*, void>::isPossible (f=<optimized out>) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:257 llvm::CastInfo<llvm::MetadataAsValue, llvm::Value* const, void>::isPossible (f=<synthetic pointer>: <optimized out>) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:509 llvm::isa<llvm::MetadataAsValue, llvm::Value*> (Val=<synthetic pointer>: <optimized out>) at /home/ubuntu/src/llvm-project/llvm/include/llvm/Support/Casting.h:549 LLVMGetNumOperands (Val=<optimized out>) at /home/ubuntu/src/llvm-project/llvm/lib/IR/Core.cpp:1089 0x0000aaaaaadb21a8 in bpf_linker::llvm::types::di::DICompositeType::elements (self=0xffffffff1bd0) at src/llvm/types/di.rs:250 0x0000aaaaaad98eec in bpf_linker::llvm::di::DISanitizer::mdnode (self=0xffffffffc970, mdnode=...) at src/llvm/di.rs:99 0x0000aaaaaad99880 in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833e50, depth=7) at src/llvm/di.rs:243 0x0000aaaaaad9a83c in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833e20, depth=6) at src/llvm/di.rs:285 0x0000aaaaaad9a83c in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833df0, depth=5) at src/llvm/di.rs:285 0x0000aaaaaad9a83c in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833c60, depth=4) at src/llvm/di.rs:285 0x0000aaaaaad9a83c in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833ba0, depth=3) at src/llvm/di.rs:285 0x0000aaaaaad9a83c in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaaf833b70, depth=2) at src/llvm/di.rs:285 0x0000aaaaaad9aad8 in bpf_linker::llvm::di::DISanitizer::discover (self=0xffffffffc970, value=0xaaaaadc13b10, depth=1) at src/llvm/di.rs:261 0x0000aaaaaad9b43c in bpf_linker::llvm::di::DISanitizer::run (self=0xffffffffc970) at src/llvm/di.rs:336 0x0000aaaaaad872bc in bpf_linker::linker::Linker::optimize (self=0xffffffffdf10) at src/linker.rs:452 0x0000aaaaaad8539c in bpf_linker::linker::Linker::link (self=0xffffffffdf10) at src/linker.rs:259 0x0000aaaaaac2e1d0 in bpf_linker::main () at src/bin/bpf-linker.rs:285
Don't use CStr to wrap MDString since MDStrings are not null terminated. They have an explicit length, and for our purposes we can assume that they're always valid utf8 since we're dealing with rust symbol names.
This is a refactor of the visitor to give it more structure, use less unsafe and generally make it a bit more idiomatic rust.
We only want exported symbols (programs marked as #[no_mangle]) to have
linkage=global. This avoid issues like:
Global function write() doesn't return scalar. Only those are supported.
verification time 18 usec
stack depth 0+0
...
The error above happens when aya-log's WriteBuf::write doesn't get
inlined, and ends up having linkage=global. Global functions are
verified independently from their callers, so the verifier has less
context, and as a result globals are harder to verify.
In clang one makes a function global by not making it `static`. That
model doesn't work for rust, where all symbols exported by dependencies
are non-static, and therefore end up being emitted as global.
This commit implements a custom pass which marks functions as
linkage=static unless they've been explicitly exported.
Fixes aya-rs/aya#808
Signed-off-by: qjerome <qjerome@rawsec.lu>
Signed-off-by: qjerome <qjerome@rawsec.lu>
Signed-off-by: qjerome <qjerome@rawsec.lu>
Signed-off-by: qjerome <qjerome@rawsec.lu>
Structured logging from `tracing` meets our needs better, especially in the parts of bpf-linker which sanitize the debug info. Fixes #177
When processing names of composite types, we sanitize the Rust names (e.g. names with generics, like `Option<T>`) to be compatible with C (otherwise, kernel would refuse to load such BTF). However, we should still keep using the original Rust name in logs.
Make it one-line, so it doesn't get whitespaces in the middle.
On WARN level, instead of logging each skipped type separately, aggreggate the skipped types and issue a WARN log once.
Instead of doing that in two random places, keep just one if-else statement which either: * Sanitizes the debug info, when `--btf` is enabled. * Strips the debug info, when `--btf` is disabled. Also, there is no need to call `DISanitizer::run` in an unsafe block, it's a safe method.
The latter is correct (Oxford Dictionary says so to when searching for the former[0]) and consistent with LLVM's naming. [0] https://www.oed.com/search/dictionary/?scope=Entries&q=sub+program
vadorovsky
force-pushed
the
feature/fix-di
branch
from
867a1f4
to
098ab02
Compare
This was referenced Feb 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Provide a CLI option
--btfwhich provides an experimental BTF support.This includes a sanitization logic, which modifies the debug info in order to make it acceptable by the kernel, which includes:
AyaBtfMapMarker- Linux kernel requires maps to be anonymous structures, but Rust doesn't support anonymous types. This is our way to make it working.static.Updates: #1
Closes: #177 #26