Skip relocations to sections when parsing elf #762
Conversation
When programs contain a section that contain data, e.g., read-only data stored in .rodata section, relocations to the data show up as relocation to the corresponding section, which results in "Unresolved external symbol" error. We can skip when a relocation to a section is encountered to avoid such errors. Signed-off-by: Ameer Hamza <[email protected]>
WalkthroughThe changes in this pull request focus on enhancing error handling and control flow in the ELF file processing functions within Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/asm_files.cpp (1 hunks)
🔇 Additional comments (1)
src/asm_files.cpp (1)
411-414: Correctly skipping relocations for section symbolsThe added check appropriately skips relocations when the symbol type is
ELFIO::STT_SECTION. This change prevents processing relocations to sections, addressing the "Unresolved external symbol" errors encountered with data sections, as described in the PR objectives.
|
@Alan-Jowett can you review this? |
Sure, happy to. |
|
Can anyone comment on what libbpf does in this scenario? I thought this got translated into a lddw with source register 3 (see https://www.ietf.org/archive/id/draft-ietf-bpf-isa-04.html#section-5.4). The verifier should have the same support so that downstream VMs can handle this correctly. |
|
Can anyone comment on what libbpf does in this scenario? I thought this got translated into a lddw with source register 3 (see https://www.ietf.org/archive/id/draft-ietf-bpf-isa-04.html#section-5.4). The verifier should have the same support so that downstream VMs can handle this correctly. The post condition should be that the destination register should contain a pointer to a read-only shared memory region (may need to be a new type?), with a size derived from the symbol information on the ELF. This would result in later code being able to be checked for dereferencing this region and reasoning about it. Let me know if you want to collaborate on getting this right. I think just skipping this is the wrong behavior though. |
|
Example of the problem: So, offset pc=6 -> offset=48 -> 0x30 This plus the disassembly shows this is a 64bit immediate load, with a relocation to the .rodat.config section. So, the correct end to end behavior should be:
The JIT/AOT compiler can then convert this back to a LDDW and embed the address of the corresponding memory region. For reference, see: This is turning into a huge PR comment, so I think I will create an issue listing the required behavior. |
|
Filed #763 with details on how I think this could be implemented. |
|
Thanks @Alan-Jowett for all the effort. I am not super familiar with how maps are initially parsed by the tool, hence the first step (Replace LDDW with source == 0 with LDDW with source == 3) confuses me. I will spend some time trying to understand it. This documentation has been helpful in understanding relocations: https://docs.kernel.org/bpf/llvm_reloc.html |
|
Thanks for the link. I would suggest adding this to the issue so it doesn't get lost. The issue has links to how the code is handling relocation for maps today. |
|
A different approach seems to be needed. Thanks @a-hamza-r! You are more than welcome to open a new PR if you continue to work on this. |
|
Thanks @elazarg. You're right. |
When programs contain a section that stores data, e.g., read-only data stored in the
.rodatasection, relocations to the data show up as relocation to the corresponding section, which results in an "Unresolved external symbol" error. We can skip when a relocation to a section is encountered to avoid such errors. This can be seen for the Cilium benchmark here: bpf_host.oSummary by CodeRabbit
New Features
Bug Fixes
Refactor