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

btf,info: Fix bad instruction offset when parsing infos from kernel #1169

Merged
merged 1 commit into from Oct 19, 2023

Conversation

dylandreimerink
Copy link
Member

When BTF ext info is encoded in ELF the instruction offsets are in bytes, but when you pass them to the kernel they must be instruction indecies. Therefor we devide the offset by the instruction size in the parsing logic for ELF.

This was missed during the initial implementation of reading back BTF ext info from the kernel. This would cause an error when loading back ext info which was not a multiple of the instruction size or a bad instruction offset if it was.

This PR adds an argument to the parsing logic to specify where the ext info is being read from. It also adds LoadKernelLineInfos and LoadKernelFuncInfos to load the ext info from the kernel, using the correct direction enum internaly. These are now used by the info package.

Fixes: #1168

@lmb
Copy link
Collaborator

lmb commented Oct 17, 2023

So how come this wasn't caught by tests? We don't have any non-zero offsets?

@dylandreimerink
Copy link
Member Author

Yea, both of the test that could have caught this have very small programs where this doesn't surface:

=== RUN   TestProgInfoExtBTF
sched_process_exec:
         ; return 0;
        0: MovImm dst: r0 imm: 0
        1: Exit
        
 === RUN   TestProgramInstructions
test_prog:
        0: LdImmDW dst: r0 imm: -1
        2: Exit

Do we want a regression test for this?

@lmb
Copy link
Collaborator

lmb commented Oct 18, 2023

Do we want a regression test for this?

That would be great!

@dylandreimerink
Copy link
Member Author

I changed the TestProgInfoExtBTF test that was added in the original PR to test this feature. It now loads a more complex program with multiple line infos and func infos to verify that this feature works. Should be a better test.

@lmb
Copy link
Collaborator

lmb commented Oct 19, 2023

@dylandreimerink super, thanks! I've pushed a fixup:

  • I don't think we need the explicit LoadKernel.... My POV is that either we parse a full ELF, or we parse parts of the BTF from the kernel.
  • I dislike bool arguments to functions but here it really makes the code easier to read.
  • I simplified the test to not glob for the loader. There is no point in parsing the "wrong" endian files.

Let me know what you think!

@dylandreimerink
Copy link
Member Author

dylandreimerink commented Oct 19, 2023

Yes, all seem sensible! If we can get CI to agree that is. Shall I squash both commits?

When BTF ext info is encoded in ELF the instruction offsets are in
bytes, but when you pass them to the kernel they must be instruction
indices. Therefore we divide the offset by the instruction size in
the parsing logic for ELF.

This was missed during the initial implementation of reading back BTF
ext info from the kernel. This would cause an error when loading
back ext info which was not a multiple of the instruction size or
a bad instruction offset if it was.

Fix this by making LoadLineInfos and LoadFuncInfos work on the kernel
format which contains instruction offsets while the ELF parser uses
bytes instead.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
Co-developed-by: Lorenz Bauer <lmb@isovalent.com>
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.

Thanks, already done. Also adjusted the commit msg.

@lmb lmb merged commit 50436f2 into cilium:main Oct 19, 2023
12 checks passed
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.

ProgramInfo.Instructions fails on parsing BTF line info
2 participants